From 8125ce520df54ce29f8e70bcf25e4eef7623be3c Mon Sep 17 00:00:00 2001 From: Aaron Chiu Date: Wed, 31 May 2017 02:16:43 -0700 Subject: [PATCH] don't block attaching ReactRootView on measuring Reviewed By: achen1 Differential Revision: D5117394 fbshipit-source-id: 00f65a59247a75d4b42240fe25935aa9bd8948b1 --- .../react/tests/CatalystUIManagerTestCase.java | 5 ++--- .../react/tests/ProgressBarTestCase.java | 3 +-- .../react/tests/ViewRenderingTestCase.java | 3 +-- .../facebook/react/ReactInstanceManager.java | 12 ++++++------ .../java/com/facebook/react/ReactRootView.java | 17 +++++------------ .../uimanager/NativeViewHierarchyManager.java | 4 ++-- .../uimanager/SizeMonitoringFrameLayout.java | 1 - .../react/uimanager/UIImplementation.java | 2 +- .../react/uimanager/UIManagerModule.java | 12 ++++++------ .../react/uimanager/UIManagerModuleTest.java | 8 ++++---- .../react/views/text/ReactTextTest.java | 2 +- .../react/views/textinput/TextInputTest.java | 4 ++-- 12 files changed, 31 insertions(+), 42 deletions(-) diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/CatalystUIManagerTestCase.java b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/CatalystUIManagerTestCase.java index 3bcecc6ec..a0d65c9bc 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/CatalystUIManagerTestCase.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/CatalystUIManagerTestCase.java @@ -25,7 +25,6 @@ import com.facebook.react.modules.appstate.AppStateModule; import com.facebook.react.modules.deviceinfo.DeviceInfoModule; import com.facebook.react.modules.systeminfo.AndroidInfoModule; import com.facebook.react.uimanager.PixelUtil; -import com.facebook.react.uimanager.UIImplementation; import com.facebook.react.uimanager.UIImplementationProvider; import com.facebook.react.uimanager.UIManagerModule; import com.facebook.react.uimanager.ViewManager; @@ -66,7 +65,7 @@ public class CatalystUIManagerTestCase extends ReactIntegrationTestCase { final DisplayMetrics metrics = getContext().getResources().getDisplayMetrics(); rootView.setLayoutParams( new FrameLayout.LayoutParams(metrics.widthPixels, metrics.heightPixels)); - uiManager.addMeasuredRootView(rootView); + uiManager.addRootView(rootView); // We add the root view by posting to the main thread so wait for that to complete so that the // root view tag is added to the view waitForIdleSync(); @@ -228,7 +227,7 @@ public class CatalystUIManagerTestCase extends ReactIntegrationTestCase { public void _testCenteredText(String text) { ReactRootView rootView = new ReactRootView(getContext()); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); jsModule.renderCenteredTextViewTestApplication(rootTag, text); waitForBridgeAndUIIdle(); diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ProgressBarTestCase.java b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ProgressBarTestCase.java index 40983432a..ea99a3ed7 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ProgressBarTestCase.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ProgressBarTestCase.java @@ -27,7 +27,6 @@ import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.modules.appstate.AppStateModule; import com.facebook.react.modules.deviceinfo.DeviceInfoModule; import com.facebook.react.modules.systeminfo.AndroidInfoModule; -import com.facebook.react.uimanager.UIImplementation; import com.facebook.react.uimanager.UIImplementationProvider; import com.facebook.react.uimanager.UIManagerModule; import com.facebook.react.uimanager.ViewManager; @@ -98,7 +97,7 @@ public class ProgressBarTestCase extends ReactIntegrationTestCase { DisplayMetrics metrics = getContext().getResources().getDisplayMetrics(); mRootView.setLayoutParams( new FrameLayout.LayoutParams(metrics.widthPixels, metrics.heightPixels)); - int rootTag = mUIManager.addMeasuredRootView(mRootView); + int rootTag = mUIManager.addRootView(mRootView); mInstance.getJSModule(ProgressBarTestModule.class).renderProgressBarApplication(rootTag); waitForBridgeAndUIIdle(); } diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ViewRenderingTestCase.java b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ViewRenderingTestCase.java index 956269d96..daacff84f 100644 --- a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ViewRenderingTestCase.java +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/ViewRenderingTestCase.java @@ -23,7 +23,6 @@ import com.facebook.react.modules.appstate.AppStateModule; import com.facebook.react.modules.deviceinfo.DeviceInfoModule; import com.facebook.react.modules.systeminfo.AndroidInfoModule; import com.facebook.react.uimanager.PixelUtil; -import com.facebook.react.uimanager.UIImplementation; import com.facebook.react.uimanager.UIImplementationProvider; import com.facebook.react.uimanager.UIManagerModule; import com.facebook.react.uimanager.ViewManager; @@ -76,7 +75,7 @@ public class ViewRenderingTestCase extends ReactIntegrationTestCase { .build(); mRootView = new ReactRootView(getContext()); - mRootTag = uiManager.addMeasuredRootView(mRootView); + mRootTag = uiManager.addRootView(mRootView); } public void testViewRenderedWithCorrectProperties() { diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index 27d105b08..2ea99c030 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java @@ -613,7 +613,7 @@ public class ReactInstanceManager { * be re-attached. */ @ThreadConfined(UI) - public void attachMeasuredRootView(ReactRootView rootView) { + public void attachRootView(ReactRootView rootView) { UiThreadUtil.assertOnUiThread(); mAttachedRootViews.add(rootView); @@ -624,7 +624,7 @@ public class ReactInstanceManager { // If react context is being created in the background, JS application will be started // automatically when creation completes, as root view is part of the attached root view list. if (mCreateReactContextThread == null && mCurrentReactContext != null) { - attachMeasuredRootViewToInstance(rootView, mCurrentReactContext.getCatalystInstance()); + attachRootViewToInstance(rootView, mCurrentReactContext.getCatalystInstance()); } } @@ -814,7 +814,7 @@ public class ReactInstanceManager { ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_START); synchronized (mAttachedRootViews) { for (ReactRootView rootView : mAttachedRootViews) { - attachMeasuredRootViewToInstance(rootView, catalystInstance); + attachRootViewToInstance(rootView, catalystInstance); } } ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END); @@ -850,16 +850,16 @@ public class ReactInstanceManager { } } - private void attachMeasuredRootViewToInstance( + private void attachRootViewToInstance( final ReactRootView rootView, CatalystInstance catalystInstance) { - Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "attachMeasuredRootViewToInstance"); + Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "attachRootViewToInstance"); if (!mSetupReactContextInBackgroundEnabled) { UiThreadUtil.assertOnUiThread(); } UIManagerModule uiManagerModule = catalystInstance.getNativeModule(UIManagerModule.class); - final int rootTag = uiManagerModule.addMeasuredRootView(rootView); + final int rootTag = uiManagerModule.addRootView(rootView); rootView.setRootViewTag(rootTag); rootView.runApplication(); Systrace.beginAsyncSection( diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java b/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java index 654e6b770..489b2394d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java @@ -78,8 +78,7 @@ public class ReactRootView extends SizeMonitoringFrameLayout implements RootView private @Nullable CustomGlobalLayoutListener mCustomGlobalLayoutListener; private @Nullable ReactRootViewEventListener mRootViewEventListener; private int mRootViewTag; - private boolean mWasMeasured = false; - private boolean mIsAttachedToInstance = false; + private boolean mIsAttachedToInstance; private final JSTouchDispatcher mJSTouchDispatcher = new JSTouchDispatcher(this); public ReactRootView(Context context) { @@ -102,7 +101,6 @@ public class ReactRootView extends SizeMonitoringFrameLayout implements RootView MeasureSpec.getSize(widthMeasureSpec), MeasureSpec.getSize(heightMeasureSpec)); - mWasMeasured = true; // Check if we were waiting for onMeasure to attach the root view. if (mReactInstanceManager != null && !mIsAttachedToInstance) { attachToReactInstanceManager(); @@ -222,12 +220,7 @@ public class ReactRootView extends SizeMonitoringFrameLayout implements RootView mReactInstanceManager.createReactContextInBackground(); } - // We need to wait for the initial onMeasure, if this view has not yet been measured, we set - // which will make this view startReactApplication itself to instance manager once onMeasure - // is called. - if (mWasMeasured) { - attachToReactInstanceManager(); - } + attachToReactInstanceManager(); } finally { Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); } @@ -303,13 +296,12 @@ public class ReactRootView extends SizeMonitoringFrameLayout implements RootView } /** - * Is used by unit test to setup mWasMeasured and mIsAttachedToWindow flags, that will let this + * Is used by unit test to setup mIsAttachedToWindow flags, that will let this * view to be properly attached to catalyst instance by startReactApplication call */ @VisibleForTesting /* package */ void simulateAttachForTesting() { mIsAttachedToInstance = true; - mWasMeasured = true; } private CustomGlobalLayoutListener getCustomGlobalLayoutListener() { @@ -327,7 +319,7 @@ public class ReactRootView extends SizeMonitoringFrameLayout implements RootView } mIsAttachedToInstance = true; - Assertions.assertNotNull(mReactInstanceManager).attachMeasuredRootView(this); + Assertions.assertNotNull(mReactInstanceManager).attachRootView(this); getViewTreeObserver().addOnGlobalLayoutListener(getCustomGlobalLayoutListener()); } finally { Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); @@ -362,6 +354,7 @@ public class ReactRootView extends SizeMonitoringFrameLayout implements RootView private int mDeviceRotation = 0; /* package */ CustomGlobalLayoutListener() { + DisplayMetricsHolder.initDisplayMetricsIfNotInitialized(getContext().getApplicationContext()); mVisibleViewArea = new Rect(); mMinKeyboardHeightDetected = (int) PixelUtil.toPixelFromDIP(60); } 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 4a1dc0d9d..a5a53b314 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java @@ -487,7 +487,7 @@ public class NativeViewHierarchyManager { } /** - * See {@link UIManagerModule#addMeasuredRootView}. + * See {@link UIManagerModule#addRootView}. */ public synchronized void addRootView( int tag, @@ -504,7 +504,7 @@ public class NativeViewHierarchyManager { throw new IllegalViewOperationException( "Trying to add a root view with an explicit id already set. React Native uses " + "the id field to track react tags and will overwrite this field. If that is fine, " + - "explicitly overwrite the id field to View.NO_ID before calling addMeasuredRootView."); + "explicitly overwrite the id field to View.NO_ID before calling addRootView."); } mTagsToViews.put(tag, view); diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/SizeMonitoringFrameLayout.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/SizeMonitoringFrameLayout.java index fbfd531c8..fa952878d 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/SizeMonitoringFrameLayout.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/SizeMonitoringFrameLayout.java @@ -52,5 +52,4 @@ public class SizeMonitoringFrameLayout extends FrameLayout { mOnSizeChangedListener.onSizeChanged(w, h, oldw, oldh); } } - } diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java index 6fc29266f..6464c4503 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIImplementation.java @@ -14,7 +14,6 @@ import java.util.Arrays; import java.util.List; import com.facebook.common.logging.FLog; -import com.facebook.yoga.YogaDirection; import com.facebook.infer.annotation.Assertions; import com.facebook.react.animation.Animation; import com.facebook.react.bridge.Arguments; @@ -30,6 +29,7 @@ import com.facebook.react.uimanager.debug.NotThreadSafeViewHierarchyUpdateDebugL import com.facebook.react.uimanager.events.EventDispatcher; import com.facebook.systrace.Systrace; import com.facebook.systrace.SystraceMessage; +import com.facebook.yoga.YogaDirection; /** * An class that is used to receive React commands from JS and translate them into a diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java index ecdfd2837..d6b7d554b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java @@ -181,13 +181,13 @@ public class UIManagerModule extends ReactContextBaseJavaModule implements * Note that this must be called after getWidth()/getHeight() actually return something. See * CatalystApplicationFragment as an example. * - * TODO(6242243): Make addMeasuredRootView thread safe + * TODO(6242243): Make addRootView thread safe * NB: this method is horribly not-thread-safe. */ - public int addMeasuredRootView(final SizeMonitoringFrameLayout rootView) { + public int addRootView(final SizeMonitoringFrameLayout rootView) { Systrace.beginSection( Systrace.TRACE_TAG_REACT_JAVA_BRIDGE, - "UIManagerModule.addMeasuredRootView"); + "UIManagerModule.addRootView"); final int tag = mNextRootViewTag; mNextRootViewTag += ROOT_VIEW_TAG_INCREMENT; @@ -195,8 +195,8 @@ public class UIManagerModule extends ReactContextBaseJavaModule implements final int height; // If LayoutParams sets size explicitly, we can use that. Otherwise get the size from the view. if (rootView.getLayoutParams() != null && - rootView.getLayoutParams().width > 0 && - rootView.getLayoutParams().height > 0) { + rootView.getLayoutParams().width > 0 && + rootView.getLayoutParams().height > 0) { width = rootView.getLayoutParams().width; height = rootView.getLayoutParams().height; } else { @@ -206,7 +206,7 @@ public class UIManagerModule extends ReactContextBaseJavaModule implements final ReactApplicationContext reactApplicationContext = getReactApplicationContext(); final ThemedReactContext themedRootContext = - new ThemedReactContext(reactApplicationContext, rootView.getContext()); + new ThemedReactContext(reactApplicationContext, rootView.getContext()); mUIImplementation.registerRootView(rootView, tag, width, height, themedRootContext); diff --git a/ReactAndroid/src/test/java/com/facebook/react/uimanager/UIManagerModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/uimanager/UIManagerModuleTest.java index a54f5c2a6..8b58712ee 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/uimanager/UIManagerModuleTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/uimanager/UIManagerModuleTest.java @@ -151,7 +151,7 @@ public class UIManagerModuleTest { ReactRootView rootView = new ReactRootView(RuntimeEnvironment.application.getApplicationContext()); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); int viewTag = rootTag + 1; int subViewTag = viewTag + 1; @@ -609,7 +609,7 @@ public class UIManagerModuleTest { UIManagerModule uiManager = getUIManagerModule(); ReactRootView rootView = new ReactRootView(RuntimeEnvironment.application.getApplicationContext()); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); final int containerTag = rootTag + 1; final int containerSiblingTag = containerTag + 1; @@ -662,7 +662,7 @@ public class UIManagerModuleTest { private ViewGroup createSimpleTextHierarchy(UIManagerModule uiManager, String text) { ReactRootView rootView = new ReactRootView(RuntimeEnvironment.application.getApplicationContext()); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); int textTag = rootTag + 1; int rawTextTag = textTag + 1; @@ -701,7 +701,7 @@ public class UIManagerModuleTest { private TestMoveDeleteHierarchy createMoveDeleteHierarchy(UIManagerModule uiManager) { ReactRootView rootView = new ReactRootView(mReactContext); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); TestMoveDeleteHierarchy hierarchy = new TestMoveDeleteHierarchy(rootView, rootTag); diff --git a/ReactAndroid/src/test/java/com/facebook/react/views/text/ReactTextTest.java b/ReactAndroid/src/test/java/com/facebook/react/views/text/ReactTextTest.java index cf7d860a9..3d838394e 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/views/text/ReactTextTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/views/text/ReactTextTest.java @@ -379,7 +379,7 @@ public class ReactTextTest { JavaOnlyMap textProps, JavaOnlyMap rawTextProps) { ReactRootView rootView = new ReactRootView(RuntimeEnvironment.application); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); int textTag = rootTag + 1; int rawTextTag = textTag + 1; diff --git a/ReactAndroid/src/test/java/com/facebook/react/views/textinput/TextInputTest.java b/ReactAndroid/src/test/java/com/facebook/react/views/textinput/TextInputTest.java index 9b189414e..1f804f11b 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/views/textinput/TextInputTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/views/textinput/TextInputTest.java @@ -91,7 +91,7 @@ public class TextInputTest { ReactRootView rootView = new ReactRootView(RuntimeEnvironment.application); rootView.setLayoutParams(new ReactRootView.LayoutParams(100, 100)); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); int textInputTag = rootTag + 1; final String hintStr = "placeholder text"; @@ -125,7 +125,7 @@ public class TextInputTest { ReactRootView rootView = new ReactRootView(RuntimeEnvironment.application); rootView.setLayoutParams(new ReactRootView.LayoutParams(100, 100)); - int rootTag = uiManager.addMeasuredRootView(rootView); + int rootTag = uiManager.addRootView(rootView); int textInputTag = rootTag + 1; final String hintStr = "placeholder text";