From b05761f1bbd3827cebf1f05cc833690feeac7cb0 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 21 May 2019 13:31:57 -0700 Subject: [PATCH] Pass State to `preallocateView` and `createView` methods whenever possible Summary: For some components, we will have state as soon as the ShadowNode is created that may be meaningful. In those cases, ViewManagers should be able to use State to create or preallocate views. FB: This will be used in following diffs for Litho support. Reviewed By: mdvacca Differential Revision: D15343702 fbshipit-source-id: 8fd672251cb88dea662b5cae5a9efc96877d28a9 --- .../react/fabric/FabricUIManager.java | 2 ++ .../facebook/react/fabric/jsi/jni/Binding.cpp | 27 ++++++++++++++----- .../fabric/mounting/ContextBasedViewPool.java | 7 ++--- .../fabric/mounting/MountingManager.java | 23 +++++++++------- .../react/fabric/mounting/ViewFactory.java | 4 ++- .../fabric/mounting/ViewManagerFactory.java | 6 +++-- .../react/fabric/mounting/ViewPool.java | 9 ++++--- .../mounting/mountitems/CreateMountItem.java | 2 +- .../mountitems/PreAllocateViewMountItem.java | 6 ++++- .../uimanager/NativeViewHierarchyManager.java | 4 +-- .../facebook/react/uimanager/ViewManager.java | 14 +++++----- .../uimanager/SimpleViewPropertyTest.java | 4 +-- 12 files changed, 67 insertions(+), 41 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index 2f3c6584328..9b9c6f78107 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -176,6 +176,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { int reactTag, final String componentName, @Nullable ReadableMap props, + Object stateWrapper, boolean isLayoutable) { ThemedReactContext context = mReactContextForRootTag.get(rootTag); String component = getFabricComponentName(componentName); @@ -187,6 +188,7 @@ public class FabricUIManager implements UIManager, LifecycleEventListener { reactTag, component, props, + (StateWrapper) stateWrapper, isLayoutable)); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp index daf0fd20e47..10e9551ecbb 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jsi/jni/Binding.cpp @@ -290,14 +290,17 @@ local_ref createUpdateStateMountItem( // Do not hold onto Java object from C // We DO want to hold onto C object from Java, since we don't know the // lifetime of the Java object - auto javaStateWrapper = StateWrapperImpl::newObjectJavaArgs(); - StateWrapperImpl* cStateWrapper = cthis(javaStateWrapper); - cStateWrapper->state_ = state; + local_ref javaStateWrapper = nullptr; + if (state != nullptr) { + javaStateWrapper = StateWrapperImpl::newObjectJavaArgs(); + StateWrapperImpl* cStateWrapper = cthis(javaStateWrapper); + cStateWrapper->state_ = state; + } return updateStateInstruction( javaUIManager, mutation.newChildShadowView.tag, - javaStateWrapper.get()); + (javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr)); } @@ -525,13 +528,23 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation( static auto preallocateView = jni::findClassStatic(UIManagerJavaDescriptor) - ->getMethod("preallocateView"); + ->getMethod("preallocateView"); - local_ref readableMap = + // Do not hold onto Java object from C + // We DO want to hold onto C object from Java, since we don't know the + // lifetime of the Java object + local_ref javaStateWrapper = nullptr; + if (shadowView.state != nullptr) { + javaStateWrapper = StateWrapperImpl::newObjectJavaArgs(); + StateWrapperImpl* cStateWrapper = cthis(javaStateWrapper); + cStateWrapper->state_ = shadowView.state; + } + + local_ref props = castReadableMap(ReadableNativeMap::newObjectCxxArgs(shadowView.props->rawProps)); auto component = getPlatformComponentName(shadowView); preallocateView( - javaUIManager_, surfaceId, shadowView.tag, component.get(), readableMap.get(), isLayoutableShadowNode); + javaUIManager_, surfaceId, shadowView.tag, component.get(), props.get(), (javaStateWrapper != nullptr ? javaStateWrapper.get() : nullptr), isLayoutableShadowNode); } void Binding::registerNatives() { diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ContextBasedViewPool.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ContextBasedViewPool.java index 579e8b73005..3265d6e9577 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ContextBasedViewPool.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ContextBasedViewPool.java @@ -9,6 +9,7 @@ package com.facebook.react.fabric.mounting; import android.view.View; import androidx.annotation.UiThread; import com.facebook.react.uimanager.ReactStylesDiffMap; +import com.facebook.react.uimanager.StateWrapper; import com.facebook.react.uimanager.ThemedReactContext; import com.facebook.react.uimanager.ViewManagerRegistry; import java.util.WeakHashMap; @@ -26,13 +27,13 @@ public final class ContextBasedViewPool implements ViewFactory { @UiThread void createView(ThemedReactContext context, ReactStylesDiffMap props, String componentName) { - getViewPool(context).createView(componentName, props, context); + getViewPool(context).createView(componentName, props, null, context); } @UiThread @Override - public View getOrCreateView(String componentName, ReactStylesDiffMap props, ThemedReactContext context) { - return getViewPool(context).getOrCreateView(componentName, props, context); + public View getOrCreateView(String componentName, ReactStylesDiffMap props, StateWrapper stateWrapper, ThemedReactContext context) { + return getViewPool(context).getOrCreateView(componentName, props, stateWrapper, context); } @UiThread diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java index 675c43c57ca..730db120539 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountingManager.java @@ -170,11 +170,12 @@ public class MountingManager { } @UiThread - public void createViewWithProps( + public void createView( ThemedReactContext themedReactContext, String componentName, int reactTag, @Nullable ReadableMap props, + @Nullable StateWrapper stateWrapper, boolean isLayoutable) { if (mTagToViewState.get(reactTag) != null) { return; @@ -183,19 +184,20 @@ public class MountingManager { View view = null; ViewManager viewManager = null; - ReactStylesDiffMap diffMap = null; + ReactStylesDiffMap propsDiffMap = null; if (props != null) { - diffMap = new ReactStylesDiffMap(props); + propsDiffMap = new ReactStylesDiffMap(props); } if (isLayoutable) { viewManager = mViewManagerRegistry.get(componentName); - view = mViewFactory.getOrCreateView(componentName, diffMap, themedReactContext); + view = mViewFactory.getOrCreateView(componentName, propsDiffMap, stateWrapper, themedReactContext); view.setId(reactTag); } ViewState viewState = new ViewState(reactTag, view, viewManager); - viewState.mCurrentProps = diffMap; + viewState.mCurrentProps = propsDiffMap; + viewState.mCurrentState = (stateWrapper != null ? stateWrapper.getState() : null); mTagToViewState.put(reactTag, viewState); } @@ -313,6 +315,7 @@ public class MountingManager { String componentName, int reactTag, @Nullable ReadableMap props, + @Nullable StateWrapper stateWrapper, boolean isLayoutable) { if (mTagToViewState.get(reactTag) != null) { @@ -320,7 +323,7 @@ public class MountingManager { "View for component " + componentName + " with tag " + reactTag + " already exists."); } - createViewWithProps(reactContext, componentName, reactTag, props, isLayoutable); + createView(reactContext, componentName, reactTag, props, stateWrapper, isLayoutable); } @UiThread @@ -362,10 +365,10 @@ public class MountingManager { final int mReactTag; final boolean mIsRoot; @Nullable final ViewManager mViewManager; - public ReactStylesDiffMap mCurrentProps; - public ReadableMap mCurrentLocalData; - public ReadableMap mCurrentState; - public EventEmitterWrapper mEventEmitter; + public ReactStylesDiffMap mCurrentProps = null; + public ReadableMap mCurrentLocalData = null; + public ReadableMap mCurrentState = null; + public EventEmitterWrapper mEventEmitter = null; private ViewState(int reactTag, @Nullable View view, @Nullable ViewManager viewManager) { this(reactTag, view, viewManager, false); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewFactory.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewFactory.java index b92f576ebda..086e143b16d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewFactory.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewFactory.java @@ -8,11 +8,13 @@ package com.facebook.react.fabric.mounting; import android.view.View; import com.facebook.react.uimanager.ReactStylesDiffMap; +import com.facebook.react.uimanager.StateWrapper; import com.facebook.react.uimanager.ThemedReactContext; +import javax.annotation.Nullable; public interface ViewFactory { - View getOrCreateView(String componentName, ReactStylesDiffMap props, ThemedReactContext context); + View getOrCreateView(String componentName, @Nullable ReactStylesDiffMap props, @Nullable StateWrapper stateWrapper, ThemedReactContext context); void recycle(ThemedReactContext context, String componentName, View view); diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewManagerFactory.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewManagerFactory.java index dffac26f5a6..3e9236263ec 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewManagerFactory.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewManagerFactory.java @@ -9,8 +9,10 @@ package com.facebook.react.fabric.mounting; import androidx.annotation.UiThread; import android.view.View; import com.facebook.react.uimanager.ReactStylesDiffMap; +import com.facebook.react.uimanager.StateWrapper; import com.facebook.react.uimanager.ThemedReactContext; import com.facebook.react.uimanager.ViewManagerRegistry; +import javax.annotation.Nullable; public class ViewManagerFactory implements ViewFactory { @@ -23,8 +25,8 @@ public class ViewManagerFactory implements ViewFactory { @UiThread @Override public View getOrCreateView( - String componentName, ReactStylesDiffMap props, ThemedReactContext context) { - return mViewManagerRegistry.get(componentName).createViewWithProps(context, props, null); + String componentName, @Nullable ReactStylesDiffMap props, @Nullable StateWrapper stateWrapper, ThemedReactContext context) { + return mViewManagerRegistry.get(componentName).createView(context, props, stateWrapper, null); } @UiThread diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewPool.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewPool.java index 69a2c911c8d..4fc769aec83 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewPool.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/ViewPool.java @@ -10,6 +10,7 @@ import androidx.annotation.UiThread; import android.view.View; import com.facebook.react.common.ClearableSynchronizedPool; import com.facebook.react.uimanager.ReactStylesDiffMap; +import com.facebook.react.uimanager.StateWrapper; import com.facebook.react.uimanager.ThemedReactContext; import com.facebook.react.uimanager.ViewManager; import com.facebook.react.uimanager.ViewManagerRegistry; @@ -26,20 +27,20 @@ public final class ViewPool { } @UiThread - void createView(String componentName, ReactStylesDiffMap props, ThemedReactContext context) { + void createView(String componentName, ReactStylesDiffMap props, StateWrapper stateWrapper, ThemedReactContext context) { ClearableSynchronizedPool viewPool = getViewPoolForComponent(componentName); ViewManager viewManager = mViewManagerRegistry.get(componentName); // TODO: T31905686 Integrate / re-implement jsResponder - View view = viewManager.createViewWithProps(context, props, null); + View view = viewManager.createView(context, props, stateWrapper, null); viewPool.release(view); } @UiThread - View getOrCreateView(String componentName, ReactStylesDiffMap props, ThemedReactContext context) { + View getOrCreateView(String componentName, ReactStylesDiffMap props, StateWrapper stateWrapper, ThemedReactContext context) { ClearableSynchronizedPool viewPool = getViewPoolForComponent(componentName); View view = viewPool.acquire(); if (view == null) { - createView(componentName, props, context); + createView(componentName, props, stateWrapper, context); view = viewPool.acquire(); } return view; diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java index af518f1c471..980e7800cfe 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/CreateMountItem.java @@ -32,7 +32,7 @@ public class CreateMountItem implements MountItem { @Override public void execute(MountingManager mountingManager) { - mountingManager.createViewWithProps(mContext, mComponent, mReactTag, null, mIsLayoutable); + mountingManager.createView(mContext, mComponent, mReactTag, null, null, mIsLayoutable); } @Override diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java index ca117d45877..00d0f65c009 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PreAllocateViewMountItem.java @@ -13,6 +13,7 @@ import androidx.annotation.Nullable; import com.facebook.common.logging.FLog; import com.facebook.react.bridge.ReadableMap; import com.facebook.react.fabric.mounting.MountingManager; +import com.facebook.react.uimanager.StateWrapper; import com.facebook.react.uimanager.ThemedReactContext; /** {@link MountItem} that is used to pre-allocate views for JS components. */ @@ -22,6 +23,7 @@ public class PreAllocateViewMountItem implements MountItem { private final int mRootTag; private final int mReactTag; private final @Nullable ReadableMap mProps; + private final @Nullable StateWrapper mStateWrapper; private final ThemedReactContext mContext; private final boolean mIsLayoutable; @@ -31,11 +33,13 @@ public class PreAllocateViewMountItem implements MountItem { int reactTag, String component, @Nullable ReadableMap props, + StateWrapper stateWrapper, boolean isLayoutable) { mContext = context; mComponent = component; mRootTag = rootTag; mProps = props; + mStateWrapper = stateWrapper; mReactTag = reactTag; mIsLayoutable = isLayoutable; } @@ -45,7 +49,7 @@ public class PreAllocateViewMountItem implements MountItem { if (DEBUG) { FLog.d(TAG, "Executing pre-allocation of: " + toString()); } - mountingManager.preallocateView(mContext, mComponent, mReactTag, mProps, mIsLayoutable); + mountingManager.preallocateView(mContext, mComponent, mReactTag, mProps, mStateWrapper, mIsLayoutable); } @Override diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java index 724dd8e81d0..48d1daefd5f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -8,7 +8,6 @@ package com.facebook.react.uimanager; import android.content.res.Resources; -import android.os.Build; import android.util.SparseArray; import android.util.SparseBooleanArray; import android.util.SparseIntArray; @@ -19,7 +18,6 @@ import android.view.ViewGroup; import android.view.ViewParent; import android.widget.PopupMenu; import com.facebook.common.logging.FLog; -import com.facebook.infer.annotation.Assertions; import com.facebook.react.R; import com.facebook.react.bridge.Callback; import com.facebook.react.bridge.JSApplicationIllegalArgumentException; @@ -256,7 +254,7 @@ public class NativeViewHierarchyManager { try { ViewManager viewManager = mViewManagers.get(className); - View view = viewManager.createViewWithProps(themedContext, null, mJSResponderHandler); + View view = viewManager.createView(themedContext, null, null, mJSResponderHandler); mTagsToViews.put(tag, view); mTagsToViewManagers.put(tag, viewManager); diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java index 7100f699c40..a0f7c9ba51c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewManager.java @@ -11,10 +11,8 @@ import android.content.Context; import android.view.View; import com.facebook.react.bridge.BaseJavaModule; import com.facebook.react.bridge.ReactApplicationContext; -import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.bridge.ReadableMap; -import com.facebook.react.uimanager.StateWrapper; import com.facebook.react.touch.JSResponderHandler; import com.facebook.react.touch.ReactInterceptingViewGroup; import com.facebook.react.uimanager.annotations.ReactProp; @@ -41,6 +39,7 @@ public abstract class ViewManager * * @param viewToUpdate * @param props + * @param stateWrapper */ public void updateProperties(@Nonnull T viewToUpdate, ReactStylesDiffMap props) { ViewManagerPropertyUpdater.updateProps(this, viewToUpdate, props); @@ -53,17 +52,18 @@ public abstract class ViewManager private final @Nonnull T createView( @Nonnull ThemedReactContext reactContext, JSResponderHandler jsResponderHandler) { - return this.createViewWithProps(reactContext, null, jsResponderHandler); + return createView(reactContext, null, null, jsResponderHandler); } /** * Creates a view with knowledge of props. */ - public @Nonnull T createViewWithProps( + public @Nonnull T createView( @Nonnull ThemedReactContext reactContext, - ReactStylesDiffMap props, + @Nullable ReactStylesDiffMap props, + @Nullable StateWrapper stateWrapper, JSResponderHandler jsResponderHandler) { - T view = createViewInstanceWithProps(reactContext, props); + T view = createViewInstance(reactContext, props, stateWrapper); addEventEmitters(reactContext, view); if (view instanceof ReactInterceptingViewGroup) { ((ReactInterceptingViewGroup) view).setOnInterceptTouchEventListener(jsResponderHandler); @@ -115,7 +115,7 @@ public abstract class ViewManager * Override it if you need props upon creation of the view. * @param reactContext */ - protected @Nonnull T createViewInstanceWithProps(@Nonnull ThemedReactContext reactContext, ReactStylesDiffMap initialProps) { + protected @Nonnull T createViewInstance(@Nonnull ThemedReactContext reactContext, @Nullable ReactStylesDiffMap initialProps, @Nullable StateWrapper stateWrapper) { T view = createViewInstance(reactContext); if (initialProps != null) { updateProperties(view, initialProps); diff --git a/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java b/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java index e054da2e808..c66d9bf48df 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/uimanager/SimpleViewPropertyTest.java @@ -83,7 +83,7 @@ public class SimpleViewPropertyTest { @Test public void testOpacity() { - View view = mManager.createViewWithProps(mThemedContext, buildStyles(), new JSResponderHandler()); + View view = mManager.createView(mThemedContext, buildStyles(), null, new JSResponderHandler()); mManager.updateProperties(view, buildStyles()); assertThat(view.getAlpha()).isEqualTo(1.0f); @@ -97,7 +97,7 @@ public class SimpleViewPropertyTest { @Test public void testBackgroundColor() { - View view = mManager.createViewWithProps(mThemedContext, buildStyles(), new JSResponderHandler()); + View view = mManager.createView(mThemedContext, buildStyles(), null, new JSResponderHandler()); mManager.updateProperties(view, buildStyles()); assertThat(view.getBackground()).isEqualTo(null);