[LayoutNG] Allow block-flow layout to be fragmented using new approach.

[LayoutNG] Allow block-flow layout to be fragmented using new approach.

This patch removes most of the old fragmentation code for the new approach.

If the algorithm reaches a fragmentation line it'll produce a break token for
resuming later.

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

[ikilpatrick, 2017-02-24 20:40:17]

Description was changed from

==========
[LayoutNG] Allow block-flow layout to be fragmented using new approach.

BUG=
==========

to

==========
[LayoutNG] Allow block-flow layout to be fragmented using new approach.

This patch removes most of the old fragmentation code for the new approach.

If the algorithm reaches a fragmentation line it'll produce a break token for
resuming later.

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

[ikilpatrick, 2017-02-24 20:42:15]

ikilpatrick@chromium.org changed reviewers:
+ glebl@chromium.org, mstensho@opera.com

[ikilpatrick, 2017-02-24 20:42:16]

This is a rough first pass, it's currently missing support for parallel flow as
we'll need to re-jig how we layout floats for this.

[ikilpatrick, 2017-02-24 20:46:23]

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

[commit-bot: I haz the power, 2017-02-24 20:47:13]

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-24 20:56:07]

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

[commit-bot: I haz the power, 2017-02-24 20:56:07]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Gleb Lanbin, 2017-02-24 21:51:28]

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc (right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:31:
m_box->Layout(constraint_space, /* break_token */ nullptr);
make break_token optional?

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:367:
border_and_padding_ = ConstraintSpace().IsAnonymous()
may be introduce a helper function GetBorderPadding(ConstraintSpace(), Style())
in anonymous space and move comment and implementation there.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:400:
current_child_ = entry.node;
.nit
current_child_ = child_iterator.NextChild()

and provide getters
NGBlockNode* CurrentChild() {
  return current_child.node;
}
NGBreakToken* CurrentBreakToken() {..}

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:458: bool
is_out_of_space =
move the code and documentation to a helper function

if (IsOutOfSpace(content_size_, ConstraintSpace()))
  break

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:619:
DCHECK_GE(block_size, LayoutUnit());
DCHECK_GE(block_size, LayoutUnit()) << "Brief explanation why it's expected to
be >= 0";

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:623:
DCHECK(builder_->BfcOffset());
may be rephrase the comment a bit from the line 621 and move it to DCHECK ?
DCHECK(builder_->BfcOffset()) << "<Comment here>";

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:733: if
(constraint_space_->HasBlockFragmentation()) {
.nit
LayoutUnit space_available;
if (constraint_space_->HasBlockFragmentation()) {
  space_available = ConstraintSpace().FragmentainerSpaceAvailable();
  ...
}
space_builder_->SetFragmentainerSpaceAvailable(space_available);

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2134:
EXPECT_EQ(LayoutUnit(150), fragment->Width());
.nit 
use fragment->Size()
EXPECT_EQ(fragment->Size(), NGPhysicalSize(LayoutUnit(150), LayoutUnit(200)));

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2399:
/*TEST_F(NGBlockLayoutAlgorithmTest, ParallelFlowFragmentation) {
use DISABLED_ prefix and TODO, i.e. DISABLED_ParallelFlowFragmentation

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:38: NGBreakToken*
break_token) override;
NGBreakToken* break_token = nullptr)

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:84: if
(child->Type() == NGPhysicalBoxFragment::NGFragmentType::kFragmentBox &&
just
did_break_ = child->Type() == 
NGPhysicalBoxFragment::NGFragmentType::kFragmentBox &&
!child->BreakToken()->IsFinished();

or better add IsBox/IsText method to NGPhysicalBoxFragment so you can write
something like
did_break_ = child->IsBox() && !child->BreakToken()->IsFinished();

[ikilpatrick, 2017-02-25 00:57:24]

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc (right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/layout_ng_block_flow.cc:31:
m_box->Layout(constraint_space, /* break_token */ nullptr);
On 2017/02/24 21:51:27, Gleb Lanbin wrote:
> make break_token optional?

Done.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:367:
border_and_padding_ = ConstraintSpace().IsAnonymous()
On 2017/02/24 21:51:27, Gleb Lanbin wrote:
> may be introduce a helper function GetBorderPadding(ConstraintSpace(),
Style())
> in anonymous space and move comment and implementation there.

Done.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:400:
current_child_ = entry.node;
On 2017/02/24 21:51:27, Gleb Lanbin wrote:
> .nit
> current_child_ = child_iterator.NextChild()
> 
> and provide getters
> NGBlockNode* CurrentChild() {
>   return current_child.node;
> }
> NGBreakToken* CurrentBreakToken() {..}

Do you mind if I clean this up in a later patch? I was going to make
current_child a const ref argument into most functions. E.g. 
CreateConstraintSpaceForCurrentChild -> CreateConstraintSpaceForChild(const
NGLayoutInputNode& child, ...) , etc.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:458: bool
is_out_of_space =
On 2017/02/24 21:51:27, Gleb Lanbin wrote:
> move the code and documentation to a helper function
> 
> if (IsOutOfSpace(content_size_, ConstraintSpace()))
>   break

Done.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:619:
DCHECK_GE(block_size, LayoutUnit());
On 2017/02/24 21:51:27, Gleb Lanbin wrote:
> DCHECK_GE(block_size, LayoutUnit()) << "Brief explanation why it's expected to
> be >= 0";

Done.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:623:
DCHECK(builder_->BfcOffset());
On 2017/02/24 21:51:27, Gleb Lanbin wrote:
> may be rephrase the comment a bit from the line 621 and move it to DCHECK ?
> DCHECK(builder_->BfcOffset()) << "<Comment here>";

Done.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:733: if
(constraint_space_->HasBlockFragmentation()) {
On 2017/02/24 21:51:27, Gleb Lanbin wrote:
> .nit
> LayoutUnit space_available;
> if (constraint_space_->HasBlockFragmentation()) {
>   space_available = ConstraintSpace().FragmentainerSpaceAvailable();
>   ...
> }
> space_builder_->SetFragmentainerSpaceAvailable(space_available);

Done.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2134:
EXPECT_EQ(LayoutUnit(150), fragment->Width());
On 2017/02/24 21:51:28, Gleb Lanbin wrote:
> .nit 
> use fragment->Size()
> EXPECT_EQ(fragment->Size(), NGPhysicalSize(LayoutUnit(150), LayoutUnit(200)));

Done.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2399:
/*TEST_F(NGBlockLayoutAlgorithmTest, ParallelFlowFragmentation) {
On 2017/02/24 21:51:28, Gleb Lanbin wrote:
> use DISABLED_ prefix and TODO, i.e. DISABLED_ParallelFlowFragmentation

Blah sorry, removed, this was from an older patch, I can leave in, but i'm
guessing floats will look different to this.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:38: NGBreakToken*
break_token) override;
On 2017/02/24 21:51:28, Gleb Lanbin wrote:
> NGBreakToken* break_token = nullptr)

Done. Yeah I flipped back and forth on this a little. Other the other hand it
forces callers to be explicit, but probably fine.

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2714803002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:84: if
(child->Type() == NGPhysicalBoxFragment::NGFragmentType::kFragmentBox &&
On 2017/02/24 21:51:28, Gleb Lanbin wrote:
> just
> did_break_ = child->Type() == 
> NGPhysicalBoxFragment::NGFragmentType::kFragmentBox &&
> !child->BreakToken()->IsFinished();
> 
> or better add IsBox/IsText method to NGPhysicalBoxFragment so you can write
> something like
> did_break_ = child->IsBox() && !child->BreakToken()->IsFinished();

Done.

[Gleb Lanbin, 2017-02-25 01:12:19]

lgtm

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:571: // DO
NOT SUBMIT - Do we need to move this inside
do not forger to remove this before submit

[mstensho (USE GERRIT), 2017-02-27 13:44:37]

Nice tests!

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:314: //
Whether we've run out of space in this flow, if so there will be no work
Suggest changing punctuation and capitalization a little, for easier reading:

-> "... in this flow. If so, there will be..."

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:625: <<
"Adding and subtracting the used_block_size shouldn't leave the "
Isn't this going to fail when you have a block with fixed height that you
overflow (at least if the current fragmentainer only consists of an overflow
portion of the block)?

Like in the second fragmentainer here:

<div style="columns:2; column-fill:auto; height:100px;">
    <div id="overflowing" style="height:50px;">
        <div style="height:150px;"></div>
    </div>
</div>

Hmm.. but then there's the InnerChildrenFragmentationSmallHeight, which should
also fail then. So I guess there's something I don't understand.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:631:
builder_->BfcOffset().value().block_offset;
What does the block formatting context offset have to do with space left
calculation? Block formatting contexts should be independent of fragmentation.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:635: // A
break token is ready, which means that we're going to break
Looks like this comment now needs to be updated. There's no "break token [...]
ready" anymore, is there?

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:747:
space_available -= curr_bfc_offset_.block_offset;
Again, I don't understand the link between block formatting context and
fragmentation. It seems to me that you need to make this adjustment for any
block, regardless of it establishing a new block formatting context or not.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2138:
// Tests that a block child with fragment if it reaches the fragmentation line.
I don't think I can parse this sentence, but I think I know what you mean. :)

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2306:
// Tests that children inside which establish new formatting contexts fragment
Comment is the same as the one before the previous test. Probably not
intentional, since there's no new BFC here.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:57: NGBreakToken*
CurrentBreakToken() const;
Should remove this now, I guess.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:58: bool
IsLayoutFinished() const {
And this.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:168: unsigned
is_anonymous_ : 1;
Should probably add a blank line above this, since it has nothing to do with the
comment above.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:46:
NGConstraintSpaceBuilder& SetIsAnonymous(bool is_anonymous);
Does it ever make sense to pass false here? I.e. could we just remove the
parameter, perhaps?

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:79:
NGFragmentBuilder& SetUsedBlockSize(LayoutUnit used_block_size) {
I think this one needs documentation, and perhaps a better name, since it does
two things: set used block size and mark that we did break.

[ikilpatrick, 2017-02-27 18:50:10]

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:314: //
Whether we've run out of space in this flow, if so there will be no work
On 2017/02/27 13:44:36, mstensho wrote:
> Suggest changing punctuation and capitalization a little, for easier reading:
> 
> -> "... in this flow. If so, there will be..."

Done.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:571: // DO
NOT SUBMIT - Do we need to move this inside
On 2017/02/25 01:12:19, Gleb Lanbin wrote:
> do not forger to remove this before submit

Added TODO

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:625: <<
"Adding and subtracting the used_block_size shouldn't leave the "
On 2017/02/27 13:44:36, mstensho wrote:
> Isn't this going to fail when you have a block with fixed height that you
> overflow (at least if the current fragmentainer only consists of an overflow
> portion of the block)?
> 
> Like in the second fragmentainer here:
> 
> <div style="columns:2; column-fill:auto; height:100px;">
>     <div id="overflowing" style="height:50px;">
>         <div style="height:150px;"></div>
>     </div>
> </div>
> 
> Hmm.. but then there's the InnerChildrenFragmentationSmallHeight, which should
> also fail then. So I guess there's something I don't understand.

It doesn't fail is it's DCHECK_GE not DCHECK_GT.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:631:
builder_->BfcOffset().value().block_offset;
On 2017/02/27 13:44:36, mstensho wrote:
> What does the block formatting context offset have to do with space left
> calculation? Block formatting contexts should be independent of fragmentation.

We need a bunch more documentation around this BfcOffset concept. Basically:

1) Some children (in-flow, non-formatting-context, blocks) don't know their
position relative to parent until they've laid out their children, e.g.
zero-block-size elements can effect their position.

2) We need this position for calculating a few things, e.g. where the intruding
floats will be, where the fragmentation line will end up hitting us.

3) This is what the builder_->BfcOffset is used for,  this says our position
relative to our parent block formatting context. All geometry information is
relative to the formatting context on the constraint space.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:635: // A
break token is ready, which means that we're going to break
On 2017/02/27 13:44:36, mstensho wrote:
> Looks like this comment now needs to be updated. There's no "break token [...]
> ready" anymore, is there?

Done.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:747:
space_available -= curr_bfc_offset_.block_offset;
On 2017/02/27 13:44:36, mstensho wrote:
> Again, I don't understand the link between block formatting context and
> fragmentation. It seems to me that you need to make this adjustment for any
> block, regardless of it establishing a new block formatting context or not.

See above, basically the fragmentation line is always relative the parent
formatting context, hence if a child establishes a new formatting context we
need to adjust. This might be clearer inside ng_constraint_space_builder.cc ?

E.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng...

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2138:
// Tests that a block child with fragment if it reaches the fragmentation line.
On 2017/02/27 13:44:36, mstensho wrote:
> I don't think I can parse this sentence, but I think I know what you mean. :)

s/with/will :P

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:2306:
// Tests that children inside which establish new formatting contexts fragment
On 2017/02/27 13:44:36, mstensho wrote:
> Comment is the same as the one before the previous test. Probably not
> intentional, since there's no new BFC here.

Done.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:57: NGBreakToken*
CurrentBreakToken() const;
On 2017/02/27 13:44:36, mstensho wrote:
> Should remove this now, I guess.

Done.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_node.h:58: bool
IsLayoutFinished() const {
On 2017/02/27 13:44:36, mstensho wrote:
> And this.

Done.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:168: unsigned
is_anonymous_ : 1;
On 2017/02/27 13:44:36, mstensho wrote:
> Should probably add a blank line above this, since it has nothing to do with
the
> comment above.

Done.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:46:
NGConstraintSpaceBuilder& SetIsAnonymous(bool is_anonymous);
On 2017/02/27 13:44:36, mstensho wrote:
> Does it ever make sense to pass false here? I.e. could we just remove the
> parameter, perhaps?

So the constraint space builder can be re-used for multiple children. In
multi-cols case, if we see a spanner we'll probably want to set this to false?

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h (right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.h:79:
NGFragmentBuilder& SetUsedBlockSize(LayoutUnit used_block_size) {
On 2017/02/27 13:44:36, mstensho wrote:
> I think this one needs documentation, and perhaps a better name, since it does
> two things: set used block size and mark that we did break.

PTAL.

[cbiesinger, 2017-02-27 19:09:36]

cbiesinger@chromium.org changed reviewers:
+ cbiesinger@chromium.org

[cbiesinger, 2017-02-27 19:09:38]

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:303:
return space.IsAnonymous()
It worries me a bit to have this check only here. What if code only calls
ComputePadding -- should that take IsAnonymous into account as well?

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
(right):

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:53: //
Final adjusstments before fragment creation. We need to prevent the
While you're touching the related code, do you want to fix this typo?
(adjusstments -> adjustments)

[ikilpatrick, 2017-02-27 19:29:20]

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

[ikilpatrick, 2017-02-27 19:29:34]

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:303:
return space.IsAnonymous()
On 2017/02/27 19:09:37, cbiesinger wrote:
> It worries me a bit to have this check only here. What if code only calls
> ComputePadding -- should that take IsAnonymous into account as well?

Yeah, thoughts on moving this into ComputeBorders and ComputePadding?

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
(right):

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:53: //
Final adjusstments before fragment creation. We need to prevent the
On 2017/02/27 19:09:37, cbiesinger wrote:
> While you're touching the related code, do you want to fix this typo?
> (adjusstments -> adjustments)

Done.

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

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

[cbiesinger, 2017-02-27 19:32:03]

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:303:
return space.IsAnonymous()
On 2017/02/27 19:29:34, ikilpatrick wrote:
> On 2017/02/27 19:09:37, cbiesinger wrote:
> > It worries me a bit to have this check only here. What if code only calls
> > ComputePadding -- should that take IsAnonymous into account as well?
> 
> Yeah, thoughts on moving this into ComputeBorders and ComputePadding? 

Yes I think that would be good. ComputeBorders will need to take a
ConstraintSpace that it didn't use to but that should be fine.

[mstensho (USE GERRIT), 2017-02-27 20:28:21]

lgtm - you've resolved all the issues I had raised.

I found three new ones, though, but feel free to postpone that to a future CL.
:)

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:625: <<
"Adding and subtracting the used_block_size shouldn't leave the "
On 2017/02/27 18:50:09, ikilpatrick wrote:
> On 2017/02/27 13:44:36, mstensho wrote:
> > Isn't this going to fail when you have a block with fixed height that you
> > overflow (at least if the current fragmentainer only consists of an overflow
> > portion of the block)?
> > 
> > Like in the second fragmentainer here:
> > 
> > <div style="columns:2; column-fill:auto; height:100px;">
> >     <div id="overflowing" style="height:50px;">
> >         <div style="height:150px;"></div>
> >     </div>
> > </div>
> > 
> > Hmm.. but then there's the InnerChildrenFragmentationSmallHeight, which
should
> > also fail then. So I guess there's something I don't understand.
> 
> It doesn't fail is it's DCHECK_GE not DCHECK_GT.

Yeah, that wasn't my problem. I was thinking that used_block_space also included
space used in the overflow area, but it doesn't, so I understand why this works
now.

Ack.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:631:
builder_->BfcOffset().value().block_offset;
On 2017/02/27 18:50:09, ikilpatrick wrote:
> On 2017/02/27 13:44:36, mstensho wrote:
> > What does the block formatting context offset have to do with space left
> > calculation? Block formatting contexts should be independent of
fragmentation.
> 
> We need a bunch more documentation around this BfcOffset concept. Basically:
> 
> 1) Some children (in-flow, non-formatting-context, blocks) don't know their
> position relative to parent until they've laid out their children, e.g.
> zero-block-size elements can effect their position.
> 
> 2) We need this position for calculating a few things, e.g. where the
intruding
> floats will be, where the fragmentation line will end up hitting us.
> 
> 3) This is what the builder_->BfcOffset is used for,  this says our position
> relative to our parent block formatting context. All geometry information is
> relative to the formatting context on the constraint space.

Acknowledged.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:637:
builder_->SetUsedBlockSize(std::min(space_left, block_size) +
std::min(space_left, block_size) called twice in a row now. Maybe store the
result somewhere instead. Hijack block_size or introduce a new variable?

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:747:
space_available -= curr_bfc_offset_.block_offset;
On 2017/02/27 18:50:09, ikilpatrick wrote:
> On 2017/02/27 13:44:36, mstensho wrote:
> > Again, I don't understand the link between block formatting context and
> > fragmentation. It seems to me that you need to make this adjustment for any
> > block, regardless of it establishing a new block formatting context or not.
> 
> See above, basically the fragmentation line is always relative the parent
> formatting context, hence if a child establishes a new formatting context we
> need to adjust. This might be clearer inside ng_constraint_space_builder.cc ?
> 
> E.g.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng...

Acknowledged.

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h
(right):

https://codereview.chromium.org/2714803002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.h:46:
NGConstraintSpaceBuilder& SetIsAnonymous(bool is_anonymous);
On 2017/02/27 18:50:09, ikilpatrick wrote:
> On 2017/02/27 13:44:36, mstensho wrote:
> > Does it ever make sense to pass false here? I.e. could we just remove the
> > parameter, perhaps?
> 
> So the constraint space builder can be re-used for multiple children. In
> multi-cols case, if we see a spanner we'll probably want to set this to false?

Acknowledged.

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (left):

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:31: //
TODO(ikilpatrick): Remove this constructor and break_offset once we've
Now is the time, I guess?

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(left):

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:10:
#include "core/layout/ng/ng_column_mapper.h"
NGColumnMapper is now unused, as expected. Would you mind killing it right away,
or is there still code in there that needs to be migrated over to the new world?

[ikilpatrick, 2017-02-27 20:44:55]

cbiesinger@ would you mind having another pass?

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2714803002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:303:
return space.IsAnonymous()
On 2017/02/27 19:32:03, cbiesinger wrote:
> On 2017/02/27 19:29:34, ikilpatrick wrote:
> > On 2017/02/27 19:09:37, cbiesinger wrote:
> > > It worries me a bit to have this check only here. What if code only calls
> > > ComputePadding -- should that take IsAnonymous into account as well?
> > 
> > Yeah, thoughts on moving this into ComputeBorders and ComputePadding? 
> 
> Yes I think that would be good. ComputeBorders will need to take a
> ConstraintSpace that it didn't use to but that should be fine.

Done. PTAL

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h (left):

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_break_token.h:31: //
TODO(ikilpatrick): Remove this constructor and break_offset once we've
On 2017/02/27 20:28:20, mstensho wrote:
> Now is the time, I guess?

Done!

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(left):

https://codereview.chromium.org/2714803002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:10:
#include "core/layout/ng/ng_column_mapper.h"
On 2017/02/27 20:28:20, mstensho wrote:
> NGColumnMapper is now unused, as expected. Would you mind killing it right
away,
> or is there still code in there that needs to be migrated over to the new
world?

Done.

[ikilpatrick, 2017-02-27 22:24:41]

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

[commit-bot: I haz the power, 2017-02-27 22:25:17]

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

[cbiesinger, 2017-02-27 22:54:16]

lgtm

[commit-bot: I haz the power, 2017-02-27 23:40:29]

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

[commit-bot: I haz the power, 2017-02-27 23:40:29]

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

[ikilpatrick, 2017-02-28 00:08:42]

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

[commit-bot: I haz the power, 2017-02-28 00:09:09]

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-28 02:03:59]

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

[commit-bot: I haz the power, 2017-02-28 02:04:00]

Dry run: This issue passed the CQ dry run.

[ikilpatrick, 2017-02-28 02:32:54]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2017-02-28 02:32:55]

The patchset sent to the CQ was uploaded after l-g-t-m from glebl@chromium.org,
mstensho@opera.com, cbiesinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2714803002/#ps260001
(title: "rebase.")

[commit-bot: I haz the power, 2017-02-28 02:33:29]

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-28 04:52:24]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1488249174982860,
"parent_rev": "0decaefae68c308e45aad3b3fbda9c5b9a6793a8", "commit_rev":
"c0b2149ee87d2451e91885d1a3378aa4a7c1dad9"}

[commit-bot: I haz the power, 2017-02-28 04:53:12]

Description was changed from

==========
[LayoutNG] Allow block-flow layout to be fragmented using new approach.

This patch removes most of the old fragmentation code for the new approach.

If the algorithm reaches a fragmentation line it'll produce a break token for
resuming later.

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

to

==========
[LayoutNG] Allow block-flow layout to be fragmented using new approach.

This patch removes most of the old fragmentation code for the new approach.

If the algorithm reaches a fragmentation line it'll produce a break token for
resuming later.

BUG=635619

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

[commit-bot: I haz the power, 2017-02-28 04:53:15]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/c0b2149ee87d2451e91885d1a337...