Use the viewBox when scaling <svg:image>s non-uniformly w/ pAR=none

Use the viewBox when scaling <svg:image>s non-uniformly w/ pAR=none

When referencing an SVG image from <svg:image> with pAR set to 'none', and
said image had no intrinsic size, but a 'viewBox', the use of the intrinsic
size (which would be computed as 300x150) as the container size would
incorrect. The 'viewBox' would resolve against 300x150, so if it defined
a different aspect ratio than that, the image would not appear correctly
scaled.

For pAR=none there're a number of cases to consider:
  1) The referenced image has a 'viewBox'.
  2) The referenced image has intrinsic dimensions.
  3) The referenced image has none of the above.
For cases (1) and (2), we should use the 'viewBox' and intrinsic dimensions
(respectively) to define the container size (~= the image's viewport maps
to the viewport defined by <svg:image>). In case (3) we try to use whatever
is left to use (300x150). (In practice cases (2) and (3) should be
equivalent.)

This also recognizes that the TC
svg/custom/svg-image-par-none-zero-intrinsic-size.html
is incorrect, and modifies it. Gecko seem to agree with this change.

The TC from the bug also made it obvious that there was a repaint bug at
play, which resulted in the updated return value from
RenderSVGImage::updateImageViewport. This is covered by the existing TC
svg/custom/svg-image-par-resize.html, which now seem to issue more correct
repaint rectangles.

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

[fs, 2014-11-11 14:32:45]

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

[fs, 2014-11-11 14:32:46]

Geez, who would've thought that pAR=none would such a complicated thing to get
right... [1]

Now with new "fresh" approach for handling certain cases of viewBox'd images.

[1] N.B. it still has bugs, but hopefully no more regressions *knocks on wood*

[pdr., 2014-11-12 00:48:25]

Our old friend PAR :(

I think this looks good, mostly minor points below:

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGImage.cpp (right):

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:66: // Note: This assumes that the
'intrinsic ratio' as returned by
Could we document this on the Image interface instead of here?

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:79: return
m_imageResource->intrinsicSize(style()->effectiveZoom());
It looks like this will end up just returning an empty rect in
RenderImageResource::getImageSize so we could simplify this as:
if (ImageResource* cachedImage = ...) {
  ...
  return roundedLayoutSize(intrinsicRatio);
}
return empty rect;

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:98: if
(image->preserveAspectRatio()->currentValue()->align() ==
SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE) {
Could we move the PAR_NONE check and the viewport size comparison into
computeNonUniformScalingViewportSize?

Something like:
if (computeNonUnifornScalingViewportSize(newViewportSize)) {
    updatedViewport = true;
} else {
    ....

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:111: return updatedViewport ||
boundsChanged;
Does it make sense for the bounds to change but the viewport not update?

[fs, 2014-11-12 10:13:45]

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGImage.cpp (right):

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:66: // Note: This assumes that the
'intrinsic ratio' as returned by
On 2014/11/12 00:48:25, pdr wrote:
> Could we document this on the Image interface instead of here?

Done.

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:79: return
m_imageResource->intrinsicSize(style()->effectiveZoom());
On 2014/11/12 00:48:25, pdr wrote:
> It looks like this will end up just returning an empty rect in
> RenderImageResource::getImageSize so we could simplify this as:
> if (ImageResource* cachedImage = ...) {
>   ...
>   return roundedLayoutSize(intrinsicRatio);
> }
> return empty rect;

Yes, true. Done.

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:98: if
(image->preserveAspectRatio()->currentValue()->align() ==
SVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE) {
On 2014/11/12 00:48:25, pdr wrote:
> Could we move the PAR_NONE check and the viewport size comparison into
> computeNonUniformScalingViewportSize?
> 
> Something like:
> if (computeNonUnifornScalingViewportSize(newViewportSize)) {
>     updatedViewport = true;
> } else {
>     ....

While it's possible to do that, you then get essentially three values to store
in the bool - pAR!=none, viewport size unchanged, update viewport.
To retain the "correct" flow (and set the right viewport in all cases) you'd
need to sacrifice one of them (the middle one). While that's possible, it also
mean we'll be resetting the container size every time. Probably not a big deal
(at least not ATM), but updating the container size too much has bit us before
in these cases (i.e. the initial bug that started this farce...), so I'd rather
tread lightly here (if possible)...

Tangentially, it might make sense to structure the code like:

LayoutSize viewportSize = computeViewportSize(...);
if (viewportSize != m_imageResource->imageSize(...))
  m_imageResource->setContainerSize(roundedIntSize(viewportSize));
m_needs... |= boundsChanged;
return boundsChanged;

i.e. implicitly signal the changes in the 'viewport' / bounds.

https://codereview.chromium.org/713263002/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGImage.cpp:111: return updatedViewport ||
boundsChanged;
On 2014/11/12 00:48:25, pdr wrote:
> Does it make sense for the bounds to change but the viewport not update?

Yes, this would even be expected in the pAR=none case (where the viewport
depends on the content rather then the container). As I alluded to above though,
this could probably just be 'return boundsChanged', because 'updatedViewport'
implies 'boundsChanged' - at least in all context where the return-value is
checked (and likely in the other two as well.)

[fs, 2014-11-12 13:47:18]

Uploaded PS3 and PS4, where the latter is the more radical approach described in
comments. Both seem to test fine.

[pdr., 2014-11-12 17:39:29]

PS4 looks great, LGTM

[pdr., 2014-11-12 17:39:33]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-11-12 17:40:40]

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

[commit-bot: I haz the power, 2014-11-12 17:43:10]

Committed patchset #4 (id:60001) as 185226