Ensure logical height is overridden for stretch-aligned flex-box children

Ensure logical height is overridden for stretch-aligned flex-box children

With an SVG in a flex-box context (as a flex-item), it's possible for a
document-wide layout to clear the override-height, and then issue a
partial (<svg> as layout-root) layout that recomputes the logical height
to whatever the <svg> root thinks it should be - disregarding the flex-box
context.
To fix this, make sure that a stretch-aligned flex-item always has an
override-height set, even if the logical height appear to be up-to-date.

BUG=397085
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183293

[fs, 2014-07-31 14:29:56]

This feels a bit ugly, so likely there's a better way to achieve the same
effect... hopefully it illustrates what the problem is though...

[cbiesinger, 2014-08-06 02:57:45]

Sorry, can you clarify which code clears the override height? I didn't think we
normally did that, but I guess I'm wrong - maybe you can point me to the code?
Thanks!

[fs, 2014-08-21 16:08:19]

On 2014/08/06 02:57:45, cbiesinger wrote:
> Sorry, can you clarify which code clears the override height? I didn't think
we
> normally did that, but I guess I'm wrong - maybe you can point me to the code?
> Thanks!

Sorry for the delay (vacation, only checking this sporadically...). The clear is
in preferredMainAxisContentExtentForChild - unfortunately I don't remember the
exact/relevant caller (resolveFlexibleLengths would be my guess).

[cbiesinger, 2014-08-21 21:32:17]

lgtm

Ojan, FYI, here's a case where a flex item is a layout root.

[cbiesinger, 2014-08-29 23:28:59]

Note that https://codereview.chromium.org/331203002/ will also solve this bug,
though the testcase is still good to get checked in.

[fs, 2014-09-01 17:59:51]

On 2014/08/29 23:28:59, cbiesinger wrote:
> Note that https://codereview.chromium.org/331203002/ will also solve this bug,
> though the testcase is still good to get checked in.

Thanks for the heads-up. If you think that'll land soonish I can sit on this
until then (and then land the TC).

[fs, 2014-10-06 16:37:50]

On 2014/09/01 17:59:51, fs wrote:
> On 2014/08/29 23:28:59, cbiesinger wrote:
> > Note that https://codereview.chromium.org/331203002/ will also solve this
bug,
> > though the testcase is still good to get checked in.
> 
> Thanks for the heads-up. If you think that'll land soonish I can sit on this
> until then (and then land the TC).

Still on the table... =) (And does appear to work with the TC in issue 418591.)

[cbiesinger, 2014-10-06 17:08:52]

On 2014/10/06 16:37:50, fs wrote:
> On 2014/09/01 17:59:51, fs wrote:
> > On 2014/08/29 23:28:59, cbiesinger wrote:
> > > Note that https://codereview.chromium.org/331203002/ will also solve this
> bug,
> > > though the testcase is still good to get checked in.
> > 
> > Thanks for the heads-up. If you think that'll land soonish I can sit on this
> > until then (and then land the TC).
> 
> Still on the table... =) (And does appear to work with the TC in issue
418591.)

Yeah, fair enough, let's get this checked in :)

(you'll still need an owner lgtm)

[ojan, 2014-10-06 18:08:02]

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

[ojan, 2014-10-06 18:08:02]

The CQ bit was checked by ojan@chromium.org

[ojan, 2014-10-06 18:08:02]

lgtm

[commit-bot: I haz the power, 2014-10-06 18:09:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/423133002/20001

[commit-bot: I haz the power, 2014-10-07 01:04:06]

Committed patchset #2 (id:20001) as 183293