[LayoutNG] Move ng_block_layout_algorithm to use constraint space builder.

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 56607765eb2d5714f8ea19d16672713626840656..17bb79dc354d04ac90a376ebd27b648cde476177 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
@@ -5,6 +5,7 @@
 #include "core/layout/ng/ng_block_layout_algorithm.h"
 
 #include "core/layout/ng/ng_constraint_space.h"
+#include "core/layout/ng/ng_constraint_space_builder.h"
 #include "core/layout/ng/ng_fragment_builder.h"
 #include "core/layout/ng/ng_fragment.h"
 #include "core/layout/ng/ng_layout_opportunity_iterator.h"
@@ -58,7 +59,7 @@ NGExclusion* CreateExclusion(const NGFragment& fragment,
 // @param margins Margins of the fragment.
 // @return Layout opportunity for the fragment.
 const NGLayoutOpportunity FindLayoutOpportunityForFragment(
-    const Member<NGConstraintSpace>& space,
+    NGConstraintSpace* space,
     const NGFragment& fragment,
     const NGBoxStrut& margins) {
   NGLayoutOpportunityIterator* opportunity_iter = space->LayoutOpportunities();
@@ -143,56 +144,67 @@ bool IsNewFormattingContextForInFlowBlockLevelChild(
 
 NGBlockLayoutAlgorithm::NGBlockLayoutAlgorithm(
     PassRefPtr<const ComputedStyle> style,
-    NGBox* first_child)
-    : style_(style),
+    NGBox* first_child,
+    NGConstraintSpace* constraint_space)
+    : state_(kStateInit),
+      style_(style),
       first_child_(first_child),
-      state_(kStateInit),
+      constraint_space_(constraint_space),
       is_fragment_margin_strut_block_start_updated_(false) {
   DCHECK(style_);
 }
 
-bool NGBlockLayoutAlgorithm::Layout(const NGConstraintSpace* constraint_space,
-                                    NGPhysicalFragment** out) {
+bool NGBlockLayoutAlgorithm::Layout(NGPhysicalFragment** out) {
   switch (state_) {
     case kStateInit: {
       border_and_padding_ =
-          ComputeBorders(Style()) + ComputePadding(*constraint_space, Style());
+          ComputeBorders(Style()) + ComputePadding(*constraint_space_, Style());
 
       LayoutUnit inline_size =
-          ComputeInlineSizeForFragment(*constraint_space, Style());
+          ComputeInlineSizeForFragment(*constraint_space_, Style());
       LayoutUnit adjusted_inline_size =
           inline_size - border_and_padding_.InlineSum();
       // TODO(layout-ng): For quirks mode, should we pass blockSize instead of
       // -1?
       LayoutUnit block_size = ComputeBlockSizeForFragment(
-          *constraint_space, Style(), NGSizeIndefinite);
+          *constraint_space_, Style(), NGSizeIndefinite);
       LayoutUnit adjusted_block_size(block_size);
       // Our calculated block-axis size may be indefinite at this point.
       // If so, just leave the size as NGSizeIndefinite instead of subtracting
       // borders and padding.
       if (adjusted_block_size != NGSizeIndefinite)
         adjusted_block_size -= border_and_padding_.BlockSum();
-      constraint_space_for_children_ = new NGConstraintSpace(
-          FromPlatformWritingMode(Style().getWritingMode()),
-          FromPlatformDirection(Style().direction()), *constraint_space,
+
+      space_builder_ =
+          new NGConstraintSpaceBuilder(constraint_space_->WritingMode());

[cbiesinger, 2016/10/27 21:47:19]

You're not setting the direction? The old code did -- doesn't that mean the
constraint space will have the wrong direction set?

[ikilpatrick, 2016/10/27 21:56:13]

This is only for the builder, which only needs to know about the direction at
the moment, (only deals with {inline,block}->{width,height}) conversion at the
moment).

[cbiesinger, 2016/10/28 18:58:56]

Hmm OK. It makes me uneasy to leave this out, even if we don't currently use it,
because that may be a nonobvious aspect of the builder-created constraint space
if we want to make use of it in the future.

+      space_builder_->SetContainerSize(
+          NGLogicalSize(adjusted_inline_size, adjusted_block_size));
+
+      constraint_space_->SetSize(
           NGLogicalSize(adjusted_inline_size, adjusted_block_size));
+
       content_size_ = border_and_padding_.block_start;
 
       builder_ = new NGFragmentBuilder(NGPhysicalFragmentBase::FragmentBox);
-      builder_->SetDirection(constraint_space->Direction());
-      builder_->SetWritingMode(constraint_space->WritingMode());
+      builder_->SetDirection(constraint_space_->Direction());
+      builder_->SetWritingMode(constraint_space_->WritingMode());
       builder_->SetInlineSize(inline_size).SetBlockSize(block_size);
       current_child_ = first_child_;
+      if (current_child_)
+        space_for_current_child_ = CreateConstraintSpaceForCurrentChild();
+
       state_ = kStateChildLayout;
       return false;
     }
     case kStateChildLayout: {
       if (current_child_) {
-        if (!LayoutCurrentChild(constraint_space))
+        if (!LayoutCurrentChild())
           return false;
         current_child_ = current_child_->NextSibling();
-        if (current_child_)
+        if (current_child_) {
+          space_for_current_child_ = CreateConstraintSpaceForCurrentChild();
           return false;
+        }
       }
       state_ = kStateFinalize;
       return false;
@@ -202,7 +214,7 @@ bool NGBlockLayoutAlgorithm::Layout(const NGConstraintSpace* constraint_space,
 
       // Recompute the block-axis size now that we know our content size.
       LayoutUnit block_size = ComputeBlockSizeForFragment(
-          *constraint_space, Style(), content_size_);
+          *constraint_space_, Style(), content_size_);
 
       builder_->SetBlockSize(block_size)
           .SetInlineOverflow(max_inline_size_)
@@ -217,37 +229,28 @@ bool NGBlockLayoutAlgorithm::Layout(const NGConstraintSpace* constraint_space,
   return true;
 }
 
-bool NGBlockLayoutAlgorithm::LayoutCurrentChild(
-    const NGConstraintSpace* constraint_space) {
-  constraint_space_for_children_->SetIsNewFormattingContext(
-      IsNewFormattingContextForInFlowBlockLevelChild(*constraint_space,
-                                                     *current_child_->Style()));
-
+bool NGBlockLayoutAlgorithm::LayoutCurrentChild() {
   NGFragment* fragment;
-  if (!current_child_->Layout(constraint_space_for_children_, &fragment))
+  if (!current_child_->Layout(space_for_current_child_, &fragment))
     return false;
 
-  NGBoxStrut child_margins =
-      ComputeMargins(*constraint_space_for_children_, *current_child_->Style(),
-                     constraint_space_for_children_->WritingMode(),
-                     constraint_space_for_children_->Direction());
+  NGBoxStrut child_margins = ComputeMargins(
+      *space_for_current_child_, *current_child_->Style(),
+      constraint_space_->WritingMode(), constraint_space_->Direction());
 
   NGLogicalOffset fragment_offset;
   if (current_child_->Style()->isFloating()) {
     fragment_offset = PositionFloatFragment(*fragment, child_margins);
   } else {
-    // TODO(layout-ng): move ApplyAutoMargins to PositionFragment

[cbiesinger, 2016/10/27 21:47:19]

Thanks

[Gleb Lanbin, 2016/10/27 22:30:06]

you need to run 'git rebase-update', it was deleted here
http://crrev.com/2453553002

[ikilpatrick, 2016/10/28 00:16:28]

And added here:
https://codereview.chromium.org/2449313002/diff/20001/third_party/WebKit/Sour...

-    ApplyAutoMargins(*constraint_space_for_children_, *current_child_->Style(),
+    ApplyAutoMargins(*space_for_current_child_, *current_child_->Style(),
                      *fragment, &child_margins);
-    fragment_offset =
-        PositionFragment(*fragment, child_margins, *constraint_space);
+    fragment_offset = PositionFragment(*fragment, child_margins);
   }
   builder_->AddChild(fragment, fragment_offset);
   return true;
 }
 
 NGBoxStrut NGBlockLayoutAlgorithm::CollapseMargins(
-    const NGConstraintSpace& space,
     const NGBoxStrut& margins,
     const NGFragment& fragment) {
   bool is_zero_height_box = !fragment.BlockSize() && margins.IsEmpty() &&
@@ -260,7 +263,7 @@ NGBoxStrut NGBlockLayoutAlgorithm::CollapseMargins(
   // Calculate borders and padding for the current child.
   NGBoxStrut border_and_padding =
       ComputeBorders(*current_child_->Style()) +
-      ComputePadding(space, *current_child_->Style());
+      ComputePadding(*constraint_space_, *current_child_->Style());
 
   // Collapse BLOCK-START margins if there is no padding or border between
   // parent (current child) and its first in-flow child.
@@ -287,7 +290,7 @@ NGBoxStrut NGBlockLayoutAlgorithm::CollapseMargins(
   // - Compute margins block start for adjoining blocks *including* 1st block.
   // - Compute margins block end for the last block.
   // - Do not set the computed margins to the parent fragment.
-  if (space.IsNewFormattingContext()) {
+  if (constraint_space_->IsNewFormattingContext()) {
     result_margins.block_start = ComputeCollapsedMarginBlockStart(
         prev_child_margin_strut_, curr_margin_strut);
     bool is_last_child = !current_child_->NextSibling();
@@ -315,10 +318,8 @@ NGBoxStrut NGBlockLayoutAlgorithm::CollapseMargins(
 
 NGLogicalOffset NGBlockLayoutAlgorithm::PositionFragment(
     const NGFragment& fragment,
-    const NGBoxStrut& child_margins,
-    const NGConstraintSpace& space) {
-  const NGBoxStrut collapsed_margins =
-      CollapseMargins(space, child_margins, fragment);
+    const NGBoxStrut& child_margins) {
+  const NGBoxStrut collapsed_margins = CollapseMargins(child_margins, fragment);
 
   LayoutUnit inline_offset =
       border_and_padding_.inline_start + child_margins.inline_start;
@@ -336,8 +337,8 @@ NGLogicalOffset NGBlockLayoutAlgorithm::PositionFloatFragment(
     const NGBoxStrut& margins) {
   // TODO(glebl@chromium.org): Support the top edge alignment rule.
   // Find a layout opportunity that will fit our float.
-  const NGLayoutOpportunity opportunity = FindLayoutOpportunityForFragment(
-      constraint_space_for_children_, fragment, margins);
+  const NGLayoutOpportunity opportunity =
+      FindLayoutOpportunityForFragment(constraint_space_, fragment, margins);

[cbiesinger, 2016/10/27 21:47:19]

I don't think that's correct? If you use constraint_space_ you are finding the
layout opportunity based on the wrong width, which could then be outside of our
box. Better use constraint_space_for_current_child_?

[ikilpatrick, 2016/10/27 21:56:13]

Yeah see constraint_space_->SetSize(...) above.

Basically my logic here is:
 - Current layout can modify it's "view" on it's on constraint space to return
correct layout opportunities etc.
 - constraint_space_for_current_child_ is wrong as once we are establishing
BFC's properly that constraint space won't have any exclusions in it (for the
float case).

We may want to explicitly store the content_size_ here and use that? For this
calculation?

We are moving that direction for OppIterator anyway, i.e. instead of just:
 - start_offset, leader_offset
have...
 - rect, leader_offset

Thoughts?

[cbiesinger, 2016/10/28 18:58:56]

Ah I see. So, the basic concept is that instead of creating a "derived"
constraint space and working on those we will just use constraint_space_ for
everything and modify it as necessary. OK -- that sounds good. I suppose we
didn't use to be able to do that because the constraint_space argument was
const. May be a little confusing that the size is correct but there's no correct
offset in the constraint space (and that the caller of FindLayoutOpportunity has
to keep track of that) but this seems fine for now, we can revisit that
separately if needed.

[ikilpatrick, 2016/10/28 19:43:49]

Yeah, more than happy to revisit this later; felt that this was "less wrong"
than finding opps in the child's constraint space :).

   DCHECK(!opportunity.IsEmpty()) << "Opportunity is empty but it shouldn't be";
 
   // Calculate the float offset if needed.
@@ -349,12 +350,23 @@ NGLogicalOffset NGBlockLayoutAlgorithm::PositionFloatFragment(
   // Add the float as an exclusion.
   const NGExclusion* exclusion =
       CreateExclusion(fragment, opportunity, float_offset, margins);
-  constraint_space_for_children_->AddExclusion(exclusion);
+  constraint_space_->AddExclusion(exclusion);
 
   return CalculateLogicalOffsetForOpportunity(opportunity, border_and_padding_,
                                               float_offset, margins);
 }
 
+NGConstraintSpace*
+NGBlockLayoutAlgorithm::CreateConstraintSpaceForCurrentChild() {

[Gleb Lanbin, 2016/10/27 22:30:06]

1) move this to the unnamed namespace?

2) may be use space_builder_ to generate constraint spaces for children ? 

[ikilpatrick, 2016/10/28 00:16:28]

done?

+  DCHECK(current_child_);
+  space_builder_->SetIsNewFormattingContext(
+      IsNewFormattingContextForInFlowBlockLevelChild(*constraint_space_,
+                                                     *current_child_->Style()));
+  return new NGConstraintSpace(constraint_space_->WritingMode(),
+                               constraint_space_->Direction(),
+                               space_builder_->ToConstraintSpace());
+}
+
 void NGBlockLayoutAlgorithm::UpdateMarginStrut(const NGMarginStrut& from) {
   if (!is_fragment_margin_strut_block_start_updated_) {
     builder_->SetMarginStrutBlockStart(from);
@@ -366,8 +378,10 @@ void NGBlockLayoutAlgorithm::UpdateMarginStrut(const NGMarginStrut& from) {
 DEFINE_TRACE(NGBlockLayoutAlgorithm) {
   NGLayoutAlgorithm::trace(visitor);
   visitor->trace(first_child_);
+  visitor->trace(constraint_space_);
   visitor->trace(builder_);
-  visitor->trace(constraint_space_for_children_);
+  visitor->trace(space_builder_);
+  visitor->trace(space_for_current_child_);
   visitor->trace(current_child_);
 }