[LayoutNG] Allow reusing NGLayoutResults which clearance applies.

This allows us to reuse layout results when affected by clearance.

We can't do this when:
 1. The "old" clearance offset isn't equal to the layout results offset.
 2. The difference between the constraint's BFC-block-offset, and the
   clearance offset shrinks (e.g. a margin may affect the final
   position).

I'm not sure if (1) actually should be a DCHECK.

Bug: 635619
Change-Id: Ic92063882914d4da7a4b168783ec2f2af82ec1fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1574836
Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653583}

third_party/blink/renderer/core/layout/layout_box.cc

@@ -2384,19 +2384,42 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
   bool is_exclusion_space_equal =
       new_space.ExclusionSpace() == old_space.ExclusionSpace();
 
+  LayoutUnit old_clearance_offset = old_space.ClearanceOffset();
+  LayoutUnit new_clearance_offset = new_space.ClearanceOffset();
+
   // If anything changes within the layout regarding floats, we need to perform
   // a series of additional checks to see if the result can still be reused.
   if (!is_bfc_offset_equal || !is_exclusion_space_equal ||
-      new_space.ClearanceOffset() != old_space.ClearanceOffset()) {
-    // We can't reuse a result if it was previously affected by clearance.
-    //
-    // TODO(layout-dev): There may be cases where we can re-use results here.
-    // However as there are complexities regarding margin-collapsing and
-    // clearance we currently don't attempt to cache anything.
-    if (cached_layout_result->IsPushedByFloats()) {
+      new_clearance_offset != old_clearance_offset) {
+    // Determine if we can reuse a result if it was affected by clearance.
+    bool is_pushed_by_floats = cached_layout_result->IsPushedByFloats();
+    if (is_pushed_by_floats) {
       DCHECK(old_space.HasFloats());
+
+      // We don't attempt to reuse the cached result if the clearance offset
+      // differs from the final BFC-block-offset.
+      //
+      // The |is_pushed_by_floats| flag is also used by nodes who have a
+      // *child* which was pushed by floats. In this case the node may not have
+      // a BFC-block-offset or one equal to the clearance offset.
+      if (!cached_layout_result->BfcBlockOffset() ||
+          *cached_layout_result->BfcBlockOffset() !=
+              old_space.ClearanceOffset())
+        return nullptr;
+
+      // We only reuse the cached result if the delta between the
+      // BFC-block-offset, and the clearance offset grows or remains the same.
+      // If it shrinks it may not be affected by clearance anymore as a margin
+      // may push the fragment below the clearance offset instead.
+      //
+      // TODO(layout-dev): If we track if any margins affected this calculation
+      // (with an additional bit on the layout result) we could potentially
+      // skip this check.
+      if (old_clearance_offset - old_space.BfcOffset().block_offset >
+          new_clearance_offset - new_space.BfcOffset().block_offset) {
         return nullptr;
       }
+    }
 
     // Check we have a descendant that *may* be positioned above the block-start
     // edge. We abort if either the old or new space has floats, as we don't
@@ -2413,9 +2436,14 @@ scoped_refptr<const NGLayoutResult> LayoutBox::CachedLayoutResult(
           old_space.ExclusionSpace().ClearanceOffset(EClear::kBoth))
         return nullptr;
 
+      if (is_pushed_by_floats) {
+        DCHECK_EQ(*bfc_block_offset, old_clearance_offset);
+        bfc_block_offset = new_clearance_offset;
+      } else {
         bfc_block_offset = *bfc_block_offset -
                            old_space.BfcOffset().block_offset +
                            new_space.BfcOffset().block_offset;
+      }
 
       // Check if the new position may intersect with any floats.
       if (*bfc_block_offset <

third_party/blink/renderer/core/layout/ng/ng_layout_result_caching_test.cc

@@ -326,7 +326,7 @@ TEST_F(NGLayoutResultCachingTest, MissFloatWillIntrude2) {
   EXPECT_EQ(result.get(), nullptr);
 }
 
-TEST_F(NGLayoutResultCachingTest, MissPushedByFloats1) {
+TEST_F(NGLayoutResultCachingTest, HitPushedByFloats1) {
   ScopedLayoutNGFragmentCachingForTest layout_ng_fragment_caching(true);
 
   // Same BFC offset, different exclusion space, pushed by floats.
@@ -343,7 +343,7 @@ TEST_F(NGLayoutResultCachingTest, MissPushedByFloats1) {
     </div>
     <div class="bfc">
       <div style="height: 50px;">
-        <div class="float" style="height: 40px;"></div>
+        <div class="float" style="height: 70px;"></div>
       </div>
       <div id="src" style="height: 20px; clear: left;"></div>
     </div>
@@ -357,10 +357,10 @@ TEST_F(NGLayoutResultCachingTest, MissPushedByFloats1) {
   scoped_refptr<const NGLayoutResult> result =
       test->CachedLayoutResult(space, nullptr);
 
-  EXPECT_EQ(result.get(), nullptr);
+  EXPECT_NE(result.get(), nullptr);
 }
 
-TEST_F(NGLayoutResultCachingTest, MissPushedByFloats2) {
+TEST_F(NGLayoutResultCachingTest, HitPushedByFloats2) {
   ScopedLayoutNGFragmentCachingForTest layout_ng_fragment_caching(true);
 
   // Different BFC offset, same exclusion space, pushed by floats.
@@ -386,6 +386,76 @@ TEST_F(NGLayoutResultCachingTest, MissPushedByFloats2) {
   auto* test = To<LayoutBlockFlow>(GetLayoutObjectByElementId("test"));
   auto* src = To<LayoutBlockFlow>(GetLayoutObjectByElementId("src"));
 
+  const NGConstraintSpace& space =
+      src->GetCachedLayoutResult()->GetConstraintSpaceForCaching();
+  scoped_refptr<const NGLayoutResult> result =
+      test->CachedLayoutResult(space, nullptr);
+
+  EXPECT_NE(result.get(), nullptr);
+}
+
+TEST_F(NGLayoutResultCachingTest, MissPushedByFloats1) {
+  ScopedLayoutNGFragmentCachingForTest layout_ng_fragment_caching(true);
+
+  // Same BFC offset, different exclusion space, pushed by floats.
+  // Miss due to shrinking offset.
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      .bfc { display: flow-root; width: 300px; height: 300px; }
+      .float { float: left; width: 50px; }
+    </style>
+    <div class="bfc">
+      <div style="height: 50px;">
+        <div class="float" style="height: 70px;"></div>
+      </div>
+      <div id="test" style="height: 20px; clear: left;"></div>
+    </div>
+    <div class="bfc">
+      <div style="height: 50px;">
+        <div class="float" style="height: 60px;"></div>
+      </div>
+      <div id="src" style="height: 20px; clear: left;"></div>
+    </div>
+  )HTML");
+
+  auto* test = To<LayoutBlockFlow>(GetLayoutObjectByElementId("test"));
+  auto* src = To<LayoutBlockFlow>(GetLayoutObjectByElementId("src"));
+
+  const NGConstraintSpace& space =
+      src->GetCachedLayoutResult()->GetConstraintSpaceForCaching();
+  scoped_refptr<const NGLayoutResult> result =
+      test->CachedLayoutResult(space, nullptr);
+
+  EXPECT_EQ(result.get(), nullptr);
+}
+
+TEST_F(NGLayoutResultCachingTest, MissPushedByFloats2) {
+  ScopedLayoutNGFragmentCachingForTest layout_ng_fragment_caching(true);
+
+  // Different BFC offset, same exclusion space, pushed by floats.
+  // Miss due to shrinking offset.
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      .bfc { display: flow-root; width: 300px; height: 300px; }
+      .float { float: left; width: 50px; }
+    </style>
+    <div class="bfc">
+      <div style="height: 30px;">
+        <div class="float" style="height: 60px;"></div>
+      </div>
+      <div id="test" style="height: 20px; clear: left;"></div>
+    </div>
+    <div class="bfc">
+      <div style="height: 50px;">
+        <div class="float" style="height: 60px;"></div>
+      </div>
+      <div id="src" style="height: 20px; clear: left;"></div>
+    </div>
+  )HTML");
+
+  auto* test = To<LayoutBlockFlow>(GetLayoutObjectByElementId("test"));
+  auto* src = To<LayoutBlockFlow>(GetLayoutObjectByElementId("src"));
+
   const NGConstraintSpace& space =
       src->GetCachedLayoutResult()->GetConstraintSpaceForCaching();
   scoped_refptr<const NGLayoutResult> result =