Use viewBox for SVG image as intrinsic size when all else fails

Use viewBox for SVG as intrinsic size when all else fails

In http://crrev.com/308643002 the use of viewBox for intrinsicSize was
removed (which means this is a partial revert of that change). The
reason for the removal was that there wasn't a known cases where it
would be useful.

But then crbug.com/441862 surfaced and showed that using the viewBox
for intrinsic size can indeed be useful and necessary for SVG inside
an image through a relatively complex chain. What in the end happens
is that the intrinsic size is used not as a size but as a ratio when
doing scaling to accommodate for object-fit:cover (see
LayoutReplaced::replacedContentRect).

Ideally, we should only allow use of the ratio from the viewBox, but
we currently have no way of expressing just the ratio for an Image. So
there is potential for the viewBox being misused as size. Putting the
code back should be relatively safe though, since we've had it before.

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

[davve, 2015-03-19 12:20:24]

davve@opera.com changed reviewers:
+ pdr@chromium.org

[davve, 2015-03-19 12:20:25]

So now I got to know at least one case where the code I removed in
http://crrev.com/308643002 mattered.

The causality chain from SVGImage::containerSize(), where we extra viewBox as
intrinsic size, to LayoutReplaced::replacedContentRect where it's used as an
intrinsic ratio, is not trivial, and I would sure prefer another way to
propagate the ratio. But there are too many unfamiliar layers in the way for me
to suggest something better.

[pdr., 2015-03-20 06:23:01]

On 2015/03/19 at 12:20:25, davve wrote:
> So now I got to know at least one case where the code I removed in
http://crrev.com/308643002 mattered.
> 
> The causality chain from SVGImage::containerSize(), where we extra viewBox as
intrinsic size, to LayoutReplaced::replacedContentRect where it's used as an
intrinsic ratio, is not trivial, and I would sure prefer another way to
propagate the ratio. But there are too many unfamiliar layers in the way for me
to suggest something better.

I agree with your conclusion--this is some gnarly code and a simple solution
seems appropriate.

LGTM

[davve, 2015-03-24 12:42:38]

davve@opera.com changed reviewers:
+ fs@opera.com

[davve, 2015-03-24 12:48:00]

On 2015/03/20 06:23:01, pdr wrote:
> On 2015/03/19 at 12:20:25, davve wrote:
> > So now I got to know at least one case where the code I removed in
> http://crrev.com/308643002 mattered.
> > 
> > The causality chain from SVGImage::containerSize(), where we extra viewBox
as
> intrinsic size, to LayoutReplaced::replacedContentRect where it's used as an
> intrinsic ratio, is not trivial, and I would sure prefer another way to
> propagate the ratio. But there are too many unfamiliar layers in the way for
me
> to suggest something better.
> 
> I agree with your conclusion--this is some gnarly code and a simple solution
> seems appropriate.

Perhaps, but I worry a little about the test failure where the viewBox leaks as
size of the image.

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

So we're trading one bug for another. Anyone having an idea how to fix this
properly? Is extend Image to support the scenario of no intrinsic size but
intrinsic ratio worth it?

[fs, 2015-03-24 13:56:45]

On 2015/03/24 12:48:00, David Vest wrote:
> On 2015/03/20 06:23:01, pdr wrote:
> > On 2015/03/19 at 12:20:25, davve wrote:
> > > So now I got to know at least one case where the code I removed in
> > http://crrev.com/308643002 mattered.
> > > 
> > > The causality chain from SVGImage::containerSize(), where we extra viewBox
> as
> > intrinsic size, to LayoutReplaced::replacedContentRect where it's used as an
> > intrinsic ratio, is not trivial, and I would sure prefer another way to
> > propagate the ratio. But there are too many unfamiliar layers in the way for
> me
> > to suggest something better.
> > 
> > I agree with your conclusion--this is some gnarly code and a simple solution
> > seems appropriate.
> 
> Perhaps, but I worry a little about the test failure where the viewBox leaks
as
> size of the image.
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> 
> So we're trading one bug for another.

Well, at least it's a fail->fail transition (aka "no transition")...
(crbug.com/396955) is still open due to cases like the one you're seeing.

I'm not sure if the "300x150" behavior here is important enough to cling to
(although it'll obviously hint at the actual result slightly better...)

> Anyone having an idea how to fix this
> properly? Is extend Image to support the scenario of no intrinsic size but
> intrinsic ratio worth it?

Image has hasRelativeWidth/Height - which are not implemented for SVGImage (only
GeneratedImage return true in them I think). I've no idea what they are supposed
to convey =), but they sure sound like they could be for something like this...
Feels like a dusty road to walk. It also sounds like fixing this is key to
actually fixing naturalWidth/Height for cases like this.

[davve, 2015-03-25 08:31:16]

On 2015/03/24 13:56:45, fs wrote:
> On 2015/03/24 12:48:00, David Vest wrote:
> > Anyone having an idea how to fix this
> > properly? Is extend Image to support the scenario of no intrinsic size but
> > intrinsic ratio worth it?
> 
> Image has hasRelativeWidth/Height - which are not implemented for SVGImage
(only
> GeneratedImage return true in them I think). I've no idea what they are
supposed
> to convey =), but they sure sound like they could be for something like
this...
> Feels like a dusty road to walk. It also sounds like fixing this is key to
> actually fixing naturalWidth/Height for cases like this.

hasRelativeWidth/Height might deserve to be investigated. Agree on the dusty
road feeling though. :)

Will read up on naturalWidth/Height, those are new to me.

[davve, 2015-04-02 10:58:58]

PTAL. No code changes, only added TODOs and updated the FAIL -> FAIL test
expectation.

I hope to follow this up in the future but putting the fallback back together
with some TODO is probably the best course of action at the moment.

[fs, 2015-04-02 11:03:37]

lgtm

[davve, 2015-04-02 11:04:27]

The CQ bit was checked by davve@opera.com

[davve, 2015-04-02 11:04:27]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1019813002/#ps40001
(title: "Updated test expectations and added TODOs")

[commit-bot: I haz the power, 2015-04-02 11:04:46]

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

[commit-bot: I haz the power, 2015-04-02 12:17:47]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193030