Fix SVGImage::imageForCurrentFrameForContainer() sizing

Fix SVGImage::imageForCurrentFrameForContainer() sizing

SVGImageForContainer implements the Image interface, and reports a size
equal to the container size.

But its imageForCurrentFrame() implementation delegates to
SVGImage::imageForCurrentFrameForContainer(), which currently returns an
SkImage with a size equal to the SVG intrinsic size.

This mismatch between the size reported by SVGImageForContainer and the
size of the SkImage returned by its imageForCurrentFrame() is causing
problems when tiling.

The CL updates SVGImage::imageForCurrentFrameForContainer() to return an
SkImage reflecting the container size instead of intrinsic size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969, 633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com

Committed: https://crrev.com/8148eedf1595dab9ec126e3e20403bad53021db9
Cr-Commit-Position: refs/heads/master@{#409853}

[f(malita), 2016-08-02 18:11:55]

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

[commit-bot: I haz the power, 2016-08-02 18:12:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2016-08-02 18:13:32]

Description was changed from

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug/com).

Temporarily disable the fast path, pending an SVGImage fix.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=chrishtr@chromium.org,schenney@chromium.org
==========

to

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug/com).

Temporarily disable the fast path, pending an SVGImage fix.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

[f(malita), 2016-08-02 18:13:32]

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

[f(malita), 2016-08-02 18:14:05]

Description was changed from

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug/com).

Temporarily disable the fast path, pending an SVGImage fix.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

to

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug.com/633637).

Temporarily disable the fast path, pending an SVGImage fix.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

[f(malita), 2016-08-02 18:15:03]

Description was changed from

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug.com/633637).

Temporarily disable the fast path, pending an SVGImage fix.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

to

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug.com/633637).

Temporarily disable the fast path, pending an SVGImage fix.

Also add missing SVGImageForContainer::isSVGImage() forwarder.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

[f(malita), 2016-08-02 18:15:33]

Description was changed from

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug.com/633637).

Temporarily disable the fast path, pending an SVGImage fix.

Also add missing SVGImageForContainer::isSVGImage() forwarder.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

to

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug.com/633637).

Temporarily disable the fast path for SVG images.

Also add missing SVGImageForContainer::isSVGImage() forwarder.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

[f(malita), 2016-08-02 19:04:49]

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

[commit-bot: I haz the power, 2016-08-02 19:05:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2016-08-02 19:17:24]

Expected new test result:
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

I also have a proper fix for the underlying SVGImage issue in the works, but
since the bug is a stable-release-blocker it seems more prudent to merge
something like this.

[Stephen Chennney, 2016-08-02 19:32:10]

LGTM

[fs, 2016-08-02 19:47:36]

https://codereview.chromium.org/2203093003/diff/2/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h (right):

https://codereview.chromium.org/2203093003/diff/2/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:50: bool
isSVGImage() const override { return m_image->isSVGImage(); }
This is used to determine if an Image can be casted to an SVGImage - which is
not the case here. So this is hazardous at best (I don't _think_ there's
currently any such users of the ...Container type but...)

[commit-bot: I haz the power, 2016-08-02 20:33:29]

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

[commit-bot: I haz the power, 2016-08-02 20:33:30]

Dry run: This issue passed the CQ dry run.

[f(malita), 2016-08-02 20:53:11]

https://codereview.chromium.org/2203093003/diff/2/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h (right):

https://codereview.chromium.org/2203093003/diff/2/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:50: bool
isSVGImage() const override { return m_image->isSVGImage(); }
On 2016/08/02 19:47:35, fs (OoO until Aug 15) wrote:
> This is used to determine if an Image can be casted to an SVGImage - which is
> not the case here. So this is hazardous at best (I don't _think_ there's
> currently any such users of the ...Container type but...)

Argh.  Yeah, probably not a good idea to play with that.

We could use usesContainerSize() & hasRelativeSize() which *are* forwarded by
SVGImageForContainer, and provide a unique signature for SVGImage AFAICT.  But
ugh.

Let me see how intrusive is the real fix, maybe it's mergeable and we can avoid
this wacky workaround.  I'll be back.

[Stephen Chennney, 2016-08-02 20:54:47]

On 2016/08/02 20:53:11, f(malita) wrote:
>
https://codereview.chromium.org/2203093003/diff/2/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h
(right):
> 
>
https://codereview.chromium.org/2203093003/diff/2/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:50: bool
> isSVGImage() const override { return m_image->isSVGImage(); }
> On 2016/08/02 19:47:35, fs (OoO until Aug 15) wrote:
> > This is used to determine if an Image can be casted to an SVGImage - which
is
> > not the case here. So this is hazardous at best (I don't _think_ there's
> > currently any such users of the ...Container type but...)
> 
> Argh.  Yeah, probably not a good idea to play with that.
> 
> We could use usesContainerSize() & hasRelativeSize() which *are* forwarded by
> SVGImageForContainer, and provide a unique signature for SVGImage AFAICT.  But
> ugh.
> 
> Let me see how intrusive is the real fix, maybe it's mergeable and we can
avoid
> this wacky workaround.  I'll be back.

not lgtm to make sure we don't accidentally land this after my bad review.

[f(malita), 2016-08-02 21:32:29]

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

[commit-bot: I haz the power, 2016-08-02 21:33:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2016-08-02 21:41:56]

Description was changed from

==========
Temporarily disable rrect background fast path for SVG images

http://crrev.com/1949253004 introduced a shader-based fast path for
rounded-rect background images.  That exposed an issue with
SVGImage::imageForCurrentFrameForContainer (the result SkImage size
ignores the container size - http://crbug.com/633637).

Temporarily disable the fast path for SVG images.

Also add missing SVGImageForContainer::isSVGImage() forwarder.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

to

==========
Fix SVGImage::imageForCurrentFrameForContainer() sizing

The SkImage returned from imageForCurrentFrameForContainer() needs to
reflect the container size, not the intrinsic SVG image size.

This ensures correct resolution and tiling when the container size is
not the same as the SVG image size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969,633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

[commit-bot: I haz the power, 2016-08-02 22:56:44]

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

[commit-bot: I haz the power, 2016-08-02 22:56:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[f(malita), 2016-08-03 17:58:38]

Description was changed from

==========
Fix SVGImage::imageForCurrentFrameForContainer() sizing

The SkImage returned from imageForCurrentFrameForContainer() needs to
reflect the container size, not the intrinsic SVG image size.

This ensures correct resolution and tiling when the container size is
not the same as the SVG image size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969,633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

to

==========
Fix SVGImage::imageForCurrentFrameForContainer() sizing

SVGImageForContainer implements the Image interface, and reports a size
equal to the container size.

But its imageForCurrentFrame() implementation delegates to
SVGImage::imageForCurrentFrameForContainer(), which currently returns an
SkImage with a size equal to the SVG intrinsic size.

This mismatch between the size reported by SVGImageForContainer and the
size of the SkImage returned by its imageForCurrentFrame() is causing
problems when tiling.

The CL updates SVGImage::imageForCurrentFrameForContainer() to return an
SkImage reflecting the container size instead of intrinsic size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969,633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

[f(malita), 2016-08-03 17:58:38]

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

[f(malita), 2016-08-03 18:08:57]

On 2016/08/02 20:53:11, f(malita) wrote:
> Let me see how intrusive is the real fix, maybe it's mergeable and we can
avoid
> this wacky workaround.  I'll be back.

+davve

So PS#4 is the real fix IMO: updates
SVGImage::imageForCurrentFrameForContainer() to return an SkImage sized to the
container size instead of intrinsic size.

I think this makes sense, since SVGImageForContainer is claiming its size is ==
container size, so it's odd for its imageForCurrentFrameForContainer() to return
something different.

But it does cause a difference in canvas-default-object-sizing.html:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

I happen to think that the new result makes more sense, but it's a handcrafted
reftest, so we should double-check:

- before:

  * container size: [100x50]
  * SVG intrinsic size: [300x150]
  * => SkImage tile: [300x150]
  * => when filling the 100x100 pattern we only draw one tile

- with this change:

  * container size: [100x50]
  * SVG intrinsic size: [300x150]
  * => SkImage tile: [100x50]
  * => when filling the 100x100 pattern we draw two tiles vertically

The actual/visual SVG content is scaled down based on the container size anyway,
so it makes sense to me to also resize the tile similarly.

WDYT?

[fs, 2016-08-04 00:06:47]

On 2016/08/03 at 18:08:57, fmalita wrote:
> On 2016/08/02 20:53:11, f(malita) wrote:
> > Let me see how intrusive is the real fix, maybe it's mergeable and we can
avoid
> > this wacky workaround.  I'll be back.
> 
> +davve
> 
> So PS#4 is the real fix IMO: updates
SVGImage::imageForCurrentFrameForContainer() to return an SkImage sized to the
container size instead of intrinsic size.
> 
> I think this makes sense, since SVGImageForContainer is claiming its size is
== container size, so it's odd for its imageForCurrentFrameForContainer() to
return something different.
> 
> But it does cause a difference in canvas-default-object-sizing.html:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
> 
> I happen to think that the new result makes more sense, but it's a handcrafted
reftest, so we should double-check:
> 
> - before:
> 
>   * container size: [100x50]
>   * SVG intrinsic size: [300x150]
>   * => SkImage tile: [300x150]
>   * => when filling the 100x100 pattern we only draw one tile
> 
> - with this change:
> 
>   * container size: [100x50]
>   * SVG intrinsic size: [300x150]
>   * => SkImage tile: [100x50]
>   * => when filling the 100x100 pattern we draw two tiles vertically

I did a few rounds in my head with "default object size" (100x100 here IIRC),
and "concrete object size" (==container size) would be 100x50, and I don't see
why a replaced content default intrinsic size (300x150) would come into play
here. So the "after" rendering looks correct - presumably just the same bug (and
I confess that reviewing all those circles was a bit of a mindtwister...) I'd
welcome other input though. Also, change LGTM.

> The actual/visual SVG content is scaled down based on the container size
anyway, so it makes sense to me to also resize the tile similarly.

Indeed.

[f(malita), 2016-08-04 16:36:40]

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

[commit-bot: I haz the power, 2016-08-04 16:37:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 16:54:16]

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

[commit-bot: I haz the power, 2016-08-04 16:54:17]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[f(malita), 2016-08-04 18:16:30]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2016-08-04 18:16:30]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/2203093003/#ps70001
(title: "updated canvas-default-object-sizing ref")

[commit-bot: I haz the power, 2016-08-04 18:17:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 18:17:41]

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

[commit-bot: I haz the power, 2016-08-04 18:17:42]

A disapproval has been posted by following reviewers: schenney@chromium.org.
Please make sure to get positive review before another CQ attempt.

[Stephen Chennney, 2016-08-04 18:19:26]

On 2016/08/04 00:06:47, fs (OoO until Aug 15) wrote:
> On 2016/08/03 at 18:08:57, fmalita wrote:
> > On 2016/08/02 20:53:11, f(malita) wrote:
> > > Let me see how intrusive is the real fix, maybe it's mergeable and we can
> avoid
> > > this wacky workaround.  I'll be back.
> > 
> > +davve
> > 
> > So PS#4 is the real fix IMO: updates
> SVGImage::imageForCurrentFrameForContainer() to return an SkImage sized to the
> container size instead of intrinsic size.
> > 
> > I think this makes sense, since SVGImageForContainer is claiming its size is
> == container size, so it's odd for its imageForCurrentFrameForContainer() to
> return something different.
> > 
> > But it does cause a difference in canvas-default-object-sizing.html:
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
> > 
> > I happen to think that the new result makes more sense, but it's a
handcrafted
> reftest, so we should double-check:
> > 
> > - before:
> > 
> >   * container size: [100x50]
> >   * SVG intrinsic size: [300x150]
> >   * => SkImage tile: [300x150]
> >   * => when filling the 100x100 pattern we only draw one tile
> > 
> > - with this change:
> > 
> >   * container size: [100x50]
> >   * SVG intrinsic size: [300x150]
> >   * => SkImage tile: [100x50]
> >   * => when filling the 100x100 pattern we draw two tiles vertically
> 
> I did a few rounds in my head with "default object size" (100x100 here IIRC),
> and "concrete object size" (==container size) would be 100x50, and I don't see
> why a replaced content default intrinsic size (300x150) would come into play
> here. So the "after" rendering looks correct - presumably just the same bug
(and
> I confess that reviewing all those circles was a bit of a mindtwister...) I'd
> welcome other input though. Also, change LGTM.
> 
> > The actual/visual SVG content is scaled down based on the container size
> anyway, so it makes sense to me to also resize the tile similarly.
> 
> Indeed.

LGTM to revert previous not.

[f(malita), 2016-08-04 18:19:59]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2016-08-04 18:20:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 19:00:06]

Description was changed from

==========
Fix SVGImage::imageForCurrentFrameForContainer() sizing

SVGImageForContainer implements the Image interface, and reports a size
equal to the container size.

But its imageForCurrentFrame() implementation delegates to
SVGImage::imageForCurrentFrameForContainer(), which currently returns an
SkImage with a size equal to the SVG intrinsic size.

This mismatch between the size reported by SVGImageForContainer and the
size of the SkImage returned by its imageForCurrentFrame() is causing
problems when tiling.

The CL updates SVGImage::imageForCurrentFrameForContainer() to return an
SkImage reflecting the container size instead of intrinsic size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969,633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

to

==========
Fix SVGImage::imageForCurrentFrameForContainer() sizing

SVGImageForContainer implements the Image interface, and reports a size
equal to the container size.

But its imageForCurrentFrame() implementation delegates to
SVGImage::imageForCurrentFrameForContainer(), which currently returns an
SkImage with a size equal to the SVG intrinsic size.

This mismatch between the size reported by SVGImageForContainer and the
size of the SkImage returned by its imageForCurrentFrame() is causing
problems when tiling.

The CL updates SVGImage::imageForCurrentFrameForContainer() to return an
SkImage reflecting the container size instead of intrinsic size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969,633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

[commit-bot: I haz the power, 2016-08-04 19:00:08]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-08-04 19:01:59]

Description was changed from

==========
Fix SVGImage::imageForCurrentFrameForContainer() sizing

SVGImageForContainer implements the Image interface, and reports a size
equal to the container size.

But its imageForCurrentFrame() implementation delegates to
SVGImage::imageForCurrentFrameForContainer(), which currently returns an
SkImage with a size equal to the SVG intrinsic size.

This mismatch between the size reported by SVGImageForContainer and the
size of the SkImage returned by its imageForCurrentFrame() is causing
problems when tiling.

The CL updates SVGImage::imageForCurrentFrameForContainer() to return an
SkImage reflecting the container size instead of intrinsic size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969,633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com
==========

to

==========
Fix SVGImage::imageForCurrentFrameForContainer() sizing

SVGImageForContainer implements the Image interface, and reports a size
equal to the container size.

But its imageForCurrentFrame() implementation delegates to
SVGImage::imageForCurrentFrameForContainer(), which currently returns an
SkImage with a size equal to the SVG intrinsic size.

This mismatch between the size reported by SVGImageForContainer and the
size of the SkImage returned by its imageForCurrentFrame() is causing
problems when tiling.

The CL updates SVGImage::imageForCurrentFrameForContainer() to return an
SkImage reflecting the container size instead of intrinsic size.

New pixel test: fast/backgrounds/background-svg-scaling.html

BUG=632969,633637
R=schenney@chromium.org, pdr@chromium.org, fs@opera.com

Committed: https://crrev.com/8148eedf1595dab9ec126e3e20403bad53021db9
Cr-Commit-Position: refs/heads/master@{#409853}
==========

[commit-bot: I haz the power, 2016-08-04 19:02:00]

Patchset 5 (id:??) landed as
https://crrev.com/8148eedf1595dab9ec126e3e20403bad53021db9
Cr-Commit-Position: refs/heads/master@{#409853}