Drop Image::setContainerSize()

Drop Image::setContainerSize()

Prior to this patch, the only user of Image::setContainerSize() was
HTMLImageElement::getSourceImageForCanvas().

SVGImage, one the relevant kinds of image that return true for
Image::usesContainerSize(), is a shared resource between all places in
the document pointing to the same SVG image. Each time a SVGImage is
drawn, the container size it is drawn relative to is saved. This may
cause subsequent paints of the same SVGImage to re-use the old
container size unless a new one is provided.

The old code addressed just that. When it detected that there was no
layout object attached, it overwrote the old container size with the
size of the image itself, to avoid reusing an old container size.

The new code uses the SVGImageForContainer wrapper to insert the image
size as container size. This closes the loop for using _any_ container
size at all from the Image element. It seems unreasonable that the
layout'ed size should have anything to do with what's drawn to the
canvas anyway.

GeneratedImage had a setContainerSize() implementation too, which is
removed in this patch. It's suspected that this implementation was
unused since a generated image can't be set on a HTMLImageElement
directly.

BUG=563923
Committed: https://crrev.com/9503d96104a3cb851fc4d4fe28c065edc7e17db3
Cr-Commit-Position: refs/heads/master@{#363437}

[davve, 2015-12-01 13:16:15]

Description was changed from

==========
Drop Image::setContainerSize()

Prior to this patch, the only user of Image::setContainerSize() was
HTMLImageElement::getSourceImageForCanvas().

SVGImage, one the relevant kind of image that returns true for
Image::usesContainerSize(), is a shared resource between all places in
the document pointing to the same SVG image. Each time a SVGImage is
drawn, the container size it is drawn relative to saved. This may
cause subsequent paints of the same SVGImage to re-use old container
sizes unless a new one is provided.

The old code did just that. When it detected that there was no layout
object attached, it overwrote the old container size with the size of
the image itself, to avoid reusing an old container size.

The new code uses the SVGImageForContainer wrapper to insert the image
size as container size. The closes the loop for using _any_ container
size at all from the Image element. It seems unreasonble that the
layout'ed size should have anything to do with what's drawn to the
canvas anyway.

GeneratedImage had a setContainerSize() implementation too removed in
this patch. It's suspected that this implementation was unused since a
generated image can be set to a HTMLImageElement directly.

BUG=563923
==========

to

==========
Drop Image::setContainerSize()

Prior to this patch, the only user of Image::setContainerSize() was
HTMLImageElement::getSourceImageForCanvas().

SVGImage, one the relevant kind of image that returns true for
Image::usesContainerSize(), is a shared resource between all places in
the document pointing to the same SVG image. Each time a SVGImage is
drawn, the container size it is drawn relative is saved. This may
cause subsequent paints of the same SVGImage to re-use old container
sizes unless a new one is provided.

The old code addressed just that. When it detected that there was no
layout object attached, it overwrote the old container size with the
size of the image itself, to avoid reusing an old container size.

The new code uses the SVGImageForContainer wrapper to insert the image
size as container size. The closes the loop for using _any_ container
size at all from the Image element. It seems unreasonble that the
layout'ed size should have anything to do with what's drawn to the
canvas anyway.

GeneratedImage had a setContainerSize() implementation too removed in
this patch. It's suspected that this implementation was unused since a
generated image can be set to a HTMLImageElement directly.

BUG=563923
==========

[davve, 2015-12-01 13:23:00]

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

[davve, 2015-12-01 13:23:00]

Supporting fragments properly when drawing SVG images to canvases must be super
important, right? :)

I was a bit bothered by that setContainerSize() in HTMLImageElement when I
stopped returning SVGImageForContainer from ImageResource. This makes
HTMLImageElement::getSourceImageForCanvas join the fold, even though it is a bit
special in the sense that it has no container size. It just uses the image size.

The zoom level is hard-coded to 1. I laborated a bit with supporting zooming
properly but couldn't get it to work. It seems like this doesn't make the matter
worse at least. Zooming gets you pixelated results before and after this patch,
according to my testing.

[davve, 2015-12-01 13:23:08]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-01 13:23:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1489003002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1489003002/1

[fs, 2015-12-01 13:56:07]

Description was changed from

==========
Drop Image::setContainerSize()

Prior to this patch, the only user of Image::setContainerSize() was
HTMLImageElement::getSourceImageForCanvas().

SVGImage, one the relevant kind of image that returns true for
Image::usesContainerSize(), is a shared resource between all places in
the document pointing to the same SVG image. Each time a SVGImage is
drawn, the container size it is drawn relative is saved. This may
cause subsequent paints of the same SVGImage to re-use old container
sizes unless a new one is provided.

The old code addressed just that. When it detected that there was no
layout object attached, it overwrote the old container size with the
size of the image itself, to avoid reusing an old container size.

The new code uses the SVGImageForContainer wrapper to insert the image
size as container size. The closes the loop for using _any_ container
size at all from the Image element. It seems unreasonble that the
layout'ed size should have anything to do with what's drawn to the
canvas anyway.

GeneratedImage had a setContainerSize() implementation too removed in
this patch. It's suspected that this implementation was unused since a
generated image can be set to a HTMLImageElement directly.

BUG=563923
==========

to

==========
Drop Image::setContainerSize()

Prior to this patch, the only user of Image::setContainerSize() was
HTMLImageElement::getSourceImageForCanvas().

SVGImage, one the relevant kinds of image that return true for
Image::usesContainerSize(), is a shared resource between all places in
the document pointing to the same SVG image. Each time a SVGImage is
drawn, the container size it is drawn relative to is saved. This may
cause subsequent paints of the same SVGImage to re-use the old
container size unless a new one is provided.

The old code addressed just that. When it detected that there was no
layout object attached, it overwrote the old container size with the
size of the image itself, to avoid reusing an old container size.

The new code uses the SVGImageForContainer wrapper to insert the image
size as container size. This closes the loop for using _any_ container
size at all from the Image element. It seems unreasonable that the
layout'ed size should have anything to do with what's drawn to the
canvas anyway.

GeneratedImage had a setContainerSize() implementation too, which is
removed in this patch. It's suspected that this implementation was
unused since a generated image can't be set on a HTMLImageElement
directly.

BUG=563923
==========

[fs, 2015-12-01 14:23:21]

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/canvas/canvas-draw-svg-fragments.html
(right):

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/canvas/canvas-draw-svg-fragments.html:8: var
domImage = images.item(i);
Nit: images[i]? (any particular reason to use item(...)?)

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/canvas/canvas-draw-svg-fragments.html:17:
<canvas width="240px" height="240px"></canvas>
Nit: 'px' will be "trailing garbage" in this context.

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:610: sourceImage =
SVGImageForContainer::create(toSVGImage(cachedImage()->image()),
I get a feeling that it would be better to do the wrapping in the calling code.
I think that would mesh better with spec [1] and make things a bit more
scalable. There's also an isSVGImage() check in CRC2D::drawImage that would now
fail (since SVGImageForContainer::isSVGImage returns false IIRC.)

[fs, 2015-12-01 14:30:48]

On 2015/12/01 at 14:23:21, fs wrote:
>
https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/LayoutTe...
> File third_party/WebKit/LayoutTests/svg/canvas/canvas-draw-svg-fragments.html
(right):
> 
>
https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/svg/canvas/canvas-draw-svg-fragments.html:8:
var domImage = images.item(i);
> Nit: images[i]? (any particular reason to use item(...)?)
> 
>
https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/LayoutTe...
> third_party/WebKit/LayoutTests/svg/canvas/canvas-draw-svg-fragments.html:17:
<canvas width="240px" height="240px"></canvas>
> Nit: 'px' will be "trailing garbage" in this context.
> 
>
https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/HTMLImageElement.cpp:610: sourceImage =
SVGImageForContainer::create(toSVGImage(cachedImage()->image()),
> I get a feeling that it would be better to do the wrapping in the calling
code. I think that would mesh better with spec [1] and make things a bit more
scalable. There's also an isSVGImage() check in CRC2D::drawImage that would now
fail (since SVGImageForContainer::isSVGImage returns false IIRC.)

[1] https://html.spec.whatwg.org/multipage/scripting.html#drawing-images

[commit-bot: I haz the power, 2015-12-01 14:35:55]

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

[commit-bot: I haz the power, 2015-12-01 14:35:55]

Dry run: This issue passed the CQ dry run.

[davve, 2015-12-01 14:54:16]

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:610: sourceImage =
SVGImageForContainer::create(toSVGImage(cachedImage()->image()),
On 2015/12/01 14:23:21, fs wrote:
> I get a feeling that it would be better to do the wrapping in the calling
code.
> I think that would mesh better with spec [1] and make things a bit more
> scalable. There's also an isSVGImage() check in CRC2D::drawImage that would
now
> fail (since SVGImageForContainer::isSVGImage returns false IIRC.)

Extracting the url would have to go though the CanvasImageSource interface then,
I guess. A bit more cumbersome, but doable.

I agree about the isSVGImage() thing. I removed
SVGImageForContainer::isSVGImage() because isSVGImage() is used for
DEFINE_IMAGE_TYPE_CASTS and it seemed unsafe to mix in SVGImageForContainer
unless we really need it, which we didn't AFAICT.

[davve, 2015-12-01 14:57:38]

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:610: sourceImage =
SVGImageForContainer::create(toSVGImage(cachedImage()->image()),
On 2015/12/01 14:54:16, David Vest wrote:
> On 2015/12/01 14:23:21, fs wrote:
> > I get a feeling that it would be better to do the wrapping in the calling
> code.
> > I think that would mesh better with spec [1] and make things a bit more
> > scalable. There's also an isSVGImage() check in CRC2D::drawImage that would
> now
> > fail (since SVGImageForContainer::isSVGImage returns false IIRC.)
> 
> Extracting the url would have to go though the CanvasImageSource interface
then,
> I guess. A bit more cumbersome, but doable.

Oh, CanvasImageSource has sourceURL already. I doubt it keeps the fragment part
in the HTMLImageElement implementation though.

[fs, 2015-12-01 14:59:22]

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:610: sourceImage =
SVGImageForContainer::create(toSVGImage(cachedImage()->image()),
On 2015/12/01 at 14:54:16, David Vest wrote:
> On 2015/12/01 14:23:21, fs wrote:
> > I get a feeling that it would be better to do the wrapping in the calling
code.
> > I think that would mesh better with spec [1] and make things a bit more
> > scalable. There's also an isSVGImage() check in CRC2D::drawImage that would
now
> > fail (since SVGImageForContainer::isSVGImage returns false IIRC.)
> 
> Extracting the url would have to go though the CanvasImageSource interface
then, I guess. A bit more cumbersome, but doable.

Cumbersome, yes...

> I agree about the isSVGImage() thing. I removed
SVGImageForContainer::isSVGImage() because isSVGImage() is used for
DEFINE_IMAGE_TYPE_CASTS and it seemed unsafe to mix in SVGImageForContainer
unless we really need it, which we didn't AFAICT.

Yes, there seem to be more good reason for _not_ having it than having it.

[fs, 2015-12-01 15:12:17]

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1489003002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:610: sourceImage =
SVGImageForContainer::create(toSVGImage(cachedImage()->image()),
On 2015/12/01 at 14:57:38, David Vest wrote:
> On 2015/12/01 14:54:16, David Vest wrote:
> > On 2015/12/01 14:23:21, fs wrote:
> > > I get a feeling that it would be better to do the wrapping in the calling
> > code.
> > > I think that would mesh better with spec [1] and make things a bit more
> > > scalable. There's also an isSVGImage() check in CRC2D::drawImage that
would
> > now
> > > fail (since SVGImageForContainer::isSVGImage returns false IIRC.)
> > 
> > Extracting the url would have to go though the CanvasImageSource interface
then,
> > I guess. A bit more cumbersome, but doable.
> 
> Oh, CanvasImageSource has sourceURL already. I doubt it keeps the fragment
part in the HTMLImageElement implementation though.

Considering how it's used I'd suspect it doesn't, no. (And the HTMLVideoElement
impl looks a bit scary in that light...)

[davve, 2015-12-02 15:32:01]

Can't say I'm super happy with how this turned out so far. Comments welcome!

I don't like having to add a method to CanvasImageSource so similar to what's
already there.

The rest might be OK modulo some naming. I have no more time for this today so I
thought I should let you take a look.

[davve, 2015-12-02 15:32:10]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-02 15:33:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1489003002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1489003002/20001

[commit-bot: I haz the power, 2015-12-02 15:57:49]

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

[commit-bot: I haz the power, 2015-12-02 15:57:50]

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

[davve, 2015-12-03 11:20:02]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-03 11:20:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1489003002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1489003002/40001

[davve, 2015-12-03 11:25:47]

Didn't find a good way to do wrapping in CanvasRenderingContext2D. Back to the
initial way, but with the is_expensive test asking the source instead of the
image.

Sizing issues (bug 230163) left for a later CL, but can probably be solved by
passing relevant data into getSourceImageForCanvas().

[fs, 2015-12-03 11:46:17]

lgtm

[davve, 2015-12-03 12:38:37]

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

[davve, 2015-12-03 12:38:37]

Florin, can you take a look at platform/?

[davve, 2015-12-03 12:40:13]

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

[davve, 2015-12-03 12:40:14]

Justin, can you take a look at modules/canvas2d/?

[commit-bot: I haz the power, 2015-12-03 12:45:42]

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

[commit-bot: I haz the power, 2015-12-03 12:45:42]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-12-03 20:12:55]

lgtm

[davve, 2015-12-04 08:05:33]

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

[davve, 2015-12-04 08:05:34]

Stephen, can you review modules/canvas2d?

The reason isSVGImage() can't be used from modules/canvas2d anymore is
that we now use a wrapper object for SVG images to carry extra
information normal images don't need. Normal images are not as
context-sensitive as SVG images.

The wrapper object does not return true for isSVGImage() since
isSVGImage() is used by the DEFINE_IMAGE_TYPE_CASTS macro to do safe
type-casting of SVGImage objects. So that's why we needed a new way of
detecting SVG images.

[Stephen White, 2015-12-04 14:51:38]

LGTM, but Justin might like to have a look.

[davve, 2015-12-04 15:16:27]

On 2015/12/04 14:51:38, Stephen White wrote:
> LGTM, but Justin might like to have a look.

Thanks. I'll give Justin some time before moving this along.

[Justin Novosad, 2015-12-04 15:43:11]

On 2015/12/04 15:16:27, David Vest wrote:
> On 2015/12/04 14:51:38, Stephen White wrote:
> > LGTM, but Justin might like to have a look.
> 
> Thanks. I'll give Justin some time before moving this along.

lgtm

[davve, 2015-12-07 07:30:04]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2015-12-07 07:30:44]

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

[commit-bot: I haz the power, 2015-12-07 10:07:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-07 10:08:23]

Description was changed from

==========
Drop Image::setContainerSize()

Prior to this patch, the only user of Image::setContainerSize() was
HTMLImageElement::getSourceImageForCanvas().

SVGImage, one the relevant kinds of image that return true for
Image::usesContainerSize(), is a shared resource between all places in
the document pointing to the same SVG image. Each time a SVGImage is
drawn, the container size it is drawn relative to is saved. This may
cause subsequent paints of the same SVGImage to re-use the old
container size unless a new one is provided.

The old code addressed just that. When it detected that there was no
layout object attached, it overwrote the old container size with the
size of the image itself, to avoid reusing an old container size.

The new code uses the SVGImageForContainer wrapper to insert the image
size as container size. This closes the loop for using _any_ container
size at all from the Image element. It seems unreasonable that the
layout'ed size should have anything to do with what's drawn to the
canvas anyway.

GeneratedImage had a setContainerSize() implementation too, which is
removed in this patch. It's suspected that this implementation was
unused since a generated image can't be set on a HTMLImageElement
directly.

BUG=563923
==========

to

==========
Drop Image::setContainerSize()

Prior to this patch, the only user of Image::setContainerSize() was
HTMLImageElement::getSourceImageForCanvas().

SVGImage, one the relevant kinds of image that return true for
Image::usesContainerSize(), is a shared resource between all places in
the document pointing to the same SVG image. Each time a SVGImage is
drawn, the container size it is drawn relative to is saved. This may
cause subsequent paints of the same SVGImage to re-use the old
container size unless a new one is provided.

The old code addressed just that. When it detected that there was no
layout object attached, it overwrote the old container size with the
size of the image itself, to avoid reusing an old container size.

The new code uses the SVGImageForContainer wrapper to insert the image
size as container size. This closes the loop for using _any_ container
size at all from the Image element. It seems unreasonable that the
layout'ed size should have anything to do with what's drawn to the
canvas anyway.

GeneratedImage had a setContainerSize() implementation too, which is
removed in this patch. It's suspected that this implementation was
unused since a generated image can't be set on a HTMLImageElement
directly.

BUG=563923

Committed: https://crrev.com/9503d96104a3cb851fc4d4fe28c065edc7e17db3
Cr-Commit-Position: refs/heads/master@{#363437}
==========

[commit-bot: I haz the power, 2015-12-07 10:08:24]

Patchset 3 (id:??) landed as
https://crrev.com/9503d96104a3cb851fc4d4fe28c065edc7e17db3
Cr-Commit-Position: refs/heads/master@{#363437}