Dont measure single flex grow+shrink child

Summary: If there is a single child which is flex grow and flex shrink then instead of measuring and then shrinking we can just set the flex basis to zero as we know the final result will be that the child take up all remaining space. This is a re-land of D4147298. I have updated the diff to check explicitly for exact measure mode to also handle at_most case correctly. Reviewed By: gkassabli Differential Revision: D4153133 fbshipit-source-id: 2333150a83857cc30078cc8d52761cbd00652830
2016-11-09 11:23:21 -08:00
parent 863378d74e
commit a253c6fbb7
8 changed files with 260 additions and 38 deletions
--- a/CSSLayout/CSSLayout.c
+++ b/CSSLayout/CSSLayout.c
@@ -286,7 +286,6 @@ static void _CSSNodeMarkDirty(const CSSNodeRef node) {
 }

 void CSSNodeSetMeasureFunc(const CSSNodeRef node, CSSMeasureFunc measureFunc) {
-  // You can always NULLify the measure function of a node.
  if (measureFunc == NULL) {
    node->measure = NULL;
  } else {
@@ -301,7 +300,8 @@ CSSMeasureFunc CSSNodeGetMeasureFunc(const CSSNodeRef node) {

 void CSSNodeInsertChild(const CSSNodeRef node, const CSSNodeRef child, const uint32_t index) {
  CSS_ASSERT(child->parent == NULL, "Child already has a parent, it must be removed first.");
-  CSS_ASSERT(node->measure == NULL, "Cannot add child: Nodes with measure functions cannot have children.");
+  CSS_ASSERT(node->measure == NULL,
+             "Cannot add child: Nodes with measure functions cannot have children.");
  CSSNodeListInsert(&node->children, child, index);
  child->parent = node;
  _CSSNodeMarkDirty(node);
@@ -1399,6 +1399,26 @@ static void layoutNodeImpl(const CSSNodeRef node,
  const float availableInnerMainDim = isMainAxisRow ? availableInnerWidth : availableInnerHeight;
  const float availableInnerCrossDim = isMainAxisRow ? availableInnerHeight : availableInnerWidth;

+  // If there is only one child with flexGrow + flexShrink it means we can set the
+  // computedFlexBasis to 0 instead of measuring and shrinking / flexing the child to exactly
+  // match the remaining space
+  CSSNodeRef singleFlexChild = NULL;
+  if ((isMainAxisRow && widthMeasureMode == CSSMeasureModeExactly) ||
+      (!isMainAxisRow && heightMeasureMode == CSSMeasureModeExactly)) {
+    for (uint32_t i = 0; i < childCount; i++) {
+      const CSSNodeRef child = CSSNodeGetChild(node, i);
+      if (singleFlexChild) {
+        if (isFlex(child)) {
+          // There is already a flexible child, abort.
+          singleFlexChild = NULL;
+          break;
+        }
+      } else if (CSSNodeStyleGetFlexGrow(child) > 0 && CSSNodeStyleGetFlexShrink(child) > 0) {
+        singleFlexChild = child;
+      }
+    }
+  }
+
  // STEP 3: DETERMINE FLEX BASIS FOR EACH ITEM
  for (uint32_t i = 0; i < childCount; i++) {
    const CSSNodeRef child = CSSNodeListGet(node->children, i);
@@ -1423,13 +1443,17 @@ static void layoutNodeImpl(const CSSNodeRef node,
      currentAbsoluteChild = child;
      child->nextChild = NULL;
    } else {
-      computeChildFlexBasis(node,
-                            child,
-                            availableInnerWidth,
-                            widthMeasureMode,
-                            availableInnerHeight,
-                            heightMeasureMode,
-                            direction);
+      if (child == singleFlexChild) {
+        child->layout.computedFlexBasis = 0;
+      } else {
+        computeChildFlexBasis(node,
+                              child,
+                              availableInnerWidth,
+                              widthMeasureMode,
+                              availableInnerHeight,
+                              heightMeasureMode,
+                              direction);
+      }
    }
  }

@@ -2165,17 +2189,17 @@ static inline bool newMeasureSizeIsStricterAndStillValid(CSSMeasureMode sizeMode
 }

 bool CSSNodeCanUseCachedMeasurement(const CSSMeasureMode widthMode,
-                                           const float width,
-                                           const CSSMeasureMode heightMode,
-                                           const float height,
-                                           const CSSMeasureMode lastWidthMode,
-                                           const float lastWidth,
-                                           const CSSMeasureMode lastHeightMode,
-                                           const float lastHeight,
-                                           const float lastComputedWidth,
-                                           const float lastComputedHeight,
-                                           const float marginRow,
-                                           const float marginColumn) {
+                                    const float width,
+                                    const CSSMeasureMode heightMode,
+                                    const float height,
+                                    const CSSMeasureMode lastWidthMode,
+                                    const float lastWidth,
+                                    const CSSMeasureMode lastHeightMode,
+                                    const float lastHeight,
+                                    const float lastComputedWidth,
+                                    const float lastComputedHeight,
+                                    const float marginRow,
+                                    const float marginColumn) {
  if (lastComputedHeight < 0 || lastComputedWidth < 0) {
    return false;
  }
@@ -2193,19 +2217,16 @@ bool CSSNodeCanUseCachedMeasurement(const CSSMeasureMode widthMode,
      newMeasureSizeIsStricterAndStillValid(
          widthMode, width - marginRow, lastWidthMode, lastWidth, lastComputedWidth);

-  const bool heightIsCompatible = hasSameHeightSpec ||
-                                  newSizeIsExactAndMatchesOldMeasuredSize(heightMode,
-                                                                          height - marginColumn,
-                                                                          lastComputedHeight) ||
-                                  oldSizeIsUnspecifiedAndStillFits(heightMode,
+  const bool heightIsCompatible =
+      hasSameHeightSpec || newSizeIsExactAndMatchesOldMeasuredSize(heightMode,
                                                                   height - marginColumn,
-                                                                   lastHeightMode,
                                                                   lastComputedHeight) ||
-                                  newMeasureSizeIsStricterAndStillValid(heightMode,
-                                                                        height - marginColumn,
-                                                                        lastHeightMode,
-                                                                        lastHeight,
-                                                                        lastComputedHeight);
+      oldSizeIsUnspecifiedAndStillFits(heightMode,
+                                       height - marginColumn,
+                                       lastHeightMode,
+                                       lastComputedHeight) ||
+      newMeasureSizeIsStricterAndStillValid(
+          heightMode, height - marginColumn, lastHeightMode, lastHeight, lastComputedHeight);

  return widthIsCompatible && heightIsCompatible;
 }
--- a/csharp/tests/Facebook.CSSLayout/CSSLayoutFlexTest.cs
+++ b/csharp/tests/Facebook.CSSLayout/CSSLayoutFlexTest.cs
@@ -41,6 +41,12 @@
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
 </div>
+
+<div id="flex_grow_shrink_at_most" style="height: 100px; width: 100px;">
+  <div>
+    <div style="flex-grow:1; flex-shrink:1;"></div>
+  </div>
+</div>
 *
 */

@@ -399,5 +405,56 @@ namespace Facebook.CSSLayout
            Assert.AreEqual(20, root_child2.LayoutHeight);
        }

+        [Test]
+        public void Test_flex_grow_shrink_at_most()
+        {
+            CSSNode root = new CSSNode();
+            root.StyleWidth = 100;
+            root.StyleHeight = 100;
+
+            CSSNode root_child0 = new CSSNode();
+            root.Insert(0, root_child0);
+
+            CSSNode root_child0_child0 = new CSSNode();
+            root_child0_child0.FlexGrow = 1;
+            root_child0_child0.FlexShrink = 1;
+            root_child0.Insert(0, root_child0_child0);
+            root.StyleDirection = CSSDirection.LeftToRight;
+            root.CalculateLayout();
+
+            Assert.AreEqual(0, root.LayoutX);
+            Assert.AreEqual(0, root.LayoutY);
+            Assert.AreEqual(100, root.LayoutWidth);
+            Assert.AreEqual(100, root.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0.LayoutX);
+            Assert.AreEqual(0, root_child0.LayoutY);
+            Assert.AreEqual(100, root_child0.LayoutWidth);
+            Assert.AreEqual(0, root_child0.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0_child0.LayoutX);
+            Assert.AreEqual(0, root_child0_child0.LayoutY);
+            Assert.AreEqual(100, root_child0_child0.LayoutWidth);
+            Assert.AreEqual(0, root_child0_child0.LayoutHeight);
+
+            root.StyleDirection = CSSDirection.RightToLeft;
+            root.CalculateLayout();
+
+            Assert.AreEqual(0, root.LayoutX);
+            Assert.AreEqual(0, root.LayoutY);
+            Assert.AreEqual(100, root.LayoutWidth);
+            Assert.AreEqual(100, root.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0.LayoutX);
+            Assert.AreEqual(0, root_child0.LayoutY);
+            Assert.AreEqual(100, root_child0.LayoutWidth);
+            Assert.AreEqual(0, root_child0.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0_child0.LayoutX);
+            Assert.AreEqual(0, root_child0_child0.LayoutY);
+            Assert.AreEqual(100, root_child0_child0.LayoutWidth);
+            Assert.AreEqual(0, root_child0_child0.LayoutHeight);
+        }
+
    }
 }
--- a/gentest/fixtures/CSSLayoutFlexTest.html
+++ b/gentest/fixtures/CSSLayoutFlexTest.html
@@ -29,3 +29,9 @@
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
 </div>
+
+<div id="flex_grow_shrink_at_most" style="height: 100px; width: 100px;">
+  <div>
+    <div style="flex-grow:1; flex-shrink:1;"></div>
+  </div>
+</div>
--- a/java/jni/CSSJNI.cpp
+++ b/java/jni/CSSJNI.cpp
@@ -40,7 +40,8 @@ static void _jniTransferLayoutOutputsRecursive(CSSNodeRef root) {
 }

 static void _jniPrint(CSSNodeRef node) {
-  auto obj = adopt_local(Environment::current()->NewLocalRef(reinterpret_cast<jweak>(CSSNodeGetContext(node))));
+  auto obj = adopt_local(
+      Environment::current()->NewLocalRef(reinterpret_cast<jweak>(CSSNodeGetContext(node))));
  cout << obj->toString() << endl;
 }

@@ -49,10 +50,11 @@ static CSSSize _jniMeasureFunc(CSSNodeRef node,
                               CSSMeasureMode widthMode,
                               float height,
                               CSSMeasureMode heightMode) {
-  auto obj = adopt_local(Environment::current()->NewLocalRef(reinterpret_cast<jweak>(CSSNodeGetContext(node))));
+  auto obj = adopt_local(
+      Environment::current()->NewLocalRef(reinterpret_cast<jweak>(CSSNodeGetContext(node))));

  static auto measureFunc = findClassLocal("com/facebook/csslayout/CSSNode")
-      ->getMethod<jlong(jfloat, jint, jfloat, jint)>("measure");
+                                ->getMethod<jlong(jfloat, jint, jfloat, jint)>("measure");

  _jniTransferLayoutDirection(node, obj);
  const auto measureResult = measureFunc(obj, width, widthMode, height, heightMode);
--- a/java/tests/com/facebook/csslayout/CSSLayoutFlexTest.java
+++ b/java/tests/com/facebook/csslayout/CSSLayoutFlexTest.java
@@ -41,6 +41,12 @@
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
 </div>
+
+<div id="flex_grow_shrink_at_most" style="height: 100px; width: 100px;">
+  <div>
+    <div style="flex-grow:1; flex-shrink:1;"></div>
+  </div>
+</div>
 *
 */

@@ -392,4 +398,54 @@ public class CSSLayoutFlexTest {
    assertEquals(20, root_child2.getLayoutHeight(), 0.0f);
  }

+  @Test
+  public void test_flex_grow_shrink_at_most() {
+    final CSSNode root = new CSSNode();
+    root.setStyleWidth(100);
+    root.setStyleHeight(100);
+
+    final CSSNode root_child0 = new CSSNode();
+    root.addChildAt(root_child0, 0);
+
+    final CSSNode root_child0_child0 = new CSSNode();
+    root_child0_child0.setFlexGrow(1);
+    root_child0_child0.setFlexShrink(1);
+    root_child0.addChildAt(root_child0_child0, 0);
+    root.setDirection(CSSDirection.LTR);
+    root.calculateLayout(null);
+
+    assertEquals(0, root.getLayoutX(), 0.0f);
+    assertEquals(0, root.getLayoutY(), 0.0f);
+    assertEquals(100, root.getLayoutWidth(), 0.0f);
+    assertEquals(100, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0.getLayoutY(), 0.0f);
+    assertEquals(100, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(0, root_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0_child0.getLayoutY(), 0.0f);
+    assertEquals(100, root_child0_child0.getLayoutWidth(), 0.0f);
+    assertEquals(0, root_child0_child0.getLayoutHeight(), 0.0f);
+
+    root.setDirection(CSSDirection.RTL);
+    root.calculateLayout(null);
+
+    assertEquals(0, root.getLayoutX(), 0.0f);
+    assertEquals(0, root.getLayoutY(), 0.0f);
+    assertEquals(100, root.getLayoutWidth(), 0.0f);
+    assertEquals(100, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0.getLayoutY(), 0.0f);
+    assertEquals(100, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(0, root_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0_child0.getLayoutY(), 0.0f);
+    assertEquals(100, root_child0_child0.getLayoutWidth(), 0.0f);
+    assertEquals(0, root_child0_child0.getLayoutHeight(), 0.0f);
+  }
+
 }
--- a/tests/CSSLayoutFlexTest.cpp
+++ b/tests/CSSLayoutFlexTest.cpp
@@ -41,6 +41,12 @@
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
 </div>
+
+<div id="flex_grow_shrink_at_most" style="height: 100px; width: 100px;">
+  <div>
+    <div style="flex-grow:1; flex-shrink:1;"></div>
+  </div>
+</div>
 *
 */

@@ -381,3 +387,52 @@ TEST(CSSLayoutTest, flex_basis_overrides_main_size) {

  CSSNodeFreeRecursive(root);
 }
+
+TEST(CSSLayoutTest, flex_grow_shrink_at_most) {
+  const CSSNodeRef root = CSSNodeNew();
+  CSSNodeStyleSetWidth(root, 100);
+  CSSNodeStyleSetHeight(root, 100);
+
+  const CSSNodeRef root_child0 = CSSNodeNew();
+  CSSNodeInsertChild(root, root_child0, 0);
+
+  const CSSNodeRef root_child0_child0 = CSSNodeNew();
+  CSSNodeStyleSetFlexGrow(root_child0_child0, 1);
+  CSSNodeStyleSetFlexShrink(root_child0_child0, 1);
+  CSSNodeInsertChild(root_child0, root_child0_child0, 0);
+  CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR);
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root));
+  ASSERT_EQ(100, CSSNodeLayoutGetWidth(root));
+  ASSERT_EQ(100, CSSNodeLayoutGetHeight(root));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0));
+  ASSERT_EQ(100, CSSNodeLayoutGetWidth(root_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetHeight(root_child0));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0_child0));
+  ASSERT_EQ(100, CSSNodeLayoutGetWidth(root_child0_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetHeight(root_child0_child0));
+
+  CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionRTL);
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root));
+  ASSERT_EQ(100, CSSNodeLayoutGetWidth(root));
+  ASSERT_EQ(100, CSSNodeLayoutGetHeight(root));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0));
+  ASSERT_EQ(100, CSSNodeLayoutGetWidth(root_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetHeight(root_child0));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0_child0));
+  ASSERT_EQ(100, CSSNodeLayoutGetWidth(root_child0_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetHeight(root_child0_child0));
+
+  CSSNodeFreeRecursive(root);
+}
--- a/tests/CSSLayoutMeasureCacheTest.cpp
+++ b/tests/CSSLayoutMeasureCacheTest.cpp
@@ -17,7 +17,8 @@ static CSSSize _measureMax(CSSNodeRef node,
                        CSSMeasureMode heightMode) {

  int *measureCount = (int *)CSSNodeGetContext(node);
-  *measureCount = *measureCount + 1;
+  (*measureCount)++;
+
  return CSSSize {
      .width = widthMode == CSSMeasureModeUndefined ? 10 : width,
      .height = heightMode == CSSMeasureModeUndefined ? 10 : height,
--- a/tests/CSSLayoutMeasureTest.cpp
+++ b/tests/CSSLayoutMeasureTest.cpp
@@ -10,18 +10,42 @@
 #include <CSSLayout/CSSLayout.h>
 #include <gtest/gtest.h>

-#if GTEST_HAS_DEATH_TEST
 static CSSSize _measure(CSSNodeRef node,
                        float width,
                        CSSMeasureMode widthMode,
                        float height,
                        CSSMeasureMode heightMode) {
+  int *measureCount = (int*) CSSNodeGetContext(node);
+  if (measureCount) {
+    (*measureCount)++;
+  }
+
  return CSSSize {
-      .width = 0,
-      .height = 0,
+      .width = 10,
+      .height = 10,
  };
 }

+TEST(CSSLayoutTest, dont_measure_single_grow_shrink_child) {
+  const CSSNodeRef root = CSSNodeNew();
+  CSSNodeStyleSetWidth(root, 100);
+  CSSNodeStyleSetHeight(root, 100);
+
+  int measureCount = 0;
+
+  const CSSNodeRef root_child0 = CSSNodeNew();
+  CSSNodeSetContext(root_child0, &measureCount);
+  CSSNodeSetMeasureFunc(root_child0, _measure);
+  CSSNodeStyleSetFlexGrow(root_child0, 1);
+  CSSNodeStyleSetFlexShrink(root_child0, 1);
+  CSSNodeInsertChild(root, root_child0, 0);
+
+  CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR);
+
+  ASSERT_EQ(0, measureCount);
+}
+
+#if GTEST_HAS_DEATH_TEST
 TEST(CSSLayoutTest, cannot_add_child_to_node_with_measure_func) {
  const CSSNodeRef root = CSSNodeNew();
  CSSNodeSetMeasureFunc(root, _measure);