[css-flexbox] More correctly implement percentage sizing

[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

BUG=341310
R=leviw@chromium.org
CC=dgrogan@chromium.org

Committed: https://crrev.com/0702247984d85e123f0ad2074e69743c710050ad
Cr-Commit-Position: refs/heads/master@{#388308}

[cbiesinger, 2016-04-12 21:21:46]

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

[commit-bot: I haz the power, 2016-04-12 21:22:12]

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

[commit-bot: I haz the power, 2016-04-13 00:26:38]

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

[commit-bot: I haz the power, 2016-04-13 00:26:39]

Dry run: This issue passed the CQ dry run.

[cbiesinger, 2016-04-13 18:39:23]

Description was changed from

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

R=leviw@chromium.org
CC=dgrogan@chromium.org
==========

to

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

R=leviw@chromium.org
CC=dgrogan@chromium.org
==========

[cbiesinger, 2016-04-13 20:00:20]

Description was changed from

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

R=leviw@chromium.org
CC=dgrogan@chromium.org
==========

to

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

BUG=341310
R=leviw@chromium.org
CC=dgrogan@chromium.org
==========

[leviw_travelin_and_unemployed, 2016-04-13 20:10:22]

Is there a bug that tracks this and has more context?

https://codereview.chromium.org/1875263005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1875263005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:793:
dirtyForLayoutFromPercentageHeightDescendants(layoutScope);
Why this change? Is it necessary?

[leviw_travelin_and_unemployed, 2016-04-13 20:10:34]

Whoops, just saw the bug

[cbiesinger, 2016-04-13 20:20:51]

On 2016/04/13 20:10:34, leviw wrote:
> Whoops, just saw the bug

Yeah sorry, I added the bug number late (was in a rush when I sent this out
yesterday and forgot to add it). See also some of the duplicates of that bug,
especially those mentioned in comment 50.

The dirtyForLayoutFromPercentageHeightDescendants call is necessary or
percentage descendants will not be marked for layout. I am fairly sure this was
an existing bug that we just never stumbled upon, that would get triggered e.g.
when a definite height of the flexbox changes. If I remove the call,
css3/flexbox/definite-cross-sizes.html fails. We currently only call it in
LayoutBlockFlow::layoutBlockChildren.

[leviw_travelin_and_unemployed, 2016-04-18 23:19:12]

On 2016/04/13 at 20:20:51, cbiesinger wrote:
> On 2016/04/13 20:10:34, leviw wrote:
> > Whoops, just saw the bug
> 
> Yeah sorry, I added the bug number late (was in a rush when I sent this out
yesterday and forgot to add it). See also some of the duplicates of that bug,
especially those mentioned in comment 50.
> 
> The dirtyForLayoutFromPercentageHeightDescendants call is necessary or
percentage descendants will not be marked for layout. I am fairly sure this was
an existing bug that we just never stumbled upon, that would get triggered e.g.
when a definite height of the flexbox changes. If I remove the call,
css3/flexbox/definite-cross-sizes.html fails. We currently only call it in
LayoutBlockFlow::layoutBlockChildren.

Seems like you should really be doing that in a separate patch.

The HashSet makes me sad, but I get it. LGTM.

[cbiesinger, 2016-04-19 18:45:52]

On 2016/04/18 23:19:12, leviw wrote:
> On 2016/04/13 at 20:20:51, cbiesinger wrote:
> > On 2016/04/13 20:10:34, leviw wrote:
> > > Whoops, just saw the bug
> > 
> > Yeah sorry, I added the bug number late (was in a rush when I sent this out
> yesterday and forgot to add it). See also some of the duplicates of that bug,
> especially those mentioned in comment 50.
> > 
> > The dirtyForLayoutFromPercentageHeightDescendants call is necessary or
> percentage descendants will not be marked for layout. I am fairly sure this
was
> an existing bug that we just never stumbled upon, that would get triggered
e.g.
> when a definite height of the flexbox changes. If I remove the call,
> css3/flexbox/definite-cross-sizes.html fails. We currently only call it in
> LayoutBlockFlow::layoutBlockChildren.
> 
> Seems like you should really be doing that in a separate patch.

I could, but it's necessary to correctly handle this percentage sizing -- is it
ok to submit it with the rest here? I haven't come across a testcase that needs
it *without* this patch.

[leviw_travelin_and_unemployed, 2016-04-19 19:55:25]

Go for it :)

[cbiesinger, 2016-04-19 19:55:53]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2016-04-19 19:56:15]

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

[commit-bot: I haz the power, 2016-04-19 21:12:18]

Description was changed from

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

BUG=341310
R=leviw@chromium.org
CC=dgrogan@chromium.org
==========

to

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

BUG=341310
R=leviw@chromium.org
CC=dgrogan@chromium.org
==========

[commit-bot: I haz the power, 2016-04-19 21:12:19]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-22 19:16:18]

Description was changed from

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

BUG=341310
R=leviw@chromium.org
CC=dgrogan@chromium.org
==========

to

==========
[css-flexbox] More correctly implement percentage sizing

All stretched flex items need to be considered definite per:
https://drafts.csswg.org/css-flexbox/#algo-stretch

This implements that part of the spec as described by
redoing layout of flex items that have percentage children.

BUG=341310
R=leviw@chromium.org
CC=dgrogan@chromium.org

Committed: https://crrev.com/0702247984d85e123f0ad2074e69743c710050ad
Cr-Commit-Position: refs/heads/master@{#388308}
==========

[commit-bot: I haz the power, 2016-04-22 19:16:19]

Patchset 1 (id:??) landed as
https://crrev.com/0702247984d85e123f0ad2074e69743c710050ad
Cr-Commit-Position: refs/heads/master@{#388308}