[layoutng] Create a more correct constraint space for children

[layoutng] Create a more correct constraint space for children

At the start of layout, create a new constraint space with our size
but the existing exclusions. This will be used as the base for
layout out our children. In the future, we will call layoutOpportunities()
on this constraint space to find the correct place for our
children.

Copying the offset should not be necessary as we start with a fresh (0,0)
offset for our children, relative to us.

R=ikilpatrick@chromium.org,eae@chromium.org
BUG=635619
Committed: https://crrev.com/e66cd9caa74be985f45c426546d93555c2a5df53
Cr-Commit-Position: refs/heads/master@{#413817}

[cbiesinger, 2016-08-23 00:41:11]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2016-08-23 00:41:52]

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-08-23 00:41:53]

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

[commit-bot: I haz the power, 2016-08-23 00:41:54]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[cbiesinger, 2016-08-23 00:44:48]

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

[commit-bot: I haz the power, 2016-08-23 00:45:37]

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

[ikilpatrick, 2016-08-23 00:49:04]

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:27:
computeBlockSizeForFragment(constraintSpace, *m_style, LayoutUnit(-1));
Is the -1 correct? Should it be LayoutUnit(0) instead?
 - height percentages will get resolved to zero in this case?

(and constraintSpace.blockSize() for quirks mode?) :P

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:45: //
Recompute the block-axis size now that we know our content size
grammar-Nit: period and end?

(I don't really care if this isn't a rule).

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right):

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:56: // Constructs
a new constraint base based on an old one with a new size but
s/base/space

[cbiesinger, 2016-08-23 01:06:39]

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

[cbiesinger, 2016-08-23 01:06:55]

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:27:
computeBlockSizeForFragment(constraintSpace, *m_style, LayoutUnit(-1));
On 2016/08/23 00:49:04, ikilpatrick wrote:
> Is the -1 correct? Should it be LayoutUnit(0) instead?
>  - height percentages will get resolved to zero in this case?
> 
> (and constraintSpace.blockSize() for quirks mode?) :P

They don't get resolved to zero -- indefinite percentages get resolved to auto =
the height of their content.

(You're right about quirks mode, I'll add a TODO)

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:45: //
Recompute the block-axis size now that we know our content size
On 2016/08/23 00:49:04, ikilpatrick wrote:
> grammar-Nit: period and end?
> 
> (I don't really care if this isn't a rule).

Done.

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h (right):

https://codereview.chromium.org/2266313002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space.h:56: // Constructs
a new constraint base based on an old one with a new size but
On 2016/08/23 00:49:04, ikilpatrick wrote:
> s/base/space

Done.

[commit-bot: I haz the power, 2016-08-23 01:06:59]

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

[cbiesinger, 2016-08-23 01:08:09]

Come to think of it, ng_length_utils does not actually handle indefinite
percentages correctly :(

[commit-bot: I haz the power, 2016-08-23 03:57:55]

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

[commit-bot: I haz the power, 2016-08-23 03:57:56]

Dry run: This issue passed the CQ dry run.

[cbiesinger, 2016-08-23 15:59:11]

On 2016/08/23 01:08:09, cbiesinger wrote:
> Come to think of it, ng_length_utils does not actually handle indefinite
> percentages correctly :(

OK, fixing indefinite percentages is now
https://codereview.chromium.org/2274593002

Could you take another look at this one? Thanks!

[eae, 2016-08-23 16:54:09]

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

https://codereview.chromium.org/2266313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:26: //
TODO(layout-ng): For quirks mode, should we pass blockSize instead of -1?
Might make sense to have a NGQuirksModeBlockLayoutAlgorithm that inherits from
NGBlockLayoutAlgorithm to separate the two and avoiding the extra complexity in
the common case.

[cbiesinger, 2016-08-23 17:53:36]

On 2016/08/23 16:54:09, eae wrote:
>
https://codereview.chromium.org/2266313002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
> (right):
> 
>
https://codereview.chromium.org/2266313002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:26: //
> TODO(layout-ng): For quirks mode, should we pass blockSize instead of -1?
> Might make sense to have a NGQuirksModeBlockLayoutAlgorithm that inherits from
> NGBlockLayoutAlgorithm to separate the two and avoiding the extra complexity
in
> the common case.

That may be reasonable. But I don't really want to focus too much on quirks mode
yet -- I think for now we can keep this todo as it is?

[eae, 2016-08-23 18:07:57]

On 2016/08/23 17:53:36, cbiesinger wrote:
> On 2016/08/23 16:54:09, eae wrote:
> >
>
https://codereview.chromium.org/2266313002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2266313002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:26: //
> > TODO(layout-ng): For quirks mode, should we pass blockSize instead of -1?
> > Might make sense to have a NGQuirksModeBlockLayoutAlgorithm that inherits
from
> > NGBlockLayoutAlgorithm to separate the two and avoiding the extra complexity
> in
> > the common case.
> 
> That may be reasonable. But I don't really want to focus too much on quirks
mode
> yet -- I think for now we can keep this todo as it is?

SGTM

[ikilpatrick, 2016-08-23 19:42:15]

lgtm

[cbiesinger, 2016-08-23 19:42:56]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2016-08-23 19:43:42]

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-08-23 19:50:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-23 19:52:17]

Description was changed from

==========
[layoutng] Create a more correct constraint space for children

At the start of layout, create a new constraint space with our size
but the existing exclusions. This will be used as the base for
layout out our children. In the future, we will call layoutOpportunities()
on this constraint space to find the correct place for our
children.

Copying the offset should not be necessary as we start with a fresh (0,0)
offset for our children, relative to us.

R=ikilpatrick@chromium.org,eae@chromium.org
BUG=635619
==========

to

==========
[layoutng] Create a more correct constraint space for children

At the start of layout, create a new constraint space with our size
but the existing exclusions. This will be used as the base for
layout out our children. In the future, we will call layoutOpportunities()
on this constraint space to find the correct place for our
children.

Copying the offset should not be necessary as we start with a fresh (0,0)
offset for our children, relative to us.

R=ikilpatrick@chromium.org,eae@chromium.org
BUG=635619

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

[commit-bot: I haz the power, 2016-08-23 19:52:19]

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