[css-flexbox] Correctly resolve percentages in children of stretched flex items

[css-flexbox] Correctly resolve percentages in children of stretched flex items

percent-height-children-of-alignSelf-stretch-flex-item.html
was taken from a previous attempt at this patch, by harpreet.sk:
https://codereview.chromium.org/331203002

Note that the behavior has changed relative to that patch. This
patch implements the current required behavior by the spec.

BUG=341310
Committed: https://crrev.com/0c12856b59827e462642d3ed8ce2d7042bcf7218
Cr-Commit-Position: refs/heads/master@{#378690}

[cbiesinger, 2016-02-27 02:26:52]

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

[commit-bot: I haz the power, 2016-02-27 02:28:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1742973002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1742973002/1

[cbiesinger, 2016-02-27 02:50:29]

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

[commit-bot: I haz the power, 2016-02-27 02:52:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1742973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1742973002/20001

[commit-bot: I haz the power, 2016-02-27 04:11:14]

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

[commit-bot: I haz the power, 2016-02-27 04:11:14]

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

[cbiesinger, 2016-02-29 21:55:25]

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

[cbiesinger, 2016-02-29 21:56:51]

Description was changed from

==========
[css-flexbox] Correctly resolve percentages in children of stretched flex items

BUG=341310
==========

to

==========
[css-flexbox] Correctly resolve percentages in children of stretched flex items

percent-height-children-of-alignSelf-stretch-flex-item.html
was taken from a previous attempt at this patch, by harpreet.sk:
https://codereview.chromium.org/331203002

Note that the behavior has changed relative to that patch. This
patch implements the current required behavior by the spec.

BUG=341310
==========

[commit-bot: I haz the power, 2016-02-29 21:57:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1742973002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1742973002/40001

[cbiesinger, 2016-02-29 21:57:48]

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

[cbiesinger, 2016-02-29 21:57:49]

Levi, I believe this is ready for review.

[cbiesinger, 2016-02-29 22:03:54]

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1074: return
LayoutUnit(-1);
Note, this part of the two new functions will be used for point 2) of
https://drafts.csswg.org/css-flexbox/#definite-sizes

[commit-bot: I haz the power, 2016-03-01 00:39:26]

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

[commit-bot: I haz the power, 2016-03-01 00:39:27]

Dry run: This issue passed the CQ dry run.

[leviw_travelin_and_unemployed, 2016-03-01 01:58:52]

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2104: } else if
(styleToUse.logicalWidth().hasPercent() && !isOutOfFlowPositioned() &&
cb->isFlexItem()) {
Nit, I may have put the isFlexItem() part first.

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2105: LayoutUnit
stretchedWidth =
toLayoutFlexibleBox(cb->parent())->childLogicalHeightForPercentageResolution(*cb);
Link to spec about why width = height?

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1070: LayoutUnit
LayoutFlexibleBox::childLogicalWidthForPercentageResolution(const LayoutBox&
child)
This will be used later? I'd rather it land with the patch that uses it.

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right):

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:65: LayoutUnit
childLogicalWidthForPercentageResolution(const LayoutBox& child);
This is never called.

[cbiesinger, 2016-03-01 20:36:11]

PTAL

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2104: } else if
(styleToUse.logicalWidth().hasPercent() && !isOutOfFlowPositioned() &&
cb->isFlexItem()) {
On 2016/03/01 01:58:51, leviw wrote:
> Nit, I may have put the isFlexItem() part first.

Done.

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2105: LayoutUnit
stretchedWidth =
toLayoutFlexibleBox(cb->parent())->childLogicalHeightForPercentageResolution(*cb);
On 2016/03/01 01:58:51, leviw wrote:
> Link to spec about why width = height?

That was a typo :(

I added a test that hits this case.

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1070: LayoutUnit
LayoutFlexibleBox::childLogicalWidthForPercentageResolution(const LayoutBox&
child)
On 2016/03/01 01:58:51, leviw wrote:
> This will be used later? I'd rather it land with the patch that uses it.

This is now used.

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right):

https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:65: LayoutUnit
childLogicalWidthForPercentageResolution(const LayoutBox& child);
On 2016/03/01 01:58:51, leviw wrote:
> This is never called.

This is now used.

[cbiesinger, 2016-03-01 20:55:28]

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

[commit-bot: I haz the power, 2016-03-01 20:56:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1742973002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1742973002/60001

[leviw_travelin_and_unemployed, 2016-03-01 21:03:48]

On 2016/03/01 at 20:36:11, cbiesinger wrote:
> PTAL
> 
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:2104: } else if
(styleToUse.logicalWidth().hasPercent() && !isOutOfFlowPositioned() &&
cb->isFlexItem()) {
> On 2016/03/01 01:58:51, leviw wrote:
> > Nit, I may have put the isFlexItem() part first.
> 
> Done.
> 
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:2105: LayoutUnit
stretchedWidth =
toLayoutFlexibleBox(cb->parent())->childLogicalHeightForPercentageResolution(*cb);
> On 2016/03/01 01:58:51, leviw wrote:
> > Link to spec about why width = height?
> 
> That was a typo :(
> 
> I added a test that hits this case.
> 
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):
> 
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1070: LayoutUnit
LayoutFlexibleBox::childLogicalWidthForPercentageResolution(const LayoutBox&
child)
> On 2016/03/01 01:58:51, leviw wrote:
> > This will be used later? I'd rather it land with the patch that uses it.
> 
> This is now used.
> 
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right):
> 
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:65: LayoutUnit
childLogicalWidthForPercentageResolution(const LayoutBox& child);
> On 2016/03/01 01:58:51, leviw wrote:
> > This is never called.
> 
> This is now used.

So do we have test coverage for those cases then? Seems they're missing if the
tests didn't catch that typo.

[cbiesinger, 2016-03-01 21:04:30]

On 2016/03/01 21:03:48, leviw wrote:
> On 2016/03/01 at 20:36:11, cbiesinger wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2104: } else if
> (styleToUse.logicalWidth().hasPercent() && !isOutOfFlowPositioned() &&
> cb->isFlexItem()) {
> > On 2016/03/01 01:58:51, leviw wrote:
> > > Nit, I may have put the isFlexItem() part first.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBox.cpp:2105: LayoutUnit
> stretchedWidth =
>
toLayoutFlexibleBox(cb->parent())->childLogicalHeightForPercentageResolution(*cb);
> > On 2016/03/01 01:58:51, leviw wrote:
> > > Link to spec about why width = height?
> > 
> > That was a typo :(
> > 
> > I added a test that hits this case.
> > 
> >
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1070: LayoutUnit
> LayoutFlexibleBox::childLogicalWidthForPercentageResolution(const LayoutBox&
> child)
> > On 2016/03/01 01:58:51, leviw wrote:
> > > This will be used later? I'd rather it land with the patch that uses it.
> > 
> > This is now used.
> > 
> >
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right):
> > 
> >
>
https://codereview.chromium.org/1742973002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:65: LayoutUnit
> childLogicalWidthForPercentageResolution(const LayoutBox& child);
> > On 2016/03/01 01:58:51, leviw wrote:
> > > This is never called.
> > 
> > This is now used.
> 
> So do we have test coverage for those cases then? Seems they're missing if the
> tests didn't catch that typo.

Yes, we didn't have that coverage but I added a test for it in this version of
the patch.

[commit-bot: I haz the power, 2016-03-01 22:26:56]

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

[commit-bot: I haz the power, 2016-03-01 22:26:57]

Dry run: This issue passed the CQ dry run.

[leviw_travelin_and_unemployed, 2016-03-01 23:53:56]

lgtm

https://codereview.chromium.org/1742973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1742973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1031: if
(isMultiline() || alignmentForChild(child) != ItemPositionStretch ||
hasAutoMarginsInCrossAxis(child))
Is there a part of the spec that specifically mentions the multiline case? If
so, can we reference it here?

[cbiesinger, 2016-03-02 03:19:37]

https://codereview.chromium.org/1742973002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1742973002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1031: if
(isMultiline() || alignmentForChild(child) != ItemPositionStretch ||
hasAutoMarginsInCrossAxis(child))
On 2016/03/01 23:53:56, leviw wrote:
> Is there a part of the spec that specifically mentions the multiline case? If
> so, can we reference it here?

Done.

[cbiesinger, 2016-03-02 03:19:43]

The CQ bit was checked by cbiesinger@chromium.org

[cbiesinger, 2016-03-02 03:19:44]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1742973002/#ps80001
(title: "add comment")

[commit-bot: I haz the power, 2016-03-02 03:20:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1742973002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1742973002/80001

[commit-bot: I haz the power, 2016-03-02 04:46:05]

Description was changed from

==========
[css-flexbox] Correctly resolve percentages in children of stretched flex items

percent-height-children-of-alignSelf-stretch-flex-item.html
was taken from a previous attempt at this patch, by harpreet.sk:
https://codereview.chromium.org/331203002

Note that the behavior has changed relative to that patch. This
patch implements the current required behavior by the spec.

BUG=341310
==========

to

==========
[css-flexbox] Correctly resolve percentages in children of stretched flex items

percent-height-children-of-alignSelf-stretch-flex-item.html
was taken from a previous attempt at this patch, by harpreet.sk:
https://codereview.chromium.org/331203002

Note that the behavior has changed relative to that patch. This
patch implements the current required behavior by the spec.

BUG=341310
==========

[commit-bot: I haz the power, 2016-03-02 04:46:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-02 04:47:07]

Description was changed from

==========
[css-flexbox] Correctly resolve percentages in children of stretched flex items

percent-height-children-of-alignSelf-stretch-flex-item.html
was taken from a previous attempt at this patch, by harpreet.sk:
https://codereview.chromium.org/331203002

Note that the behavior has changed relative to that patch. This
patch implements the current required behavior by the spec.

BUG=341310
==========

to

==========
[css-flexbox] Correctly resolve percentages in children of stretched flex items

percent-height-children-of-alignSelf-stretch-flex-item.html
was taken from a previous attempt at this patch, by harpreet.sk:
https://codereview.chromium.org/331203002

Note that the behavior has changed relative to that patch. This
patch implements the current required behavior by the spec.

BUG=341310

Committed: https://crrev.com/0c12856b59827e462642d3ed8ce2d7042bcf7218
Cr-Commit-Position: refs/heads/master@{#378690}
==========

[commit-bot: I haz the power, 2016-03-02 04:47:08]

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