[LayoutNG] Move ng_block_layout_algorithm to use constraint space builder.

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "core/layout/ng/ng_length_utils.h"
 #include "core/layout/ng/ng_units.h"
 #include "core/style/ComputedStyle.h"
 #include "platform/LengthFunctions.h"
 
 namespace blink {
 namespace {
@@ skipping 33 matching lines @@
 // It iterates over all layout opportunities in the constraint space and returns
 // the first layout opportunity that is wider than the fragment or returns the
 // last one which is always the widest.
 //
 // @param space Constraint space that is used to find layout opportunity for
 //              the fragment.
 // @param fragment Fragment that needs to be placed.
 // @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();
   NGLayoutOpportunity opportunity;
   NGLayoutOpportunity opportunity_candidate = opportunity_iter->Next();
 
   while (!opportunity_candidate.IsEmpty()) {
     opportunity = opportunity_candidate;
     // Checking opportunity's block size is not necessary as a float cannot be
     // positioned on top of another float inside of the same constraint space.
@@ skipping 64 matching lines @@
   if (space.WritingMode() != FromPlatformWritingMode(style.getWritingMode()))
     return true;
 
   return false;
 }
 
 }  // namespace
 
 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;
     }
     case kStateFinalize: {
       content_size_ += border_and_padding_.block_end;
 
       // 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_)
           .SetBlockOverflow(content_size_);
       *out = builder_->ToFragment();
       state_ = kStateInit;
       return true;
     }
   };
   NOTREACHED();
   *out = nullptr;
   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() &&
                             fragment.MarginStrut().IsEmpty();
   // Create the current child's margin strut from its children's margin strut or
   // use margin strut from the the last non-empty child.
   NGMarginStrut curr_margin_strut =
       is_zero_height_box ? prev_child_margin_strut_ : fragment.MarginStrut();
 
   // 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.
   if (border_and_padding.block_start) {
     curr_margin_strut.SetMarginBlockStart(margins.block_start);
   } else {
     curr_margin_strut.AppendMarginBlockStart(margins.block_start);
   }
 
   // Collapse BLOCK-END margins if
   // 1) there is no padding or border between parent (current child) and its
   //    first/last in-flow child
   // 2) parent's logical height is auto.
   if (current_child_->Style()->logicalHeight().isAuto() &&
       !border_and_padding.block_end) {
     curr_margin_strut.AppendMarginBlockEnd(margins.block_end);
   } else {
     curr_margin_strut.SetMarginBlockEnd(margins.block_end);
   }
 
   NGBoxStrut result_margins;
   // Margins of the newly established formatting context do not participate
   // in Collapsing Margins:
   // - 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();
     if (is_last_child)
       result_margins.block_end = curr_margin_strut.BlockEndSum();
     return result_margins;
   }
 
   // Zero-height boxes are ignored and do not participate in margin collapsing.
   if (is_zero_height_box)
     return result_margins;
 
   // Compute the margin block start for adjoining blocks *excluding* 1st block
   if (is_fragment_margin_strut_block_start_updated_) {
     result_margins.block_start = ComputeCollapsedMarginBlockStart(
         prev_child_margin_strut_, curr_margin_strut);
   }
 
   // Update the parent fragment's margin strut
   UpdateMarginStrut(curr_margin_strut);
 
   prev_child_margin_strut_ = curr_margin_strut;
   return result_margins;
 }
 
 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;
   LayoutUnit block_offset = content_size_ + collapsed_margins.block_start;
 
   content_size_ += fragment.BlockSize() + collapsed_margins.BlockSum();
   max_inline_size_ = std::max(
       max_inline_size_, fragment.InlineSize() + child_margins.InlineSum() +
                             border_and_padding_.InlineSum());
   return NGLogicalOffset(inline_offset, block_offset);
 }
 
 NGLogicalOffset NGBlockLayoutAlgorithm::PositionFloatFragment(
     const NGFragment& fragment,
     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.
   LayoutUnit float_offset;
   if (current_child_->Style()->floating() == EFloat::Right) {
     float_offset = opportunity.size.inline_size - fragment.InlineSize();
   }
 
   // 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);
     is_fragment_margin_strut_block_start_updated_ = true;
   }
   builder_->SetMarginStrutBlockEnd(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_);
 }
 
 }  // namespace blink