Use Layout Opportunity Iterator to position new FC blocks.

third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc

Index: third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
diff --git a/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc b/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
index f7f23b06477d3b8f328b98a8ae5743780c1f76dd..c6e7803b5c7d2cfcf8c48cdadc4c1e8049c16e39 100644
--- a/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
+++ b/third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
@@ -7,7 +7,6 @@
 #include "core/layout/ng/inline/ng_inline_node.h"
 #include "core/layout/ng/ng_absolute_utils.h"
 #include "core/layout/ng/ng_block_child_iterator.h"
-#include "core/layout/ng/ng_box_fragment.h"
 #include "core/layout/ng/ng_constraint_space.h"
 #include "core/layout/ng/ng_constraint_space_builder.h"
 #include "core/layout/ng/ng_floats_utils.h"
@@ -130,14 +129,75 @@ Optional<MinMaxContentSize> NGBlockLayoutAlgorithm::ComputeMinMaxContentSize()
 
 NGLogicalOffset NGBlockLayoutAlgorithm::CalculateLogicalOffset(
     const WTF::Optional<NGLogicalOffset>& known_fragment_offset) {
+  if (known_fragment_offset)
+    return known_fragment_offset.value() - ContainerBfcOffset();
   LayoutUnit inline_offset =
       border_and_padding_.inline_start + curr_child_margins_.inline_start;
-  LayoutUnit block_offset = content_size_;
-  if (known_fragment_offset) {
-    block_offset = known_fragment_offset.value().block_offset -
-                   ContainerBfcOffset().block_offset;
-  }
-  return {inline_offset, block_offset};
+  return {inline_offset, content_size_};
+}
+
+NGLogicalOffset NGBlockLayoutAlgorithm::PositionNewFc(

[ikilpatrick, 2017/04/17 08:17:24]

.nit, so i like having these types of methods close to where they are used,
thoughts on moving them below ::FinishChildLayout?

[ikilpatrick, 2017/04/17 08:17:24]

might be worth writing up here which case this covers, (and why it's gross).

[Gleb Lanbin, 2017/04/17 21:50:09]

Done.

[Gleb Lanbin, 2017/04/17 21:50:10]

Done.

+    const NGBoxFragment& fragment,
+    const NGConstraintSpace* child_space) {

[ikilpatrick, 2017/04/17 08:17:24]

you aren't modifying child_space right? so this should be a ref?

[Gleb Lanbin, 2017/04/17 21:50:10]

Done.

+  // 1. Position all pending floats to a temporary space.
+  RefPtr<NGConstraintSpace> tmp_space =
+      NGConstraintSpaceBuilder(child_space)
+          .SetIsNewFormattingContext(false)
+          .ToConstraintSpace(child_space->WritingMode());
+
+  NGFragmentBuilder tmp_builder(NGPhysicalFragment::kFragmentBox,
+                                /* node */ nullptr);
+  tmp_builder.SetBfcOffset(curr_bfc_offset_);
+  tmp_builder.MutableUnpositionedFloats().AppendVector(
+      container_builder_.UnpositionedFloats());
+  PositionPendingFloats(curr_bfc_offset_.block_offset, tmp_space.Get(),
+                        &tmp_builder);

[ikilpatrick, 2017/04/17 08:17:24]

is there an easy way we can avoid tmp_builder here?

[Gleb Lanbin, 2017/04/17 21:50:10]

Done.

+
+  // 2. Find an estimated layout opportunity for our fragment.
+  NGLayoutOpportunity opportunity = FindLayoutOpportunityForFragment(
+      tmp_space->Exclusions().get(), child_space->AvailableSize(),
+      curr_bfc_offset_, curr_child_margins_, fragment);
+
+  // 3. If the found opportunity lies on the same line with our estimated
+  //    child's BFC offset then merge fragment's margins with the current
+  //    MarginStrut.
+  if (opportunity.offset.block_offset == curr_bfc_offset_.block_offset)
+    curr_margin_strut_.Append(curr_child_margins_.block_start);
+  curr_bfc_offset_.block_offset += curr_margin_strut_.Sum();
+  curr_margin_strut_ = {};
+
+  // 4. Child's BFC block's offset is known here.

[ikilpatrick, 2017/04/17 08:17:24]

4. The child's BFC block offset is known here.

?

[Gleb Lanbin, 2017/04/17 21:50:10]

Done.

+  MaybeUpdateFragmentBfcOffset(ConstraintSpace(), curr_bfc_offset_,
+                               &container_builder_);
+  PositionPendingFloats(curr_bfc_offset_.block_offset, MutableConstraintSpace(),
+                        &container_builder_);
+
+  // 5. Find the final layout opportunity for the fragment after all pending
+  // floats are positioned at the correct BFC block's offset.
+  opportunity = FindLayoutOpportunityForFragment(
+      MutableConstraintSpace()->Exclusions().get(),
+      child_space->AvailableSize(), curr_bfc_offset_, curr_child_margins_,
+      fragment);
+
+  curr_bfc_offset_ = opportunity.offset;
+  return curr_bfc_offset_;
+}
+
+NGLogicalOffset NGBlockLayoutAlgorithm::PositionWithBfcOffset(
+    const NGBoxFragment& fragment) {
+  DCHECK(fragment.BfcOffset());
+  curr_bfc_offset_.block_offset = fragment.BfcOffset().value().block_offset;
+  MaybeUpdateFragmentBfcOffset(ConstraintSpace(), curr_bfc_offset_,
+                               &container_builder_);
+  PositionPendingFloats(curr_bfc_offset_.block_offset, MutableConstraintSpace(),
+                        &container_builder_);
+  return fragment.BfcOffset().value();
+}
+
+NGLogicalOffset NGBlockLayoutAlgorithm::PositionWithParentBfc() {
+  curr_bfc_offset_ +=
+      {curr_child_margins_.inline_start, curr_margin_strut_.Sum()};
+  return curr_bfc_offset_;
 }
 
 RefPtr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
@@ -221,13 +281,16 @@ RefPtr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
       }
     }
 
-    PrepareChildLayout(child);
+    NGLogicalOffset child_bfc_offset = PrepareChildLayout(child);
     RefPtr<NGConstraintSpace> child_space =
-        CreateConstraintSpaceForChild(child);
+        CreateConstraintSpaceForChild(child_bfc_offset, child);
     RefPtr<NGLayoutResult> layout_result =
         child->Layout(child_space.Get(), child_break_token);
 
-    FinishChildLayout(child, child_space.Get(), layout_result);
+    if (child->IsFloating())
+      FinishFloatChildLayout(child->Style(), *child_space, &*layout_result);
+    else
+      FinishChildLayout(&*child_space, &*layout_result);
 
     entry = child_iterator.NextChild();
     child = entry.node;
@@ -283,25 +346,22 @@ RefPtr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
   return container_builder_.ToBoxFragment();
 }
 
-void NGBlockLayoutAlgorithm::PrepareChildLayout(NGLayoutInputNode* child) {
+NGLogicalOffset NGBlockLayoutAlgorithm::PrepareChildLayout(
+    NGLayoutInputNode* child) {
   DCHECK(child);
 
-  // Calculate margins in parent's writing mode.
-  curr_child_margins_ = CalculateMargins(
-      child, *space_builder_.ToConstraintSpace(
-                 FromPlatformWritingMode(Style().GetWritingMode())));
-
-  // Set estimated BFC offset to the next child's constraint space.
   curr_bfc_offset_ = container_builder_.BfcOffset()
                          ? container_builder_.BfcOffset().value()
                          : ConstraintSpace().BfcOffset();
-  curr_bfc_offset_.block_offset += content_size_;
-  curr_bfc_offset_.inline_offset += border_and_padding_.inline_start;
+  curr_bfc_offset_ += {border_and_padding_.inline_start, content_size_};
 
-  bool is_floating = child->IsBlock() && child->Style().IsFloating();
+  // Calculate margins in parent's writing mode.
+  curr_child_margins_ = CalculateMargins(
+      child, *space_builder_.ToConstraintSpace(
+                 FromPlatformWritingMode(Style().GetWritingMode())));
 
   bool should_position_pending_floats =
-      child->IsBlock() && !is_floating &&
+      !child->IsFloating() &&
       !IsNewFormattingContextForBlockLevelChild(Style(), *child) &&
       ClearanceMayAffectLayout(ConstraintSpace(),
                                container_builder_.UnpositionedFloats(),
@@ -320,33 +380,21 @@ void NGBlockLayoutAlgorithm::PrepareChildLayout(NGLayoutInputNode* child) {
                           &container_builder_);
   }
 
-  bool is_inflow = child->IsInline() || !is_floating;
+  bool is_inflow = child->IsInline() || !child->IsFloating();
 
+  NGLogicalOffset child_bfc_offset = curr_bfc_offset_;
   // Only inflow children (e.g. not floats) are included in the child's margin
   // strut as they do not participate in margin collapsing.
   if (is_inflow) {
-    curr_bfc_offset_.inline_offset += curr_child_margins_.inline_start;
+    child_bfc_offset.inline_offset += curr_child_margins_.inline_start;
     // Append the current margin strut with child's block start margin.
-    // Non empty border/padding use cases are handled inside of the child's
-    // layout.
-    curr_margin_strut_.Append(curr_child_margins_.block_start);
+    // Non empty border/padding, new FC use cases are handled inside of the
+    // child's layout.
+    if (!IsNewFormattingContextForBlockLevelChild(Style(), *child))

[ikilpatrick, 2017/04/17 08:17:24]

you don't strictly need to do this right?

[Gleb Lanbin, 2017/04/17 21:50:09]

the decision about whether we should append margins for a new FC block happens
in PositionNewFc method.
  if (opportunity.offset.block_offset == curr_bfc_offset_.block_offset)
    curr_margin_strut_.Append(curr_child_margins_.block_start);

+      curr_margin_strut_.Append(curr_child_margins_.block_start);
   }
 
-  // TODO(ikilpatrick): Children which establish new formatting contexts need
-  // to be placed using the opportunity iterator before we can collapse margins.
-  // If the child is placed at the block_start of this fragment, then its
-  // margins do impact the position of its parent, if not (its placed below a
-  // float for example) it doesn't. \o/
-
-  bool should_collapse_margins =
-      child->IsInline() ||
-      (!is_floating &&
-       IsNewFormattingContextForBlockLevelChild(Style(), *child));
-
-  // Inline children or children which establish a block formatting context
-  // collapse margins and position themselves immediately as they need to know
-  // their BFC offset for fragmentation purposes.
-  if (should_collapse_margins) {
+  if (bool should_collapse_margins_if_inline = child->IsInline()) {
     curr_bfc_offset_.block_offset += curr_margin_strut_.Sum();
     MaybeUpdateFragmentBfcOffset(ConstraintSpace(), curr_bfc_offset_,
                                  &container_builder_);
@@ -354,68 +402,33 @@ void NGBlockLayoutAlgorithm::PrepareChildLayout(NGLayoutInputNode* child) {
                           MutableConstraintSpace(), &container_builder_);
     curr_margin_strut_ = {};
   }
+  child_bfc_offset.block_offset = curr_bfc_offset_.block_offset;
+  return child_bfc_offset;
 }
 
 void NGBlockLayoutAlgorithm::FinishChildLayout(
-    NGLayoutInputNode* child,
-    NGConstraintSpace* child_space,
-    RefPtr<NGLayoutResult> layout_result) {
-  NGBoxFragment fragment(
-      ConstraintSpace().WritingMode(),
-      ToNGPhysicalBoxFragment(layout_result->PhysicalFragment().Get()));
-
+    const NGConstraintSpace* child_space,
+    NGLayoutResult* layout_result) {
   // Pull out unpositioned floats to the current fragment. This may needed if
   // for example the child fragment could not position its floats because it's
   // empty and therefore couldn't determine its position in space.
   container_builder_.MutableUnpositionedFloats().AppendVector(
       layout_result->UnpositionedFloats());
 
-  if (child->IsBlock() && child->Style().IsFloating()) {
-    NGLogicalOffset origin_offset = constraint_space_->BfcOffset();
-    origin_offset.inline_offset += border_and_padding_.inline_start;
-    RefPtr<NGFloatingObject> floating_object = NGFloatingObject::Create(
-        child->Style(), child_space->WritingMode(),
-        child_space->AvailableSize(), origin_offset,
-        constraint_space_->BfcOffset(), curr_child_margins_,
-        layout_result->PhysicalFragment().Get());
-    container_builder_.AddUnpositionedFloat(floating_object);
-    // No need to postpone the positioning if we know the correct offset.
-    if (container_builder_.BfcOffset()) {
-      NGLogicalOffset origin_point = curr_bfc_offset_;
-      // Adjust origin point to the margins of the last child.
-      // Example: <div style="margin-bottom: 20px"><float></div>
-      //          <div style="margin-bottom: 30px"></div>
-      origin_point.block_offset += curr_margin_strut_.Sum();
-      PositionPendingFloats(origin_point.block_offset, MutableConstraintSpace(),
-                            &container_builder_);
-    }
-    return;
-  }
+  NGBoxFragment fragment(
+      ConstraintSpace().WritingMode(),
+      ToNGPhysicalBoxFragment(layout_result->PhysicalFragment().Get()));
 
-  // Determine the fragment's position in the parent space either by using
-  // content_size_ or known fragment's BFC offset.
-  WTF::Optional<NGLogicalOffset> bfc_offset;
-  if (child_space->IsNewFormattingContext()) {
-    bfc_offset = curr_bfc_offset_;
-  } else if (fragment.BfcOffset()) {
-    // Fragment that knows its offset can be used to set parent's BFC position.
-    curr_bfc_offset_.block_offset = fragment.BfcOffset().value().block_offset;
-    MaybeUpdateFragmentBfcOffset(ConstraintSpace(), curr_bfc_offset_,
-                                 &container_builder_);
-    PositionPendingFloats(curr_bfc_offset_.block_offset,
-                          MutableConstraintSpace(), &container_builder_);
-    bfc_offset = curr_bfc_offset_;
-  } else if (container_builder_.BfcOffset()) {
-    // Fragment doesn't know its offset but we can still calculate its BFC
-    // position because the parent fragment's BFC is known.
-    // Example:
-    // BFC Offset is known here because of the padding.
-    // <div style="padding: 1px">
-    //   <div id="empty-div" style="margins: 1px"></div>
-    bfc_offset = curr_bfc_offset_;
-    bfc_offset.value().block_offset += curr_margin_strut_.Sum();
-  }
-  NGLogicalOffset logical_offset = CalculateLogicalOffset(bfc_offset);
+  // Determine the fragment's position in the parent space.

[ikilpatrick, 2017/04/17 08:17:24]

Yay! thanks for pulling these out.

[Gleb Lanbin, 2017/04/17 21:50:10]

Acknowledged.

+  WTF::Optional<NGLogicalOffset> child_bfc_offset;
+  if (child_space->IsNewFormattingContext())
+    child_bfc_offset = PositionNewFc(fragment, child_space);
+  else if (fragment.BfcOffset())
+    child_bfc_offset = PositionWithBfcOffset(fragment);
+  else if (container_builder_.BfcOffset())
+    child_bfc_offset = PositionWithParentBfc();
+
+  NGLogicalOffset logical_offset = CalculateLogicalOffset(child_bfc_offset);
 
   // Update margin strut.
   curr_margin_strut_ = fragment.EndMarginStrut();
@@ -438,6 +451,29 @@ void NGBlockLayoutAlgorithm::FinishChildLayout(
   container_builder_.AddChild(layout_result, logical_offset);
 }
 
+void NGBlockLayoutAlgorithm::FinishFloatChildLayout(
+    const ComputedStyle& child_style,
+    const NGConstraintSpace& child_space,
+    const NGLayoutResult* layout_result) {
+  NGLogicalOffset origin_offset = constraint_space_->BfcOffset();
+  origin_offset.inline_offset += border_and_padding_.inline_start;
+  RefPtr<NGFloatingObject> floating_object = NGFloatingObject::Create(
+      child_style, child_space.WritingMode(), child_space.AvailableSize(),
+      origin_offset, constraint_space_->BfcOffset(), curr_child_margins_,
+      layout_result->PhysicalFragment().Get());
+  container_builder_.AddUnpositionedFloat(floating_object);
+  // No need to postpone the positioning if we know the correct offset.

[ikilpatrick, 2017/04/17 08:17:24]

can you add a newline above this line? (just having trouble with separation
here).

[Gleb Lanbin, 2017/04/17 21:50:10]

Done.

+  if (container_builder_.BfcOffset()) {
+    NGLogicalOffset origin_point = curr_bfc_offset_;
+    // Adjust origin point to the margins of the last child.
+    // Example: <div style="margin-bottom: 20px"><float></div>
+    //          <div style="margin-bottom: 30px"></div>
+    origin_point.block_offset += curr_margin_strut_.Sum();
+    PositionPendingFloats(origin_point.block_offset, MutableConstraintSpace(),
+                          &container_builder_);
+  }
+}
+
 void NGBlockLayoutAlgorithm::FinalizeForFragmentation() {
   LayoutUnit used_block_size =
       BreakToken() ? BreakToken()->UsedBlockSize() : LayoutUnit();
@@ -480,6 +516,8 @@ NGBoxStrut NGBlockLayoutAlgorithm::CalculateMargins(
     NGLayoutInputNode* child,
     const NGConstraintSpace& space) {
   DCHECK(child);
+  if (child->IsInline())
+    return {};
   const ComputedStyle& child_style = child->Style();
 
   WTF::Optional<MinMaxContentSize> sizes;
@@ -490,25 +528,25 @@ NGBoxStrut NGBlockLayoutAlgorithm::CalculateMargins(
       ComputeInlineSizeForFragment(space, child_style, sizes);
   NGBoxStrut margins = ComputeMargins(space, child_style, space.WritingMode(),
                                       space.Direction());
-  if (!child_style.IsFloating()) {
+  if (!child->IsFloating()) {
     ApplyAutoMargins(space, child_style, child_inline_size, &margins);
   }
   return margins;
 }
 
 RefPtr<NGConstraintSpace> NGBlockLayoutAlgorithm::CreateConstraintSpaceForChild(
+    const NGLogicalOffset& child_bfc_offset,
     NGLayoutInputNode* child) {
   DCHECK(child);
 
   const ComputedStyle& child_style = child->Style();
   bool is_new_bfc = IsNewFormattingContextForBlockLevelChild(Style(), *child);
   space_builder_.SetIsNewFormattingContext(is_new_bfc)
-      .SetBfcOffset(curr_bfc_offset_);
+      .SetBfcOffset(child_bfc_offset);
 
   if (child->IsInline()) {
     // TODO(kojii): Setup space_builder_ appropriately for inline child.
-    space_builder_.SetBfcOffset(curr_bfc_offset_)
-        .SetClearanceOffset(ConstraintSpace().ClearanceOffset());
+    space_builder_.SetClearanceOffset(ConstraintSpace().ClearanceOffset());
     return space_builder_.ToConstraintSpace(
         FromPlatformWritingMode(Style().GetWritingMode()));
   }
@@ -522,8 +560,8 @@ RefPtr<NGConstraintSpace> NGBlockLayoutAlgorithm::CreateConstraintSpaceForChild(
   // Float's margins are not included in child's space because:
   // 1) Floats do not participate in margins collapsing.
   // 2) Floats margins are used separately to calculate floating exclusions.
-  space_builder_.SetMarginStrut(child_style.IsFloating() ? NGMarginStrut()
-                                                         : curr_margin_strut_);
+  space_builder_.SetMarginStrut(child->IsFloating() ? NGMarginStrut()
+                                                    : curr_margin_strut_);
 
   LayoutUnit space_available;
   if (constraint_space_->HasBlockFragmentation()) {
@@ -532,7 +570,7 @@ RefPtr<NGConstraintSpace> NGBlockLayoutAlgorithm::CreateConstraintSpaceForChild(
     // position in the formatting context, and are able to adjust the
     // fragmentation line.
     if (is_new_bfc) {
-      space_available -= curr_bfc_offset_.block_offset;
+      space_available -= child_bfc_offset.block_offset;
     }
   }
   space_builder_.SetFragmentainerSpaceAvailable(space_available);