[layout-ng] Convert ng_block_layout_algorithm to yield to the coordinator

This patch modifies ng_block_layout_algorithm to yield child
algorithms to the coordinator rather than run them directly.

(This is step 5 of https://docs.google.com/document/d/1vUmyaZoIqnVyvqAf5lFhnh_BdHYknLqViLISZiy-Iek/edit#)

BUG=636993
Committed: https://crrev.com/b5580819129cd70ee00e30df0b9ed2516c98e402
Cr-Commit-Position: refs/heads/master@{#437509}

[shans, 2016-11-22 05:56:18]

Description was changed from

==========
[WIP] block layout converted, but min-max doesn't work now.

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

to

==========
block layout converted, but min-max doesn't work yet.

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

[eae, 2016-11-23 22:05:14]

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

[eae, 2016-11-23 22:05:15]

This looks great! Let me know when you're ready for review.

[shans, 2016-12-01 00:12:38]

On 2016/11/23 at 22:05:15, eae wrote:
> This looks great! Let me know when you're ready for review.

This is ready for a review now, though I haven't tried to rebase since last week
so I might need to make some alterations still.

[eae, 2016-12-02 09:57:22]

LGTM

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

https://codereview.chromium.org/2519263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:103:
builder.SetAvailableSize(NGLogicalSize(LayoutUnit(), LayoutUnit()));
Isn't 0/0 the default for the builder? Perhaps we could add a special constuctor
for minmax to make it more explicit?

https://codereview.chromium.org/2519263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:107: // once
Rewrap

[shans, 2016-12-05 21:11:13]

I tweaked the block layout algorithm to remove repeated code. PTAL.

Should I get Ian to take a look at this before committing too?

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

https://codereview.chromium.org/2519263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:103:
builder.SetAvailableSize(NGLogicalSize(LayoutUnit(), LayoutUnit()));
On 2016/12/02 at 09:57:22, eae wrote:
> Isn't 0/0 the default for the builder? Perhaps we could add a special
constuctor for minmax to make it more explicit?

I think I picked up something like this in the merge I just did.

https://codereview.chromium.org/2519263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:107: // once
On 2016/12/02 at 09:57:22, eae wrote:
> Rewrap

Done.

[eae, 2016-12-06 10:40:48]

LGTM, thank you.

Please change the commit message to have a [LayoutNG] prefix and include a BUG
line. Thanks!

[ikilpatrick, 2016-12-06 21:45:42]

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

[ikilpatrick, 2016-12-06 21:45:43]

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:42:
while (algorithm.Layout(child_frag, &frag, &next_algorithm) !=
why isn't this using the layoutcoordinator?

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right):

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:53:
layout_coordinator_ = new NGLayoutCoordinator(this, constraint_space);
next patch is going to be removing this from here right?

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:101: // once that's
available.
this comment doesn't exist any more.

[shans, 2016-12-07 20:58:00]

Description was changed from

==========
block layout converted, but min-max doesn't work yet.

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

to

==========
This patch modifies ng_block_layout_algorithm to yield child
algorithms to the coordinator rather than run them directly.

(This is step 5 of
https://docs.google.com/document/d/1vUmyaZoIqnVyvqAf5lFhnh_BdHYknLqViLISZiy-I...)

BUG=636993
==========

[shans, 2016-12-07 21:00:52]

There was an additional comment that doesn't seem aligned with the code, asking
whether NGLayoutCoordinator construction is going to be removed from
NGBlockNode.

I don't know where coordinator construction should happen in the long term :)
Right now it happens the first time Layout is invoked on a BlockNode that can
participate in NG layout, and the NG process no longer recursively calls Layout
on blocks.

@ikilpatrick are you happy for me to commit or did you have more review
comments?

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:42:
while (algorithm.Layout(child_frag, &frag, &next_algorithm) !=
On 2016/12/06 at 21:45:43, ikilpatrick wrote:
> why isn't this using the layoutcoordinator?

Doing that would have required extensive modification of the test - e.g. the
first_child provided here would have to have a valid parent NGBlockNode, etc.

We could add some NGLayoutCoordinator tests too, but I didn't want to change the
essence of what this was testing (NGBlockLayoutAlgorithms)

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right):

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:101: // once that's
available.
On 2016/12/06 at 21:45:43, ikilpatrick wrote:
> this comment doesn't exist any more.

removed

[ikilpatrick, 2016-12-07 21:12:20]

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:42:
while (algorithm.Layout(child_frag, &frag, &next_algorithm) !=
On 2016/12/07 21:00:52, shans wrote:
> On 2016/12/06 at 21:45:43, ikilpatrick wrote:
> > why isn't this using the layoutcoordinator?
> 
> Doing that would have required extensive modification of the test - e.g. the
> first_child provided here would have to have a valid parent NGBlockNode, etc.
> 
> We could add some NGLayoutCoordinator tests too, but I didn't want to change
the
> essence of what this was testing (NGBlockLayoutAlgorithms)

Doesn't this work?

NGBlockNode parent(style_);
parent.SetFirstChild(first_child);

NGLayoutCoordinator coordinator(&parent, space);
NGPhysicalFragmentBase* fragment;
while (coordinator.Tick(&fragment));

return toNGPhysicalFragment(fragment);

[shans, 2016-12-07 21:27:15]

On 2016/12/07 at 21:12:20, ikilpatrick wrote:
>
https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
(right):
> 
>
https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:42:
while (algorithm.Layout(child_frag, &frag, &next_algorithm) !=
> On 2016/12/07 21:00:52, shans wrote:
> > On 2016/12/06 at 21:45:43, ikilpatrick wrote:
> > > why isn't this using the layoutcoordinator?
> > 
> > Doing that would have required extensive modification of the test - e.g. the
> > first_child provided here would have to have a valid parent NGBlockNode,
etc.
> > 
> > We could add some NGLayoutCoordinator tests too, but I didn't want to change
the
> > essence of what this was testing (NGBlockLayoutAlgorithms)
> 
> Doesn't this work?
> 
> NGBlockNode parent(style_);
> parent.SetFirstChild(first_child);
> 
> NGLayoutCoordinator coordinator(&parent, space);
> NGPhysicalFragmentBase* fragment;
> while (coordinator.Tick(&fragment));
> 
> return toNGPhysicalFragment(fragment);

It probably would, but at a minimum it's very unclear why that would be a
BlockLayoutAlgorithm unit test.

[atotic, 2016-12-07 21:28:47]

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

[atotic, 2016-12-07 21:28:48]

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right):

https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:124:
NGLayoutCoordinator* minmax_coordinator =
I thought LayoutCoordinator would be passed into ComputeMinAndMaxContentSizes?

The caller should have access to a coordinator before calling ComputeMinMax.

[ikilpatrick, 2016-12-07 21:29:10]

On 2016/12/07 21:27:15, shans wrote:
> On 2016/12/07 at 21:12:20, ikilpatrick wrote:
> >
>
https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
> > File
> third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2519263002/diff/60001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:42:
> while (algorithm.Layout(child_frag, &frag, &next_algorithm) !=
> > On 2016/12/07 21:00:52, shans wrote:
> > > On 2016/12/06 at 21:45:43, ikilpatrick wrote:
> > > > why isn't this using the layoutcoordinator?
> > > 
> > > Doing that would have required extensive modification of the test - e.g.
the
> > > first_child provided here would have to have a valid parent NGBlockNode,
> etc.
> > > 
> > > We could add some NGLayoutCoordinator tests too, but I didn't want to
change
> the
> > > essence of what this was testing (NGBlockLayoutAlgorithms)
> > 
> > Doesn't this work?
> > 
> > NGBlockNode parent(style_);
> > parent.SetFirstChild(first_child);
> > 
> > NGLayoutCoordinator coordinator(&parent, space);
> > NGPhysicalFragmentBase* fragment;
> > while (coordinator.Tick(&fragment));
> > 
> > return toNGPhysicalFragment(fragment);
> 
> It probably would, but at a minimum it's very unclear why that would be a
> BlockLayoutAlgorithm unit test.

I think we want to do this in this patch as otherwise it'll cause pain for the
first person that tries to do a 3-level deep unit test.

[shans, 2016-12-08 01:08:54]

@ikilpatrick how's this?

[ikilpatrick, 2016-12-08 21:55:58]

The CQ bit was checked by ikilpatrick@chromium.org

[ikilpatrick, 2016-12-08 21:55:59]

lgtm

[ikilpatrick, 2016-12-08 21:55:59]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2519263002/#ps100001
(title: "Modify block_layout_algorithm test to use coordinator and assert that a
block_layout_algorithm is t…")

[commit-bot: I haz the power, 2016-12-08 21:56:38]

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, 2016-12-08 22:00:23]

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

[commit-bot: I haz the power, 2016-12-08 22:00:25]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shans, 2016-12-09 05:19:04]

The CQ bit was checked by shans@chromium.org

[shans, 2016-12-09 05:19:05]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2519263002/#ps120001
(title: "sync to head")

[commit-bot: I haz the power, 2016-12-09 05:19:23]

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, 2016-12-09 07:20:19]

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

[commit-bot: I haz the power, 2016-12-09 07:20:20]

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_...)

[mstensho (USE GERRIT), 2016-12-09 08:40:09]

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

[mstensho (USE GERRIT), 2016-12-09 08:40:10]

Since landing failed again, I'd just like to point out that I think the commit
message isn't going to look too good here, since it lacks the first line,
followed by a blank line:

"[WIP] Convert ng_block_layout_algorithm to yield to the coordinator

"

[shans, 2016-12-09 08:45:52]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2016-12-09 08:46:13]

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

[mstensho (USE GERRIT), 2016-12-09 08:53:01]

On 2016/12/09 08:46:13, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Sorry, but can you stop this and fix the commit message before landing?

[shans, 2016-12-09 09:53:08]

On 2016/12/09 at 08:53:01, mstensho wrote:
> On 2016/12/09 08:46:13, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Sorry, but can you stop this and fix the commit message before landing?

I think I fixed it before re-committing?

I see "[layout-ng] Convert ng_block_layout_algorithm to yield to the
coordinator" as the title and

"This patch modifies ng_block_layout_algorithm to yield child
algorithms to the coordinator rather than run them directly.

(This is step 5 of
https://docs.google.com/document/d/1vUmyaZoIqnVyvqAf5lFhnh_BdHYknLqViLISZiy-I...)

BUG=636993" as the description. No blank lines :)

If it isn't fixed I guess I don't know how to do so.

[commit-bot: I haz the power, 2016-12-09 09:57:33]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481273152921720,
"parent_rev": "a335cd1a010fb8e15829b9d43e221a8fd4e81459", "commit_rev":
"956af94b0279979bc8d6f1cefced7c49885e3d4f"}

[mstensho (USE GERRIT), 2016-12-09 09:57:45]

On 2016/12/09 09:53:08, shans wrote:
> On 2016/12/09 at 08:53:01, mstensho wrote:
> > On 2016/12/09 08:46:13, commit-bot: I haz the power wrote:
> > > CQ is trying da patch. Follow status at
> > >  
> > >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> > 
> > Sorry, but can you stop this and fix the commit message before landing?
> 
> I think I fixed it before re-committing?
> 
> I see "[layout-ng] Convert ng_block_layout_algorithm to yield to the
> coordinator" as the title and
> 
> "This patch modifies ng_block_layout_algorithm to yield child
> algorithms to the coordinator rather than run them directly.
> 
> (This is step 5 of
>
https://docs.google.com/document/d/1vUmyaZoIqnVyvqAf5lFhnh_BdHYknLqViLISZiy-I...)
> 
> BUG=636993" as the description. No blank lines :)
> 
> If it isn't fixed I guess I don't know how to do so.

You need the header "[layout-ng] Convert ng_block_layout_algorithm to [...]" in
the description as well, or it will be missing in the final commit message. At
least that's how it was last time I checked.

[commit-bot: I haz the power, 2016-12-09 09:57:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-09 09:59:42]

Description was changed from

==========
This patch modifies ng_block_layout_algorithm to yield child
algorithms to the coordinator rather than run them directly.

(This is step 5 of
https://docs.google.com/document/d/1vUmyaZoIqnVyvqAf5lFhnh_BdHYknLqViLISZiy-I...)

BUG=636993
==========

to

==========
This patch modifies ng_block_layout_algorithm to yield child
algorithms to the coordinator rather than run them directly.

(This is step 5 of
https://docs.google.com/document/d/1vUmyaZoIqnVyvqAf5lFhnh_BdHYknLqViLISZiy-I...)

BUG=636993

Committed: https://crrev.com/b5580819129cd70ee00e30df0b9ed2516c98e402
Cr-Commit-Position: refs/heads/master@{#437509}
==========

[commit-bot: I haz the power, 2016-12-09 09:59:44]

Patchset 7 (id:??) landed as
https://crrev.com/b5580819129cd70ee00e30df0b9ed2516c98e402
Cr-Commit-Position: refs/heads/master@{#437509}

[mstensho (USE GERRIT), 2016-12-09 10:10:10]

Yup, the title is not part of the commit message.

[shans, 2016-12-09 10:35:23]

On 2016/12/09 at 10:10:10, mstensho wrote:
> Yup, the title is not part of the commit message.

Huh, fair enough. I guess I must have removed it from the description when
fixing the commit message earlier.