Top down version of algorithm to position margins and floats in LayoutNG

Top down version of algorithm to position margins and floats in LayoutNG

This changes the current bottom up version of the algorithm that doesn't
handle a couple of impoortant use cases. For example when floats needed
to be positioned before processing the next in-flow child.

High level overview of changes:
- Changed the direction of block offset calculation for fragment(from
  bottom up to top down approach).
  Details: If child can determine its position in space it will return
  BFC offset to its parent. The child is able to calculate its position
  in space using NGMarginStrut that is set from the parent to child's
  CS.
- Floats are only positioned when the position is known otherwise they
  are stored in UnpositionedFloats and will be positioned by the next
  in-flow child that knows its position.
- ConstraintSpace uses BFC offset instead of logical offset.
- Fixed: Exclusions are stored in BFC coordinates now
         (this is needed to support intruding floats)
- Fixed: LayoutOpportunity returns a correct opportunity if
         AvailableSize is indefinite.
- Fixed: the top edge alignment rule for floats
- Fixed: Correct handling of adjoing blocks with clearance
- Fixed: Correct opportunity is returned if
         opportunity_candidate == float.size()
- Broken: Multicol. It will be fixed shortly. Priority 2
- Broken: Writing modes. It will be fixed shortly. Priority 1
          http://crrev.com/2651793013
- Tests: - enabled 6 margins/floats unittest, disabled 11 multicol
         unittests
         - updated layout tests expectations for layout_ng virtual
         test suite
         The most common issue with the currently enabled LayoutTests
         is that LayoutNG doesn't support layout test/inline yet.
         As a result LayoutNG doesn't get sizing information from
         LayoutText/LayoutInline blocks so the algorithm positions
         them as empty blocks.

Things that are not supported at this moment:
- Offset adjustment if a float is attached to a different parent
- multicol, writing modes
- Unpositioned floats needed to be available for positioning
  for text node descendants. This is needed to handle use cases like:
  <div id="empty">
    <float>
  </div>
  <div>
    <div>
      ...
      <text style="margin-top: 10px">
  </div>
  We can only position floats after we know the vertical baseline of
  the text block.

BUG=635619
Review-Url: https://codereview.chromium.org/2655783006
Cr-Commit-Position: refs/heads/master@{#447424}
Committed: https://chromium.googlesource.com/chromium/src/+/ff3381d6a029803db825aa0a52a49510f6273e5c

[Gleb Lanbin, 2017-01-25 20:18:56]

glebl@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[Gleb Lanbin, 2017-01-25 21:51:58]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2017-01-25 23:54:18]

Description was changed from

==========
Top down version of algorithm to position margins and floats in LayoutNG

This changes the current bottom up version of the algorithm that doesn't
handle a couple of impoortant use cases. For example when floats needed
to be positioned before processing the next in-flow child.

High level overview of changes:
- Changed the direction of block offset calculation for fragment(from
  bottom up to top down apptach).
  Details: If child can determine its position in space it will return
  BFC offset to its parent. The child is able to calculate its position
  in space using NGMarginStrut that is set from the parent to child's
  CS.
- Floats are only positioned when the position is known otherwise they
  are stored in UnpositionedFloats and will be positioned by the next
  in-flow child that knows its position.
- ConstraintSpace uses BFC offset instead of logical offset.
- Fixed: Exclusions are stored in BFC coordinates now
         (this is needed to support intruding floats)
- Fixed: LayoutOpportunity returns a correct opportunity if
         AvailableSize is indefinite.
- Fixed: the top edge alignment rule for floats
- Fixed: Correct handling of adjoing blocks with clearance
- Fixed: Correct opportunity is returned if
         opportunity_candidate == float.size()
- Broken: Multicol. It will be fixed shortly. Priority 2
- Broken: Writing modes. It will be fixed shortly. Priority 1

Things that are not supported at this moment:
- Offset adjustment if a float is attached to a different parent
- multicol, writing modes
- Unpositioned floats needed to be available for positioning
  for text node descendants. This is needed to handle use cases like:
  <div id="empty">
    <float>
  </div>
  <div>
    <div>
      ...
      <text style="margin-top: 10px">
  </div>
  We can only position floats after we know the vertical baseline of
  the text block.

BUG=635619
==========

to

==========
Top down version of algorithm to position margins and floats in LayoutNG

This changes the current bottom up version of the algorithm that doesn't
handle a couple of impoortant use cases. For example when floats needed
to be positioned before processing the next in-flow child.

High level overview of changes:
- Changed the direction of block offset calculation for fragment(from
  bottom up to top down approach).
  Details: If child can determine its position in space it will return
  BFC offset to its parent. The child is able to calculate its position
  in space using NGMarginStrut that is set from the parent to child's
  CS.
- Floats are only positioned when the position is known otherwise they
  are stored in UnpositionedFloats and will be positioned by the next
  in-flow child that knows its position.
- ConstraintSpace uses BFC offset instead of logical offset.
- Fixed: Exclusions are stored in BFC coordinates now
         (this is needed to support intruding floats)
- Fixed: LayoutOpportunity returns a correct opportunity if
         AvailableSize is indefinite.
- Fixed: the top edge alignment rule for floats
- Fixed: Correct handling of adjoing blocks with clearance
- Fixed: Correct opportunity is returned if
         opportunity_candidate == float.size()
- Broken: Multicol. It will be fixed shortly. Priority 2
- Broken: Writing modes. It will be fixed shortly. Priority 1

Things that are not supported at this moment:
- Offset adjustment if a float is attached to a different parent
- multicol, writing modes
- Unpositioned floats needed to be available for positioning
  for text node descendants. This is needed to handle use cases like:
  <div id="empty">
    <float>
  </div>
  <div>
    <div>
      ...
      <text style="margin-top: 10px">
  </div>
  We can only position floats after we know the vertical baseline of
  the text block.

BUG=635619
==========

[ikilpatrick, 2017-01-26 00:49:58]

about to leave for flight... to be continued.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:372:
curr_margin_strut_ = constraint_space_->MarginStrut();
ConstraintSpace() instead?

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:377: //  
border/padding between them
.nit period.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:427: //
Set offset if the size is not empty.
Set our BFC offset if ... ? (up to you)

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:433: if
(!Style().logicalHeight().isAuto()) {
TODO(glebl): handle minLogicalHeight, maxLogicalHeight.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:433: if
(!Style().logicalHeight().isAuto()) {
add comment explaining what's going on here.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:142:
is_new_fc_ ? NGLogicalOffset() : bfc_offset_, exclusions);
TODO ian think....

[ikilpatrick, 2017-01-26 04:37:26]

I'll do a more in-depth review on the weekend when I'm not jet-lagged :)

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:74: const
LayoutUnit& float_offset,
drop reference.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:148: const
LayoutUnit& float_offset,
drop the reference, this is a single int basically;

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:167: //
Calculates the relative position(from {@code from_offset}) of the
.nit space "position ("

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:175:
toNGPhysicalBoxFragment(floating_object->fragment));
DCHECK(floating_object->fragment) ?

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:200: void
PositionFloats(const NGLogicalOffset& origin_point,
is "Pending" floats a better name than unpositioned? Then this could be:
PositionPendingFloats?

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:714: // -
Position all pending floats as position is known now.
maybe a note/todo that we don't think this will work with a clear: left , and an
intruding right?

E.g. something along the lines of.... 
https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E...

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:730: //
Collapse the current margin strut with child's block start margin.
s/Collapse/Append? or maybe we should rename NGMarginStrut::Collapse to Sum? (up
to you).

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:731: //
non empty border/padding use cases are handled inside of the child's layout
.nit capital N, period.

[mstensho (USE GERRIT), 2017-01-26 09:19:07]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2017-01-26 09:19:08]

Not a complete review from me. From the description:

> - Unpositioned floats needed to be available for positioning
>   for text node descendants. This is needed to handle use cases like:
>   <div id="empty">
>     <float>
>   </div>
>   <div>
>     <div>
>       ...
>       <text style="margin-top: 10px">
>   </div>
>   We can only position floats after we know the vertical baseline of
>   the text block.

I'm not entirely sure what <text style="margin-top: 10px"> means, but if it's a
non-atomic inline, block-direction margins won't affect layout at all.
Furthermore, the baseline definitely cannot affect the position of the float, so
I'm a bit confused. I do realize that the BFC-position of <float> cannot
necessarily be determined while laying out #empty, though, since #empty may be
affected by some margin collapsing that takes place later. But that's exactly
what you're fixing in this CL, right?

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_units.cc (right):

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_units.cc:214: return
std::tie(other.margin, other.negative_margin) ==
What's the advantage with this over simply doing:

return margin == other.margin && negative_margin == other.negative_margin?

[Gleb Lanbin, 2017-01-30 18:17:50]

PTAL

On 2017/01/26 09:19:08, mstensho wrote:
> Not a complete review from me. From the description:
> 
> > - Unpositioned floats needed to be available for positioning
> >   for text node descendants. This is needed to handle use cases like:
> >   <div id="empty">
> >     <float>
> >   </div>
> >   <div>
> >     <div>
> >       ...
> >       <text style="margin-top: 10px">
> >   </div>
> >   We can only position floats after we know the vertical baseline of
> >   the text block.
> 
> I'm not entirely sure what <text style="margin-top: 10px"> means, but if it's
a
> non-atomic inline, block-direction margins won't affect layout at all.
> Furthermore, the baseline definitely cannot affect the position of the float,
so
> I'm a bit confused. I do realize that the BFC-position of <float> cannot
> necessarily be determined while laying out #empty, though, since #empty may be
> affected by some margin collapsing that takes place later. But that's exactly
> what you're fixing in this CL, right?

sorry for the confusion, I was trying to demonstrate this code snippet as an
example 
https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4848
Yes, this patch is changing the way we lay out floats to fix the use case where
float's position can be affected by margin collapsing happened later.

> 
>
https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/ng_units.cc (right):
> 
>
https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_units.cc:214: return
> std::tie(other.margin, other.negative_margin) ==
> What's the advantage with this over simply doing:
> 
> return margin == other.margin && negative_margin == other.negative_margin?

[ikilpatrick, 2017-01-30 22:26:49]

this is looking really good, thanks for your patience :)

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:26: //
Adjusts content_size to CSS "clear" property.
Adjusts content_size to respect the CSS "clear" property.

?

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:37: //
Exclusions(stored in BFC coordinates), content_size can be negative or 0
space between "s("

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:37: //
Exclusions(stored in BFC coordinates), content_size can be negative or 0
consider re-wording this line, not sure what it is saying.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: //
offset(exclusion.BlockEnd - from_offset) and compare them with content_size
.nit period.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:91: //
earlier in the source document
.nit period

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:294:
NGLogicalOffset NGBlockLayoutAlgorithm::PositionFragmentWithKnownBfcOffset(
It might be worth just placing this function inline with
FinishCurrentChildLayout. (or some of this function).

It's doing a bunch more than just determining a fragments offset (setting
parents offset and positioning pending floats).

Thoughts?

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:417:
content_size_ += curr_margin_strut_.Sum();
is there a test case for this?

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:426:
LayoutOutOfFlowChildren();
you may need to rebase :)

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:460:
fragment.PhysicalFragment()->UnpositionedFloats());
is it worth adding a DCHECK if there are unpositioned floats the bfc offset
isn't known?

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:467: // No
need to postpone the positioning as we know the correct offset.
// No need to postpone the positioning if we know the correct offset.

?

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:480: if
(!fragment.BlockSize()) {
You can remove this? This happens below:

 498   // Update margin strut.
 499   curr_margin_strut_ = fragment.EndMarginStrut();
 500   curr_margin_strut_.Append(curr_child_margins_.block_end);

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:13: return
toNGPhysicalBoxFragment(physical_fragment_)->BfcOffset();
may want to add a similar WRITING_MODE_IGNORED here.

[Gleb Lanbin, 2017-01-31 00:25:02]

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:74: const
LayoutUnit& float_offset,
On 2017/01/26 04:37:25, ikilpatrick wrote:
> drop reference.

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:148: const
LayoutUnit& float_offset,
On 2017/01/26 04:37:26, ikilpatrick wrote:
> drop the reference, this is a single int basically;

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:167: //
Calculates the relative position(from {@code from_offset}) of the
On 2017/01/26 04:37:25, ikilpatrick wrote:
> .nit space "position ("

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:175:
toNGPhysicalBoxFragment(floating_object->fragment));
On 2017/01/26 04:37:25, ikilpatrick wrote:
> DCHECK(floating_object->fragment) ?

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:200: void
PositionFloats(const NGLogicalOffset& origin_point,
On 2017/01/26 04:37:26, ikilpatrick wrote:
> is "Pending" floats a better name than unpositioned? Then this could be:
> PositionPendingFloats?

I thought about that. Here is my rationale behind naming this method as
PositionFloats:
- I think the word "unpositioned_floats" implies that it has the list of objects
that need to be positioned. "Pending" is not that descriptive. If I rename
everything to pending_floats then it would not be clear why/what they are
pending for.
- Also I find PositionUnpositionedFloats a bit tautological.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:372:
curr_margin_strut_ = constraint_space_->MarginStrut();
On 2017/01/26 00:49:58, ikilpatrick wrote:
> ConstraintSpace() instead?

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:377: //  
border/padding between them
On 2017/01/26 00:49:58, ikilpatrick wrote:
> .nit period.

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:427: //
Set offset if the size is not empty.
On 2017/01/26 00:49:57, ikilpatrick wrote:
> Set our BFC offset if ... ? (up to you)

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:433: if
(!Style().logicalHeight().isAuto()) {
On 2017/01/26 00:49:58, ikilpatrick wrote:
> TODO(glebl): handle minLogicalHeight, maxLogicalHeight.

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:433: if
(!Style().logicalHeight().isAuto()) {
On 2017/01/26 00:49:57, ikilpatrick wrote:
> add comment explaining what's going on here.

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:714: // -
Position all pending floats as position is known now.
On 2017/01/26 04:37:25, ikilpatrick wrote:
> maybe a note/todo that we don't think this will work with a clear: left , and
an
> intruding right?
> 
> E.g. something along the lines of.... 
>
https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E...

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:730: //
Collapse the current margin strut with child's block start margin.
On 2017/01/26 04:37:25, ikilpatrick wrote:
> s/Collapse/Append? or maybe we should rename NGMarginStrut::Collapse to Sum?
(up
> to you).

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:731: //
non empty border/padding use cases are handled inside of the child's layout
On 2017/01/26 04:37:26, ikilpatrick wrote:
> .nit capital N, period.

Done.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:142:
is_new_fc_ ? NGLogicalOffset() : bfc_offset_, exclusions);
On 2017/01/26 00:49:58, ikilpatrick wrote:
> TODO ian think.... 

sorry, probably I miss the context here. Can you elaborate what kind of work is
required here so I can add a TODO.

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_units.cc (right):

https://codereview.chromium.org/2655783006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_units.cc:214: return
std::tie(other.margin, other.negative_margin) ==
On 2017/01/26 09:19:08, mstensho wrote:
> What's the advantage with this over simply doing:
> 
> return margin == other.margin && negative_margin == other.negative_margin?

done. thanks. you are right your variant is shorter. I used std::tie for
consistency.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:26: //
Adjusts content_size to CSS "clear" property.
On 2017/01/30 22:26:48, ikilpatrick wrote:
> Adjusts content_size to respect the CSS "clear" property.
> 
> ?

Done.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:37: //
Exclusions(stored in BFC coordinates), content_size can be negative or 0
On 2017/01/30 22:26:48, ikilpatrick wrote:
> space between "s("

I also found it's confusing. I tried to rephrase it as
  // Use content_size offset or block end offset of the last left/right
  // float relative to the parent block offset (specified by {@code
from_offset}) 
but then decided to remove it completely. I think it's clear what this method
does from the comment attached to this function.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:39: //
offset(exclusion.BlockEnd - from_offset) and compare them with content_size
On 2017/01/30 22:26:48, ikilpatrick wrote:
> .nit period.

Done.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:91: //
earlier in the source document
On 2017/01/30 22:26:48, ikilpatrick wrote:
> .nit period

Done.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:294:
NGLogicalOffset NGBlockLayoutAlgorithm::PositionFragmentWithKnownBfcOffset(
On 2017/01/30 22:26:48, ikilpatrick wrote:
> It might be worth just placing this function inline with
> FinishCurrentChildLayout. (or some of this function).
> 
> It's doing a bunch more than just determining a fragments offset (setting
> parents offset and positioning pending floats).
> 
> Thoughts?

PTAL

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:417:
content_size_ += curr_margin_strut_.Sum();
On 2017/01/30 22:26:48, ikilpatrick wrote:
> is there a test case for this?

I believe it's covered in CollapsingMarginsCase4
// Verifies that 2 adjoining margins are not collapsed if there is padding or
// border that separates them.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:426:
LayoutOutOfFlowChildren();
On 2017/01/30 22:26:48, ikilpatrick wrote:
> you may need to rebase :)

Done.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:460:
fragment.PhysicalFragment()->UnpositionedFloats());
On 2017/01/30 22:26:49, ikilpatrick wrote:
> is it worth adding a DCHECK if there are unpositioned floats the bfc offset
> isn't known?

Done.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:467: // No
need to postpone the positioning as we know the correct offset.
On 2017/01/30 22:26:48, ikilpatrick wrote:
> // No need to postpone the positioning if we know the correct offset.
> 
> ?

Done.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:480: if
(!fragment.BlockSize()) {
On 2017/01/30 22:26:48, ikilpatrick wrote:
> You can remove this? This happens below:
> 
>  498   // Update margin strut.
>  499   curr_margin_strut_ = fragment.EndMarginStrut();
>  500   curr_margin_strut_.Append(curr_child_margins_.block_end);

thanks.

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc (right):

https://codereview.chromium.org/2655783006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc:13: return
toNGPhysicalBoxFragment(physical_fragment_)->BfcOffset();
On 2017/01/30 22:26:49, ikilpatrick wrote:
> may want to add a similar WRITING_MODE_IGNORED here.

Done.

[Gleb Lanbin, 2017-01-31 00:33:30]

Patchset #3 (id:60001) has been deleted

[ikilpatrick, 2017-01-31 01:08:56]

lgtm

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:478: if
(fragment.BfcOffset()) {
maybe add a comment for this block of code.

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:156:
margin_strut_, bfc_offset_, exclusions);
sorry, my comment before was me thinking...

these two args here should also have the is_new_fc_ check as above?

[ikilpatrick, 2017-01-31 01:10:01]

still lgtm \o/

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:425: if
(block_size) {
may also want a comment here.

[Gleb Lanbin, 2017-01-31 18:49:43]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 18:50:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Gleb Lanbin, 2017-01-31 18:57:30]

Description was changed from

==========
Top down version of algorithm to position margins and floats in LayoutNG

This changes the current bottom up version of the algorithm that doesn't
handle a couple of impoortant use cases. For example when floats needed
to be positioned before processing the next in-flow child.

High level overview of changes:
- Changed the direction of block offset calculation for fragment(from
  bottom up to top down approach).
  Details: If child can determine its position in space it will return
  BFC offset to its parent. The child is able to calculate its position
  in space using NGMarginStrut that is set from the parent to child's
  CS.
- Floats are only positioned when the position is known otherwise they
  are stored in UnpositionedFloats and will be positioned by the next
  in-flow child that knows its position.
- ConstraintSpace uses BFC offset instead of logical offset.
- Fixed: Exclusions are stored in BFC coordinates now
         (this is needed to support intruding floats)
- Fixed: LayoutOpportunity returns a correct opportunity if
         AvailableSize is indefinite.
- Fixed: the top edge alignment rule for floats
- Fixed: Correct handling of adjoing blocks with clearance
- Fixed: Correct opportunity is returned if
         opportunity_candidate == float.size()
- Broken: Multicol. It will be fixed shortly. Priority 2
- Broken: Writing modes. It will be fixed shortly. Priority 1

Things that are not supported at this moment:
- Offset adjustment if a float is attached to a different parent
- multicol, writing modes
- Unpositioned floats needed to be available for positioning
  for text node descendants. This is needed to handle use cases like:
  <div id="empty">
    <float>
  </div>
  <div>
    <div>
      ...
      <text style="margin-top: 10px">
  </div>
  We can only position floats after we know the vertical baseline of
  the text block.

BUG=635619
==========

to

==========
Top down version of algorithm to position margins and floats in LayoutNG

This changes the current bottom up version of the algorithm that doesn't
handle a couple of impoortant use cases. For example when floats needed
to be positioned before processing the next in-flow child.

High level overview of changes:
- Changed the direction of block offset calculation for fragment(from
  bottom up to top down approach).
  Details: If child can determine its position in space it will return
  BFC offset to its parent. The child is able to calculate its position
  in space using NGMarginStrut that is set from the parent to child's
  CS.
- Floats are only positioned when the position is known otherwise they
  are stored in UnpositionedFloats and will be positioned by the next
  in-flow child that knows its position.
- ConstraintSpace uses BFC offset instead of logical offset.
- Fixed: Exclusions are stored in BFC coordinates now
         (this is needed to support intruding floats)
- Fixed: LayoutOpportunity returns a correct opportunity if
         AvailableSize is indefinite.
- Fixed: the top edge alignment rule for floats
- Fixed: Correct handling of adjoing blocks with clearance
- Fixed: Correct opportunity is returned if
         opportunity_candidate == float.size()
- Broken: Multicol. It will be fixed shortly. Priority 2
- Broken: Writing modes. It will be fixed shortly. Priority 1
          http://crrev.com/2651793013
- Tests: - enabled 6 margins/floats unittest, disabled 11 multicol
         unittests
         - updated layout tests expectations for layout_ng virtual
         test suite
         The most common issue with the currently enabled LayoutTests
         is that LayoutNG doesn't support layout test/inline yet.
         As a result LayoutNG doesn't get sizing information from
         LayoutText/LayoutInline blocks so the algorithm positions
         them as empty blocks.

Things that are not supported at this moment:
- Offset adjustment if a float is attached to a different parent
- multicol, writing modes
- Unpositioned floats needed to be available for positioning
  for text node descendants. This is needed to handle use cases like:
  <div id="empty">
    <float>
  </div>
  <div>
    <div>
      ...
      <text style="margin-top: 10px">
  </div>
  We can only position floats after we know the vertical baseline of
  the text block.

BUG=635619
==========

[Gleb Lanbin, 2017-01-31 18:58:04]

thanks for the review

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:425: if
(block_size) {
On 2017/01/31 01:10:01, ikilpatrick wrote:
> may also want a comment here.

Done.

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:478: if
(fragment.BfcOffset()) {
On 2017/01/31 01:08:56, ikilpatrick wrote:
> maybe add a comment for this block of code.

Done.

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2655783006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:156:
margin_strut_, bfc_offset_, exclusions);
On 2017/01/31 01:08:56, ikilpatrick wrote:
> sorry, my comment before was me thinking...
> 
> these two args here should also have the is_new_fc_ check as above?

Done.

[commit-bot: I haz the power, 2017-01-31 19:03:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:03:55]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Gleb Lanbin, 2017-01-31 19:07:44]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 19:08:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:21:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:21:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Gleb Lanbin, 2017-01-31 19:40:30]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 19:41:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 21:31:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:31:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Gleb Lanbin, 2017-01-31 23:34:22]

The CQ bit was checked by glebl@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 23:35:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 01:27:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 01:27:44]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-02-01 02:29:44]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-02-01 02:29:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2655783006/#ps160001
(title: "git rebase-update")

[commit-bot: I haz the power, 2017-02-01 02:30:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 02:39:28]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485916184353040,
"parent_rev": "8be0713adccfe484752b791308bcf70412d49e7a", "commit_rev":
"ff3381d6a029803db825aa0a52a49510f6273e5c"}

[commit-bot: I haz the power, 2017-02-01 02:40:30]

Description was changed from

==========
Top down version of algorithm to position margins and floats in LayoutNG

This changes the current bottom up version of the algorithm that doesn't
handle a couple of impoortant use cases. For example when floats needed
to be positioned before processing the next in-flow child.

High level overview of changes:
- Changed the direction of block offset calculation for fragment(from
  bottom up to top down approach).
  Details: If child can determine its position in space it will return
  BFC offset to its parent. The child is able to calculate its position
  in space using NGMarginStrut that is set from the parent to child's
  CS.
- Floats are only positioned when the position is known otherwise they
  are stored in UnpositionedFloats and will be positioned by the next
  in-flow child that knows its position.
- ConstraintSpace uses BFC offset instead of logical offset.
- Fixed: Exclusions are stored in BFC coordinates now
         (this is needed to support intruding floats)
- Fixed: LayoutOpportunity returns a correct opportunity if
         AvailableSize is indefinite.
- Fixed: the top edge alignment rule for floats
- Fixed: Correct handling of adjoing blocks with clearance
- Fixed: Correct opportunity is returned if
         opportunity_candidate == float.size()
- Broken: Multicol. It will be fixed shortly. Priority 2
- Broken: Writing modes. It will be fixed shortly. Priority 1
          http://crrev.com/2651793013
- Tests: - enabled 6 margins/floats unittest, disabled 11 multicol
         unittests
         - updated layout tests expectations for layout_ng virtual
         test suite
         The most common issue with the currently enabled LayoutTests
         is that LayoutNG doesn't support layout test/inline yet.
         As a result LayoutNG doesn't get sizing information from
         LayoutText/LayoutInline blocks so the algorithm positions
         them as empty blocks.

Things that are not supported at this moment:
- Offset adjustment if a float is attached to a different parent
- multicol, writing modes
- Unpositioned floats needed to be available for positioning
  for text node descendants. This is needed to handle use cases like:
  <div id="empty">
    <float>
  </div>
  <div>
    <div>
      ...
      <text style="margin-top: 10px">
  </div>
  We can only position floats after we know the vertical baseline of
  the text block.

BUG=635619
==========

to

==========
Top down version of algorithm to position margins and floats in LayoutNG

This changes the current bottom up version of the algorithm that doesn't
handle a couple of impoortant use cases. For example when floats needed
to be positioned before processing the next in-flow child.

High level overview of changes:
- Changed the direction of block offset calculation for fragment(from
  bottom up to top down approach).
  Details: If child can determine its position in space it will return
  BFC offset to its parent. The child is able to calculate its position
  in space using NGMarginStrut that is set from the parent to child's
  CS.
- Floats are only positioned when the position is known otherwise they
  are stored in UnpositionedFloats and will be positioned by the next
  in-flow child that knows its position.
- ConstraintSpace uses BFC offset instead of logical offset.
- Fixed: Exclusions are stored in BFC coordinates now
         (this is needed to support intruding floats)
- Fixed: LayoutOpportunity returns a correct opportunity if
         AvailableSize is indefinite.
- Fixed: the top edge alignment rule for floats
- Fixed: Correct handling of adjoing blocks with clearance
- Fixed: Correct opportunity is returned if
         opportunity_candidate == float.size()
- Broken: Multicol. It will be fixed shortly. Priority 2
- Broken: Writing modes. It will be fixed shortly. Priority 1
          http://crrev.com/2651793013
- Tests: - enabled 6 margins/floats unittest, disabled 11 multicol
         unittests
         - updated layout tests expectations for layout_ng virtual
         test suite
         The most common issue with the currently enabled LayoutTests
         is that LayoutNG doesn't support layout test/inline yet.
         As a result LayoutNG doesn't get sizing information from
         LayoutText/LayoutInline blocks so the algorithm positions
         them as empty blocks.

Things that are not supported at this moment:
- Offset adjustment if a float is attached to a different parent
- multicol, writing modes
- Unpositioned floats needed to be available for positioning
  for text node descendants. This is needed to handle use cases like:
  <div id="empty">
    <float>
  </div>
  <div>
    <div>
      ...
      <text style="margin-top: 10px">
  </div>
  We can only position floats after we know the vertical baseline of
  the text block.

BUG=635619

Review-Url: https://codereview.chromium.org/2655783006
Cr-Commit-Position: refs/heads/master@{#447424}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff3381d6a029803db825aa0a52a4...
==========

[commit-bot: I haz the power, 2017-02-01 02:40:32]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ff3381d6a029803db825aa0a52a4...

[ikilpatrick, 2017-02-01 02:45:40]

On 2017/02/01 02:40:32, commit-bot: I haz the power wrote:
> Committed patchset #7 (id:160001) as
>
https://chromium.googlesource.com/chromium/src/+/ff3381d6a029803db825aa0a52a4...

\o/