From d63ba47b59e3261403800c1f741d979a089efb48 Mon Sep 17 00:00:00 2001 From: Andy Street Date: Mon, 21 Nov 2016 06:16:27 -0800 Subject: [PATCH] BREAKING [react_native] Move to new C-based implementation of css-layout in RN Android Summary: Moves from CSSNodeDEPRECATED to CSSNode. This has shown to be a huge performance win for layout time within FB. This is BREAKING because CSSNode contains bug fixes that were not migrated to CSSNodeDEPRECATED which may change the way your layout appears. The most common of these by far involves `flex: 1`. Previously, developers had to put `flex: 1` in many places it didn't belong in order to work around a bug in css-layout. Now `flex: 1` is treated properly and, unfortunately, this means that your layout may no longer look correct. Specifically, you may see that your layout looks collapsed, or children don't render. The fix is to simply remove `flex: 1` from those containers. Reviewed By: emilsjolander Differential Revision: D3992787 fbshipit-source-id: 7a3a2a34a8941c0524e6ba3c5379e434d3e03247 --- ReactAndroid/DEFS | 27 +++++++++++------- .../src/androidTest/js/UIManagerTestModule.js | 1 - .../react/uimanager/ReactShadowNode.java | 10 +++---- .../text/ReactTextInlineImageShadowNode.java | 5 ++-- ...coBasedReactTextInlineImageShadowNode.java | 4 +-- .../main/jni/first-party/csslayoutjni/BUCK | 23 +++++++++++++++ ReactAndroid/src/main/jni/first-party/fb/BUCK | 27 ++++++++++++++++++ .../src/test/java/com/facebook/react/BUCK | 2 +- .../java/com/facebook/react/animated/BUCK | 2 +- .../test/java/com/facebook/react/bridge/BUCK | 2 +- .../java/com/facebook/react/devsupport/BUCK | 2 +- .../test/java/com/facebook/react/modules/BUCK | 2 +- .../java/com/facebook/react/uimanager/BUCK | 2 +- .../ReactPropForShadowNodeSetterTest.java | 28 ++++++++++++++++--- .../test/java/com/facebook/react/views/BUCK | 2 +- ReactCommon/CSSLayout/BUCK | 18 ++++++++++++ 16 files changed, 124 insertions(+), 33 deletions(-) create mode 100644 ReactAndroid/src/main/jni/first-party/csslayoutjni/BUCK create mode 100644 ReactAndroid/src/main/jni/first-party/fb/BUCK create mode 100644 ReactCommon/CSSLayout/BUCK diff --git a/ReactAndroid/DEFS b/ReactAndroid/DEFS index 340519f6b..f6d0b4a76 100644 --- a/ReactAndroid/DEFS +++ b/ReactAndroid/DEFS @@ -4,6 +4,8 @@ # - At Facebook by running buck from the root of the fb repo # - Outside of Facebook by running buck in the root of the git repo +IS_OSS_BUILD = True + with allow_unsafe_import(): import os @@ -35,6 +37,8 @@ JSC_DEPS = [ CSSLAYOUT_TARGET = '//ReactAndroid/src/main/java/com/facebook:csslayout' FBGLOGINIT_TARGET = '//ReactAndroid/src/main/jni/first-party/fbgloginit:fbgloginit' +FBJNI_TARGET = '//ReactAndroid/src/main/jni/first-party/fb:jni' +JNI_TARGET = '//ReactAndroid/src/main/jni/first-party/jni-hack:jni-hack' # React property preprocessor original_android_library=android_library @@ -76,9 +80,7 @@ def android_library( *args, **kwargs) - -def robolectric3_test(name, deps, vm_args=None, *args, **kwargs): - +def rn_robolectric_test(name, srcs, vm_args = None, *args, **kwargs): vm_args = vm_args or [] # We may need to create buck-out/gen/ if we're running after buck clean. @@ -100,11 +102,16 @@ def robolectric3_test(name, deps, vm_args=None, *args, **kwargs): '-Djava.io.tmpdir=%s' % os.path.join(os.path.abspath('.'), 'buck-out/bin')) - # defined in BUCK + # RN tests use Powermock, which means they get their own ClassLoaders. + # Because the csslayout native library (or any native library) can only be loaded into one + # ClassLoader at a time, we need to run each in its own process, hence fork_mode = 'per_test'. robolectric_test( - name=name, - deps=deps, - vm_args=vm_args + extra_vm_args, - *args, - **kwargs - ) + name = name, + use_cxx_libraries = True, + cxx_library_whitelist = [ + '//ReactAndroid/src/main/jni/first-party/csslayoutjni:jni', + ], + fork_mode = 'per_test', + srcs = srcs, + vm_args = vm_args + extra_vm_args, + *args, **kwargs) diff --git a/ReactAndroid/src/androidTest/js/UIManagerTestModule.js b/ReactAndroid/src/androidTest/js/UIManagerTestModule.js index f8896a12c..dd63b9e18 100644 --- a/ReactAndroid/src/androidTest/js/UIManagerTestModule.js +++ b/ReactAndroid/src/androidTest/js/UIManagerTestModule.js @@ -119,7 +119,6 @@ var CenteredTextView = React.createClass({ width: 200, height: 100, backgroundColor: '#aa3311', - flex: 1, justifyContent: 'center', alignItems: 'center', }, diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java index fe9c11c04..8017ffedc 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactShadowNode.java @@ -19,8 +19,8 @@ import com.facebook.csslayout.CSSDirection; import com.facebook.csslayout.CSSFlexDirection; import com.facebook.csslayout.CSSJustify; import com.facebook.csslayout.CSSLayoutContext; +import com.facebook.csslayout.CSSNode; import com.facebook.csslayout.CSSNodeAPI; -import com.facebook.csslayout.CSSNodeDEPRECATED; import com.facebook.csslayout.CSSOverflow; import com.facebook.csslayout.CSSPositionType; import com.facebook.csslayout.CSSWrap; @@ -30,9 +30,9 @@ import com.facebook.react.uimanager.annotations.ReactPropertyHolder; /** * Base node class for representing virtual tree of React nodes. Shadow nodes are used primarily - * for layouting therefore it extends {@link CSSNodeDEPRECATED} to allow that. They also help with handling - * Common base subclass of {@link CSSNodeDEPRECATED} for all layout nodes for react-based view. It extends - * {@link CSSNodeDEPRECATED} by adding additional capabilities. + * for layouting therefore it extends {@link CSSNode} to allow that. They also help with handling + * Common base subclass of {@link CSSNode} for all layout nodes for react-based view. It extends + * {@link CSSNode} by adding additional capabilities. * * Instances of this class receive property updates from JS via @{link UIManagerModule}. Subclasses * may use {@link #updateShadowNode} to persist some of the updated fields in the node instance that @@ -74,7 +74,7 @@ public class ReactShadowNode { private float mAbsoluteBottom; private final Spacing mDefaultPadding = new Spacing(0); private final Spacing mPadding = new Spacing(CSSConstants.UNDEFINED); - private final CSSNodeDEPRECATED mCSSNode = new CSSNodeDEPRECATED(); + private final CSSNode mCSSNode = new CSSNode(); /** * Nodes that return {@code true} will be treated as "virtual" nodes. That is, nodes that are not diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextInlineImageShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextInlineImageShadowNode.java index 33bba4042..cde66795b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextInlineImageShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextInlineImageShadowNode.java @@ -9,11 +9,11 @@ package com.facebook.react.views.text; -import com.facebook.csslayout.CSSNodeDEPRECATED; +import com.facebook.csslayout.CSSNode; import com.facebook.react.uimanager.LayoutShadowNode; /** - * Base class for {@link CSSNodeDEPRECATED}s that represent inline images. + * Base class for {@link CSSNode}s that represent inline images. */ public abstract class ReactTextInlineImageShadowNode extends LayoutShadowNode { @@ -22,5 +22,4 @@ public abstract class ReactTextInlineImageShadowNode extends LayoutShadowNode { * place of this node. */ public abstract TextInlineImageSpan buildInlineImageSpan(); - } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/text/frescosupport/FrescoBasedReactTextInlineImageShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/views/text/frescosupport/FrescoBasedReactTextInlineImageShadowNode.java index b7289aaf8..34bf0fbe0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/text/frescosupport/FrescoBasedReactTextInlineImageShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/text/frescosupport/FrescoBasedReactTextInlineImageShadowNode.java @@ -18,7 +18,6 @@ import android.content.res.Resources; import android.net.Uri; import com.facebook.common.util.UriUtil; -import com.facebook.csslayout.CSSNodeDEPRECATED; import com.facebook.drawee.controller.AbstractDraweeControllerBuilder; import com.facebook.react.bridge.ReadableArray; import com.facebook.react.uimanager.annotations.ReactProp; @@ -26,7 +25,7 @@ import com.facebook.react.views.text.ReactTextInlineImageShadowNode; import com.facebook.react.views.text.TextInlineImageSpan; /** - * {@link CSSNodeDEPRECATED} that represents an inline image. Loading is done using Fresco. + * Shadow node that represents an inline image. Loading is done using Fresco. * */ public class FrescoBasedReactTextInlineImageShadowNode extends ReactTextInlineImageShadowNode { @@ -113,5 +112,4 @@ public class FrescoBasedReactTextInlineImageShadowNode extends ReactTextInlineIm public @Nullable Object getCallerContext() { return mCallerContext; } - } diff --git a/ReactAndroid/src/main/jni/first-party/csslayoutjni/BUCK b/ReactAndroid/src/main/jni/first-party/csslayoutjni/BUCK new file mode 100644 index 000000000..540323fa9 --- /dev/null +++ b/ReactAndroid/src/main/jni/first-party/csslayoutjni/BUCK @@ -0,0 +1,23 @@ +include_defs('//ReactAndroid/DEFS') + +# This target is only used in open source +if IS_OSS_BUILD: + cxx_library( + name = 'jni', + soname = 'libcsslayout.$(ext)', + srcs = glob(['jni/*.cpp']), + header_namespace = '', + compiler_flags = [ + '-fno-omit-frame-pointer', + '-fexceptions', + '-Wall', + '-Werror', + '-O3', + '-std=c++11', + ], + deps = [ + '//ReactCommon/CSSLayout:CSSLayout', + FBJNI_TARGET, + ], + visibility = ['PUBLIC'], + ) diff --git a/ReactAndroid/src/main/jni/first-party/fb/BUCK b/ReactAndroid/src/main/jni/first-party/fb/BUCK new file mode 100644 index 000000000..8d9b07cc0 --- /dev/null +++ b/ReactAndroid/src/main/jni/first-party/fb/BUCK @@ -0,0 +1,27 @@ +include_defs('//ReactAndroid/DEFS') + +# This target is only used in open source +if IS_OSS_BUILD: + cxx_library( + name = 'jni', + soname = 'libfb.$(ext)', + srcs = glob(['*.cpp', 'jni/*.cpp', 'lyra/*.cpp']), + header_namespace = '', + compiler_flags = [ + '-fno-omit-frame-pointer', + '-fexceptions', + '-Wall', + '-Werror', + '-std=c++11', + '-DDISABLE_CPUCAP', + '-DDISABLE_XPLAT', + ], + exported_headers = subdir_glob([ + ('include', 'fb/**/*.h'), + ('include', 'jni/*.h'), + ]), + deps = [ + JNI_TARGET, + ], + visibility = ['PUBLIC'], + ) diff --git a/ReactAndroid/src/test/java/com/facebook/react/BUCK b/ReactAndroid/src/test/java/com/facebook/react/BUCK index 7ce47967f..4a76ac6c5 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/BUCK @@ -1,6 +1,6 @@ include_defs('//ReactAndroid/DEFS') -robolectric3_test( +rn_robolectric_test( name = 'react', # Please change the contact to the oncall of your team contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'], diff --git a/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK b/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK index 082b05bb4..4101f213b 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/animated/BUCK @@ -1,6 +1,6 @@ include_defs('//ReactAndroid/DEFS') -robolectric3_test( +rn_robolectric_test( name = 'animated', # Please change the contact to the oncall of your team contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'], diff --git a/ReactAndroid/src/test/java/com/facebook/react/bridge/BUCK b/ReactAndroid/src/test/java/com/facebook/react/bridge/BUCK index 9bfa0288c..882f64635 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/bridge/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/bridge/BUCK @@ -19,7 +19,7 @@ android_library( ], ) -robolectric3_test( +rn_robolectric_test( name = 'bridge', # Please change the contact to the oncall of your team contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'], diff --git a/ReactAndroid/src/test/java/com/facebook/react/devsupport/BUCK b/ReactAndroid/src/test/java/com/facebook/react/devsupport/BUCK index a01530d22..048e29f3c 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/devsupport/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/devsupport/BUCK @@ -1,6 +1,6 @@ include_defs('//ReactAndroid/DEFS') -robolectric3_test( +rn_robolectric_test( name = 'devsupport', # Please change the contact to the oncall of your team contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'], diff --git a/ReactAndroid/src/test/java/com/facebook/react/modules/BUCK b/ReactAndroid/src/test/java/com/facebook/react/modules/BUCK index 4ffc02a27..d5dfa2418 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/modules/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/modules/BUCK @@ -1,6 +1,6 @@ include_defs('//ReactAndroid/DEFS') -robolectric3_test( +rn_robolectric_test( # Please change the contact to the oncall of your team contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'], name = 'modules', diff --git a/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK b/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK index d5a5c27bd..3b994d6fb 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/uimanager/BUCK @@ -1,6 +1,6 @@ include_defs('//ReactAndroid/DEFS') -robolectric3_test( +rn_robolectric_test( name = 'uimanager', # Please change the contact to the oncall of your team contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'], diff --git a/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSetterTest.java b/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSetterTest.java index 59710b7ef..169a001db 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSetterTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/uimanager/ReactPropForShadowNodeSetterTest.java @@ -11,6 +11,9 @@ package com.facebook.react.uimanager; import javax.annotation.Nullable; +import com.facebook.react.bridge.ReadableArray; +import com.facebook.react.bridge.ReadableMap; +import com.facebook.react.bridge.JavaOnlyMap; import com.facebook.react.uimanager.annotations.ReactProp; import com.facebook.react.uimanager.annotations.ReactPropGroup; @@ -22,8 +25,6 @@ import org.powermock.core.classloader.annotations.PowerMockIgnore; import org.powermock.modules.junit4.rule.PowerMockRule; import org.robolectric.RobolectricTestRunner; -import static com.facebook.react.uimanager.ReactPropAnnotationSetterTest.ViewManagerUpdatesReceiver; -import static com.facebook.react.uimanager.ReactPropAnnotationSetterTest.buildStyles; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; @@ -41,6 +42,25 @@ public class ReactPropForShadowNodeSetterTest { @Rule public PowerMockRule rule = new PowerMockRule(); + public interface ViewManagerUpdatesReceiver { + void onBooleanSetterCalled(boolean value); + void onIntSetterCalled(int value); + void onDoubleSetterCalled(double value); + void onFloatSetterCalled(float value); + void onStringSetterCalled(String value); + void onBoxedBooleanSetterCalled(Boolean value); + void onBoxedIntSetterCalled(Integer value); + void onArraySetterCalled(ReadableArray value); + void onMapSetterCalled(ReadableMap value); + void onFloatGroupPropSetterCalled(int index, float value); + void onIntGroupPropSetterCalled(int index, int value); + void onBoxedIntGroupPropSetterCalled(int index, Integer value); + } + + public static ReactStylesDiffMap buildStyles(Object... keysAndValues) { + return new ReactStylesDiffMap(JavaOnlyMap.of(keysAndValues)); + } + private class ShadowViewUnderTest extends ReactShadowNode { private ViewManagerUpdatesReceiver mViewManagerUpdatesReceiver; @@ -65,8 +85,8 @@ public class ReactPropForShadowNodeSetterTest { } @ReactPropGroup(names = { - "floatGroupPropFirst", - "floatGroupPropSecond", + "floatGroupPropFirst", + "floatGroupPropSecond", }) public void setFloatGroupProp(int index, float value) { mViewManagerUpdatesReceiver.onFloatGroupPropSetterCalled(index, value); diff --git a/ReactAndroid/src/test/java/com/facebook/react/views/BUCK b/ReactAndroid/src/test/java/com/facebook/react/views/BUCK index 9f4f1ca2c..021c3537d 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/views/BUCK +++ b/ReactAndroid/src/test/java/com/facebook/react/views/BUCK @@ -1,6 +1,6 @@ include_defs('//ReactAndroid/DEFS') -robolectric3_test( +rn_robolectric_test( name = 'views', # Please change the contact to the oncall of your team contacts = ['oncall+fbandroid_sheriff@xmail.facebook.com'], diff --git a/ReactCommon/CSSLayout/BUCK b/ReactCommon/CSSLayout/BUCK new file mode 100644 index 000000000..d03bab136 --- /dev/null +++ b/ReactCommon/CSSLayout/BUCK @@ -0,0 +1,18 @@ +cxx_library( + name = 'CSSLayout', + force_static = True, + srcs = glob(['CSSLayout/*.c']), + header_namespace = '', + compiler_flags = [ + '-fno-omit-frame-pointer', + '-fexceptions', + '-Wall', + '-Werror', + '-std=c99', + '-O3', + ], + exported_headers = glob(['CSSLayout/*.h']), + deps = [ + ], + visibility = ['PUBLIC'], +)