[LayoutNG] Move ng_block_layout_algorithm to use constraint space builder.

[LayoutNG] Move ng_block_layout_algorithm to use constraint space builder.

This patch also moves the NGConstraintSpace to the constructor of the algorithm.

BUG=635619
Committed: https://crrev.com/c89a04f3ae1924c956b7bc732d429790bc57c3a8
Cr-Commit-Position: refs/heads/master@{#428536}

[ikilpatrick, 2016-10-27 21:16:11]

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

[ikilpatrick, 2016-10-27 21:16:46]

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

[commit-bot: I haz the power, 2016-10-27 21:17:25]

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

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:239: //
TODO(layout-ng): move ApplyAutoMargins to PositionFragment
Thanks

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:179: new
NGConstraintSpaceBuilder(constraint_space_->WritingMode());
You're not setting the direction? The old code did -- doesn't that mean the
constraint space will have the wrong direction set?

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:341:
FindLayoutOpportunityForFragment(constraint_space_, fragment, margins);
I don't think that's correct? If you use constraint_space_ you are finding the
layout opportunity based on the wrong width, which could then be outside of our
box. Better use constraint_space_for_current_child_?

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:179: new
NGConstraintSpaceBuilder(constraint_space_->WritingMode());
On 2016/10/27 21:47:19, cbiesinger wrote:
> You're not setting the direction? The old code did -- doesn't that mean the
> constraint space will have the wrong direction set?

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:341:
FindLayoutOpportunityForFragment(constraint_space_, fragment, margins);
On 2016/10/27 21:47:19, cbiesinger wrote:
> I don't think that's correct? If you use constraint_space_ you are finding the
> layout opportunity based on the wrong width, which could then be outside of
our
> box. Better use constraint_space_for_current_child_?

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

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

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

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

Thoughts?

[commit-bot: I haz the power, 2016-10-27 22:23:45]

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

[commit-bot: I haz the power, 2016-10-27 22:23:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Gleb Lanbin, 2016-10-27 22:30:07]

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:239: //
TODO(layout-ng): move ApplyAutoMargins to PositionFragment
On 2016/10/27 21:47:19, cbiesinger wrote:
> Thanks

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:360:
NGBlockLayoutAlgorithm::CreateConstraintSpaceForCurrentChild() {
1) move this to the unnamed namespace?

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:18: class
NGConstraintSpaceBuilder;
can we just include a header here instead of using forward declaration
https://google.github.io/styleguide/cppguide.html#Forward_Declarations
you're including the header in .cc file anyways

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:239: //
TODO(layout-ng): move ApplyAutoMargins to PositionFragment
On 2016/10/27 22:30:06, glebl wrote:
> On 2016/10/27 21:47:19, cbiesinger wrote:
> > Thanks
> 
> you need to run 'git rebase-update', it was deleted here
> http://crrev.com/2453553002

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:360:
NGBlockLayoutAlgorithm::CreateConstraintSpaceForCurrentChild() {
On 2016/10/27 22:30:06, glebl wrote:
> 1) move this to the unnamed namespace?

done?

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:18: class
NGConstraintSpaceBuilder;
On 2016/10/27 22:30:06, glebl wrote:
> can we just include a header here instead of using forward declaration
> https://google.github.io/styleguide/cppguide.html#Forward_Declarations
> you're including the header in .cc file anyways 

Done.

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

lgtm

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:179: new
NGConstraintSpaceBuilder(constraint_space_->WritingMode());
On 2016/10/27 21:56:13, ikilpatrick wrote:
> On 2016/10/27 21:47:19, cbiesinger wrote:
> > You're not setting the direction? The old code did -- doesn't that mean the
> > constraint space will have the wrong direction set?
> 
> This is only for the builder, which only needs to know about the direction at
> the moment, (only deals with {inline,block}->{width,height}) conversion at the
> moment).

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:341:
FindLayoutOpportunityForFragment(constraint_space_, fragment, margins);
On 2016/10/27 21:56:13, ikilpatrick wrote:
> On 2016/10/27 21:47:19, cbiesinger wrote:
> > I don't think that's correct? If you use constraint_space_ you are finding
the
> > layout opportunity based on the wrong width, which could then be outside of
> our
> > box. Better use constraint_space_for_current_child_?
> 
> Yeah see constraint_space_->SetSize(...) above.
> 
> Basically my logic here is:
>  - Current layout can modify it's "view" on it's on constraint space to return
> correct layout opportunities etc.
>  - constraint_space_for_current_child_ is wrong as once we are establishing
> BFC's properly that constraint space won't have any exclusions in it (for the
> float case).
> 
> We may want to explicitly store the content_size_ here and use that? For this
> calculation?
> 
> We are moving that direction for OppIterator anyway, i.e. instead of just:
>  - start_offset, leader_offset
> have...
>  - rect, leader_offset
> 
> Thoughts?

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

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:18: class
NGConstraintSpaceBuilder;
On 2016/10/27 22:30:06, glebl wrote:
> can we just include a header here instead of using forward declaration
> https://google.github.io/styleguide/cppguide.html#Forward_Declarations
> you're including the header in .cc file anyways 

That sucks that they changed the style guide like that :(

[ikilpatrick, 2016-10-28 19:43:50]

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

https://codereview.chromium.org/2456973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:341:
FindLayoutOpportunityForFragment(constraint_space_, fragment, margins);
On 2016/10/28 18:58:56, cbiesinger wrote:
> On 2016/10/27 21:56:13, ikilpatrick wrote:
> > On 2016/10/27 21:47:19, cbiesinger wrote:
> > > I don't think that's correct? If you use constraint_space_ you are finding
> the
> > > layout opportunity based on the wrong width, which could then be outside
of
> > our
> > > box. Better use constraint_space_for_current_child_?
> > 
> > Yeah see constraint_space_->SetSize(...) above.
> > 
> > Basically my logic here is:
> >  - Current layout can modify it's "view" on it's on constraint space to
return
> > correct layout opportunities etc.
> >  - constraint_space_for_current_child_ is wrong as once we are establishing
> > BFC's properly that constraint space won't have any exclusions in it (for
the
> > float case).
> > 
> > We may want to explicitly store the content_size_ here and use that? For
this
> > calculation?
> > 
> > We are moving that direction for OppIterator anyway, i.e. instead of just:
> >  - start_offset, leader_offset
> > have...
> >  - rect, leader_offset
> > 
> > Thoughts?
> 
> Ah I see. So, the basic concept is that instead of creating a "derived"
> constraint space and working on those we will just use constraint_space_ for
> everything and modify it as necessary. OK -- that sounds good. I suppose we
> didn't use to be able to do that because the constraint_space argument was
> const. May be a little confusing that the size is correct but there's no
correct
> offset in the constraint space (and that the caller of FindLayoutOpportunity
has
> to keep track of that) but this seems fine for now, we can revisit that
> separately if needed.

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

[ikilpatrick, 2016-10-28 20:26:27]

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

[commit-bot: I haz the power, 2016-10-28 20:27:03]

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, 2016-10-28 21:30:25]

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

[commit-bot: I haz the power, 2016-10-28 21:30:25]

Dry run: Try jobs failed on following builders:
  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, 2016-10-28 22:05:54]

lgtm

[ikilpatrick, 2016-10-28 22:10:33]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2016-10-28 22:11:16]

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-10-28 23:12:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-28 23:14:49]

Description was changed from

==========
[LayoutNG] Move ng_block_layout_algorithm to use constraint space builder.

This patch also moves the NGConstraintSpace to the constructor of the algorithm.

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

to

==========
[LayoutNG] Move ng_block_layout_algorithm to use constraint space builder.

This patch also moves the NGConstraintSpace to the constructor of the algorithm.

BUG=635619

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

[commit-bot: I haz the power, 2016-10-28 23:14:50]

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