Use correct viewport for intrinsic size-less SVG images in <svg:image>

Use correct viewport for intrinsic size-less SVG images in <svg:image>

Prior to https://codereview.chromium.org/403393002, an SVG image which had
no intrinsic size specified, would (somewhat accidentally) get the
<image>'s bounds set as their container size. After said CL, the container
size would never be set, and hence remain as 300x150. This will result in
incorrect scale factors being used when painting, and hence an incorrectly
scaled result.
Revert to setting the <image>'s bounds when the SVG image does not have an
intrinsic size.

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

[fs, 2014-09-09 12:50:29]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org

[fs, 2014-09-09 13:00:15]

The interacts here are a bit hairy (hairier than I initially thought...). Not
sure this makes it less so =(

[pdr., 2014-09-10 03:21:48]

I don't think this is too bad. We have a little time; how do you feel about
landing this approach, letting it sit in trunk for a week, then merging into the
release branches?

https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGImage.cpp (right):

https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGImage.cpp:75: if
(!floatValueForLength(intrinsicWidth, 0) ||
!floatValueForLength(intrinsicHeight, 0))
Are these checks correct when the user does want a width of 0? Do we need them
in SVGSVGElement::hasIntrinsicWidth too?

https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGImage.cpp:97: if
(image->preserveAspectRatio()->currentValue()->align() ==
SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE
The code change here looks good but I think we can simplify the logic a bit.

What about something like:
if (forceNonUniformScaling(image)) {
    // etc
} else if (boundsChanged) {
...

forceNonUniformScaling could then encapsulate this somewhat tricky logic and the
comment above this.

[pdr., 2014-09-10 03:53:56]

Forgot to mention... I tested this in FF and IE and both seem to agree with your
patch, so I'm less worried than usual about compat issues.

[fs, 2014-09-10 09:31:00]

>I don't think this is too bad. We have a little time; how do you feel about
>landing this approach, letting it sit in trunk for a week, then merging into
the
>release branches?

sgtm

https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGImage.cpp (right):

https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGImage.cpp:75: if
(!floatValueForLength(intrinsicWidth, 0) ||
!floatValueForLength(intrinsicHeight, 0))
On 2014/09/10 03:21:48, pdr wrote:
> Are these checks correct when the user does want a width of 0? Do we need them
> in SVGSVGElement::hasIntrinsicWidth too?

This could be considered a "counter-weight" to how SVGImage computes its size -
in this case you'll get the fallback intrinsic size of 300x150. I added an
additional test for a case like that.

In a slightly wider scope of things, the intrinsic size of the referenced image
doesn't really matter, because width/height/etc. on the <image> ends up
overriding whatever's on the the image root [1]. I.e. embedder really decides in
these cases. But since what we're really doing here is matching the impedance of
(SVG)Image and RenderSVGImage, the former ("avoid 300x150") is what we are
interested in here [2].

[1] "When an ‘image’ element references an SVG image, the ‘clip’ and ‘overflow’
properties on the root element in the referenced SVG image are ignored (in the
same manner as the ‘x’, ‘y’, ‘width’ and ‘height’ attributes are ignored).",
http://dev.w3.org/SVG/profiles/1.1F2/publish/struct.html#ImageElement fourth
paragraph
[2] Yes, I think this is hairy.

https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGImage.cpp:97: if
(image->preserveAspectRatio()->currentValue()->align() ==
SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE
On 2014/09/10 03:21:48, pdr wrote:
> The code change here looks good but I think we can simplify the logic a bit.
> 
> What about something like:
> if (forceNonUniformScaling(image)) {
>     // etc
> } else if (boundsChanged) {
> ...
> 
> forceNonUniformScaling could then encapsulate this somewhat tricky logic and
the
> comment above this.

Certainly looks better. Done.

[pdr., 2014-09-10 18:36:25]

On 2014/09/10 09:31:00, fs wrote:
> >I don't think this is too bad. We have a little time; how do you feel about
> >landing this approach, letting it sit in trunk for a week, then merging into
> the
> >release branches?
> 
> sgtm
> 
>
https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
> File Source/core/rendering/svg/RenderSVGImage.cpp (right):
> 
>
https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGImage.cpp:75: if
> (!floatValueForLength(intrinsicWidth, 0) ||
> !floatValueForLength(intrinsicHeight, 0))
> On 2014/09/10 03:21:48, pdr wrote:
> > Are these checks correct when the user does want a width of 0? Do we need
them
> > in SVGSVGElement::hasIntrinsicWidth too?
> 
> This could be considered a "counter-weight" to how SVGImage computes its size
-
> in this case you'll get the fallback intrinsic size of 300x150. I added an
> additional test for a case like that.
> 
> In a slightly wider scope of things, the intrinsic size of the referenced
image
> doesn't really matter, because width/height/etc. on the <image> ends up
> overriding whatever's on the the image root [1]. I.e. embedder really decides
in
> these cases. But since what we're really doing here is matching the impedance
of
> (SVG)Image and RenderSVGImage, the former ("avoid 300x150") is what we are
> interested in here [2].
> 
> [1] "When an ‘image’ element references an SVG image, the ‘clip’ and
‘overflow’
> properties on the root element in the referenced SVG image are ignored (in the
> same manner as the ‘x’, ‘y’, ‘width’ and ‘height’ attributes are ignored).",
> http://dev.w3.org/SVG/profiles/1.1F2/publish/struct.html#ImageElement fourth
> paragraph
> [2] Yes, I think this is hairy.
> 
>
https://codereview.chromium.org/551043006/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGImage.cpp:97: if
> (image->preserveAspectRatio()->currentValue()->align() ==
> SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE
> On 2014/09/10 03:21:48, pdr wrote:
> > The code change here looks good but I think we can simplify the logic a bit.
> > 
> > What about something like:
> > if (forceNonUniformScaling(image)) {
> >     // etc
> > } else if (boundsChanged) {
> > ...
> > 
> > forceNonUniformScaling could then encapsulate this somewhat tricky logic and
> the
> > comment above this.
> 
> Certainly looks better. Done.

LGTM

[pdr., 2014-09-10 18:36:30]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-09-10 18:37:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fs@opera.com/551043006/20001

[commit-bot: I haz the power, 2014-09-10 18:41:31]

Committed patchset #2 (id:20001) as 181752

[fs, 2014-09-22 10:56:04]

On 2014/09/10 18:36:25, pdr wrote:
> On 2014/09/10 09:31:00, fs wrote:
> > >I don't think this is too bad. We have a little time; how do you feel about
> > >landing this approach, letting it sit in trunk for a week, then merging
into
> > the
> > >release branches?
> > 
> > sgtm
> > 

Time to execute?

[pdr., 2014-09-22 20:38:24]

On 2014/09/22 10:56:04, fs wrote:
> On 2014/09/10 18:36:25, pdr wrote:
> > On 2014/09/10 09:31:00, fs wrote:
> > > >I don't think this is too bad. We have a little time; how do you feel
about
> > > >landing this approach, letting it sit in trunk for a week, then merging
> into
> > > the
> > > >release branches?
> > > 
> > > sgtm
> > > 
> 
> Time to execute?

+1! Thank you for seeing this all the way through. I will add the appropriate
merge request label. We will need to wait for one of the release managers to
approve this; they will add a comment on the bug once everything is ready to go.