Add Image::updateConcreteSize()

Add Image::updateConcreteSize()

For SVG and generated images using the default sizing algorithm to
determine size, a "default object size" is needed to resolve the final
size, also called the concrete object size.

https://drafts.csswg.org/css-images/#default-sizing

This is done in a somewhat adhoc way in Blink, mainly by letting
Image::imageSize() return a bogus* size for these kind of images and
in selected places using
LayoutBoxModelObject::calculateImageIntrinsicDimensions to compute the
real size. But when using canvas.drawImage with an <img> pointing to
an SVG, the bogus size was used when determining the default size
leading to the wrong size being used.

This patch adds an updateConcreteObjectSize method to Image so that
the image can properly size in the context it's in, according to the
specification and use that to size images drawn with
canvas.drawImage().

BUG=581357, 475009

[davve, 2016-02-19 13:31:12]

Description was changed from

==========
Add Image::updateConcreteSize()

For SVG and generated images using the default sizing algorithm to
determine size, a "default object size" is needed to resolve the final
size, also called the concrete object size.

https://drafts.csswg.org/css-images/#default-sizing

This has been done in a somewhat adhoc way in Blink previously, mainly
by letting Image::imageSize() return a bogus size for these kind of
images and in selected places using
LayoutBoxModelObject::calculateImageIntrinsicDimensions to compute the
real size.

This patch changes the flow so that Image::imageSize() always returns
the correct size, as long as the image has been updated with the
default object size for the context the image is currently in.

BUG=581357, 475009
==========

to

==========
Add Image::updateConcreteObjectSize()

For SVG and generated images using the default sizing algorithm to
determine size, a "default object size" is needed to resolve the final
size, also called the concrete object size.

https://drafts.csswg.org/css-images/#default-sizing

This has been done in a somewhat adhoc way in Blink previously, mainly
by letting Image::imageSize() return a bogus size for these kind of
images and in selected places using
LayoutBoxModelObject::calculateImageIntrinsicDimensions to compute the
real size.

Image::updateConcreteObjectSize() changes the flow so that
Image::imageSize() always has the ability to return the correct size,
as long as the image has been updated with the default object size for
the context the image is currently in.

At this stage, updateConcreteSize is only used for canvas.drawImage().
It's planned to be used by call-site currently using
LBMO::calculateImageIntrinsicDimensions().

BUG=581357, 475009
==========

[davve, 2016-02-23 10:22:30]

Description was changed from

==========
Add Image::updateConcreteObjectSize()

For SVG and generated images using the default sizing algorithm to
determine size, a "default object size" is needed to resolve the final
size, also called the concrete object size.

https://drafts.csswg.org/css-images/#default-sizing

This has been done in a somewhat adhoc way in Blink previously, mainly
by letting Image::imageSize() return a bogus size for these kind of
images and in selected places using
LayoutBoxModelObject::calculateImageIntrinsicDimensions to compute the
real size.

Image::updateConcreteObjectSize() changes the flow so that
Image::imageSize() always has the ability to return the correct size,
as long as the image has been updated with the default object size for
the context the image is currently in.

At this stage, updateConcreteSize is only used for canvas.drawImage().
It's planned to be used by call-site currently using
LBMO::calculateImageIntrinsicDimensions().

BUG=581357, 475009
==========

to

==========
Add Image::updateConcreteSize()

For SVG and generated images using the default sizing algorithm to
determine size, a "default object size" is needed to resolve the final
size, also called the concrete object size.

https://drafts.csswg.org/css-images/#default-sizing

This is done in a somewhat adhoc way in Blink, mainly by letting
Image::imageSize() return a bogus* size for these kind of images and
in selected places using
LayoutBoxModelObject::calculateImageIntrinsicDimensions to compute the
real size. But when using canvas.drawImage with an <img> pointing to
an SVG, the bogus size was used when determining the default size
leading to the wrong size being used.

This patch adds an updateConcreteObjectSize method to Image so that
the image can properly size in the context it's in, according to the
specification and use that to size images drawn with
canvas.drawImage().

BUG=581357, 475009
==========

[davve, 2016-02-23 10:23:55]

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

[davve, 2016-02-23 10:23:56]

+fs and +pdr for SVG and image wisdom. +senorblanco for modules/canvas2d.

[fs, 2016-02-23 11:44:23]

LGTM w/ thoughts, nits and other ramblings.

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/canvas/canvas-default-object-sizing-expected.html
(right):

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/canvas/canvas-default-object-sizing-expected.html:6:
sx = typeof sx !== 'undefined' ? sx : 1;
Nit: Assuming we don't intend to pass 0, then maybe just "sx || 1" at the point
where it's used?

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:83: void
updateConcreteObjectSize(const LayoutSize& defaultObjectSize);
Thought: Could we avoid adding additional logistic through ImageResource (which
is oftentimes just an additional null-check) and instead do that in
LayoutImageResource (and other "hosts")? (I.e don't rely on the "fetch" layer
for anything but fetching the resource.) Just a thought thoug, no action need be
taken in/by this CL.

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Image.h:102: // object’s intrinsic
dimensions and specified size with the
Is that U+2019? Might want to avoid that and go with the regular dull U+0027
(for the sake of silly toolchains.) At least ISTR that it has been complained
about previously...

[davve, 2016-02-23 12:57:21]

On 2016/02/23 11:44:23, fs wrote:
> LGTM w/ thoughts, nits and other ramblings.

Note: I'm delaying adding the test until I add the contain constraint
against default object size in SVGImage::calculateCOS(). And I can't
add that before I move the other call-sites over to Image::upateCOS().

[davve, 2016-02-23 13:15:31]

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/canvas/canvas-default-object-sizing-expected.html
(right):

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/canvas/canvas-default-object-sizing-expected.html:6:
sx = typeof sx !== 'undefined' ? sx : 1;
On 2016/02/23 11:44:23, fs wrote:
> Nit: Assuming we don't intend to pass 0, then maybe just "sx || 1" at the
point
> where it's used?

Fixed in dependent CL.

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:83: void
updateConcreteObjectSize(const LayoutSize& defaultObjectSize);
On 2016/02/23 11:44:23, fs wrote:
> Thought: Could we avoid adding additional logistic through ImageResource
(which
> is oftentimes just an additional null-check) and instead do that in
> LayoutImageResource (and other "hosts")? (I.e don't rely on the "fetch" layer
> for anything but fetching the resource.) Just a thought though, no action need
be
> taken in/by this CL.

Sounds interesting and likely a good idea. Not sure how to keep track of the
idea. Create an issue or add a TODO?

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Image.h:102: // object’s intrinsic
dimensions and specified size with the
On 2016/02/23 11:44:23, fs wrote:
> Is that U+2019? Might want to avoid that and go with the regular dull U+0027
> (for the sake of silly toolchains.) At least ISTR that it has been complained
> about previously...

Yes, I c&p from spec text... Good old U+0027 coming up.

[fs, 2016-02-23 14:52:00]

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1694263003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:83: void
updateConcreteObjectSize(const LayoutSize& defaultObjectSize);
On 2016/02/23 at 13:15:30, David Vest wrote:
> On 2016/02/23 11:44:23, fs wrote:
> > Thought: Could we avoid adding additional logistic through ImageResource
(which
> > is oftentimes just an additional null-check) and instead do that in
> > LayoutImageResource (and other "hosts")? (I.e don't rely on the "fetch"
layer
> > for anything but fetching the resource.) Just a thought though, no action
need be
> > taken in/by this CL.
> 
> Sounds interesting and likely a good idea. Not sure how to keep track of the
idea. Create an issue or add a TODO?

Since it's a definite maybe, creating an issue might be more appropriate.

[Justin Novosad, 2016-02-23 15:53:03]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2016-02-23 15:53:04]

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1270:
imageSourceInternal->updateConcreteObjectSize(FloatSize(canvas()->width(),
canvas()->height()));
I did some spec analysis on this.
The spec for drawImage[1] says:

"If the image has no intrinsic dimensions, the concrete object size must be used
instead, as determined using the CSS "Concrete Object Size Resolution"
algorithm, with the specified size having neither a definite width nor height,
nor any additional constraints, the object's intrinsic properties being those of
the image argument, and the default object size being the size of the scratch
bitmap."

Now if we refer to Concrete Object Size algorithm[2], It is the last step that
applies in this case, which states:
"Otherwise, its size is resolved as a contain constraint against the default
object size."

A contain constraint is defined as[3]: "A contain constraint is resolved by
setting the concrete object size to the largest rectangle that has the object’s
intrinsic aspect ratio and additionally has neither width nor height larger than
the constraint rectangle’s width and height, respectively."

As far as I can tell, this is taken care of in
SVGImage::calculateConcreteObjectSize
So this particular overload of drawImage seems correct with this change, but
this CL is missing layout tests to cover these cases.  You should have tests to
cover SVG images with aspect ratio greater and smaller than the canvas, and you
should have tests that verify how this call handles transforms, in particular a
scale on the 2D context, and non-uniform scales that affect aspect ratio (which
do not affect the object size calculation).  It would also be helpful to add
comments that refer to the spec to explain that this line of code is
specifically for handling source images that have no intrinsic size.

[1]
https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-drawimage
[2] https://drafts.csswg.org/css-images/#default-sizing
[3] https://drafts.csswg.org/css-images/#contain-constraint

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1280:
imageSourceInternal->updateConcreteObjectSize(FloatSize(canvas()->width(),
canvas()->height()));
In this case, the change also seem to comply with the spec AFAICT, but tests are
missing.

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1289:
CanvasImageSource* imageSourceInternal = toImageSourceInternal(imageSource);
Here, we still have a problem. Even though the source rect is defined, we still
have a situation where the image has no intrinsic dimensions. Thus the frame of
reference for sx, sy, sw and sh is undefined.  This also needs tests.

[davve, 2016-02-23 18:40:31]

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1270:
imageSourceInternal->updateConcreteObjectSize(FloatSize(canvas()->width(),
canvas()->height()));
On 2016/02/23 15:53:04, Justin Novosad wrote:
> I did some spec analysis on this.
> The spec for drawImage[1] says:
> 
> "If the image has no intrinsic dimensions, the concrete object size must be
used
> instead, as determined using the CSS "Concrete Object Size Resolution"
> algorithm, with the specified size having neither a definite width nor height,
> nor any additional constraints, the object's intrinsic properties being those
of
> the image argument, and the default object size being the size of the scratch
> bitmap."
> 
> Now if we refer to Concrete Object Size algorithm[2], It is the last step that
> applies in this case, which states:
> "Otherwise, its size is resolved as a contain constraint against the default
> object size."
> 
> A contain constraint is defined as[3]: "A contain constraint is resolved by
> setting the concrete object size to the largest rectangle that has the
object’s
> intrinsic aspect ratio and additionally has neither width nor height larger
than
> the constraint rectangle’s width and height, respectively."
> 
> As far as I can tell, this is taken care of in
> SVGImage::calculateConcreteObjectSize

The contain constraint is actually not added yet, but it is included in the
dependent CL. I had to move my tests there as well. Until all code paths have
switched over to updateConcreteObjectSize, I got a lot of test failures when
adding the contain constraint because other code paths using 300x150 to resolve
instead of what updateConcreteObjectSize would feed it. Feel free to take a look
at it and related tests even though I haven't published it yet.

> So this particular overload of drawImage seems correct with this change, but
> this CL is missing layout tests to cover these cases.  You should have tests
to
> cover SVG images with aspect ratio greater and smaller than the canvas, and
you
> should have tests that verify how this call handles transforms, in particular
a
> scale on the 2D context, and non-uniform scales that affect aspect ratio
(which
> do not affect the object size calculation).

I'll look over testing in dependent CL tomorrow to see that these cases are all
covered.

> It would also be helpful to add
> comments that refer to the spec to explain that this line of code is
> specifically for handling source images that have no intrinsic size.

Sounds good.

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1280:
imageSourceInternal->updateConcreteObjectSize(FloatSize(canvas()->width(),
canvas()->height()));
On 2016/02/23 15:53:04, Justin Novosad wrote:
> In this case, the change also seem to comply with the spec AFAICT, but tests
are
> missing.

As I said above, this patch is mostly plumbing and the real change (dependent
CL) takes place when I can update the actual algorithm using this
defaultObjectSize propagated from here.

https://codereview.chromium.org/1694263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1289:
CanvasImageSource* imageSourceInternal = toImageSourceInternal(imageSource);
On 2016/02/23 15:53:04, Justin Novosad wrote:
> Here, we still have a problem. Even though the source rect is defined, we
still
> have a situation where the image has no intrinsic dimensions. Thus the frame
of
> reference for sx, sy, sw and sh is undefined.  This also needs tests.

Yes, I haven't gotten to this branch yet since it didn't use elementSize(). I
looked at it briefly this morning and concluded that it probably should
updateConcreteObjectSize too. Would it be ok to do that as a follow-up?

[Justin Novosad, 2016-02-23 19:40:35]

Test in follow-up sgtm.

There is one thing I would like to resolve in this CL though: does
CanvasRenderingContext2D.cpp need to know about the notion of Concrete Object
Size? It is a problem that is specific to <img>, so it could be solved in a code
path that is specific to drawing images into canvas. I mean
updateConcreteObjectSize could just be in HTMLImageElement::elementSize() (could
rename it to elementSizeForCanvas, and pass the canvas as an arg.). Would also
need it in HTMLImageElement::getSourceImageForCanvas, I think.

Also, why is concreteObjectSize a state? I mean why is this something that gets
stored on the object? I find this confusing because the value is contextual. I
mean an HTMLImageElement can have multiple concurrent concrete object sizes if
is drawn via DOM *and* via canvas, to multiple canvases, etc.

While reviewing this, I think I found an issue with the spec: How is the image
size computed when using CanvasRenderingContext2D.createPattern on a n SVG
image?
Appears to not be defined:
https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-createpa...

Also, I just looked at this section of the WebGL spec for determining the size
of an SVG image uploaded to a texture
https://www.khronos.org/registry/webgl/specs/latest/1.0/#TEXTURE_UPLOAD_SIZE
It says: "If an SVG image is uploaded, the width and height of the texture are
set to the current values of the width and height properties of the
HTMLImageElement object"
On the surface, that seems so much saner to use and reason about for both
implementors and web devs. Except that... the spec says that the width and
height IDL attributes are "0, if the image is not available or does not have
intrinsic dimensions" 
Source:
https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-width

OMG!

[Stephen White, 2016-02-23 20:24:49]

senorblanco@chromium.org changed reviewers:
- senorblanco@chromium.org

[Stephen White, 2016-02-23 20:25:03]

Deferring to Justin for the <canvas> changes.

[Justin Novosad, 2016-02-23 21:02:01]

Rebase required. Conflicts with this:
https://chromium.googlesource.com/chromium/src/+/4333ef80f90f702a0571c030f15f...

[Justin Novosad, 2016-02-23 21:07:14]

On 2016/02/23 21:02:01, Justin Novosad wrote:
> Rebase required. Conflicts with this:
>
https://chromium.googlesource.com/chromium/src/+/4333ef80f90f702a0571c030f15f...

This refactor moved drawImage into BaseRenderingContext2D, which does not have
access to the canvas element. You can get the intrinsic size through the new
virtual methods width() and height().

[davve, 2016-02-24 14:53:18]

Responding in reverse order:

On 2016/02/23 19:40:35, Justin Novosad wrote:

> Also, I just looked at this section of the WebGL spec for determining the size
> of an SVG image uploaded to a texture
> https://www.khronos.org/registry/webgl/specs/latest/1.0/#TEXTURE_UPLOAD_SIZE
> It says: "If an SVG image is uploaded, the width and height of the texture are
> set to the current values of the width and height properties of the
> HTMLImageElement object"
> On the surface, that seems so much saner to use and reason about for both
> implementors and web devs. Except that... the spec says that the width and
> height IDL attributes are "0, if the image is not available or does not have
> intrinsic dimensions" 
> Source:
> https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-width
> 
> OMG!

Yep, see also https://www.w3.org/Bugs/Public/show_bug.cgi?id=23581
which is about naturalWidth/naturalHeight but basically comes down to
the same thing. I think the best course of action is to define them in
terms of the default sizing algorithm, since that's the best we have
so far.

So let's assume we (still) want the default sizing algorithm for
WebGL, canvas and other consumers of <img>:

> While reviewing this, I think I found an issue with the spec: How is the image
> size computed when using CanvasRenderingContext2D.createPattern on a n SVG
> image?
> Appears to not be defined:
>
https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-createpa...

Looks like it. I'm not very familiar with
CanvasRenderingContext2D.createPattern but the reasonable thing would
be to size it similarly to drawImage(), default object size being the
canvas size?

> Also, why is concreteObjectSize a state? I mean why is this something that
gets
> stored on the object? I find this confusing because the value is contextual. I
> mean an HTMLImageElement can have multiple concurrent concrete object sizes if
> is drawn via DOM *and* via canvas, to multiple canvases, etc.

Since the other alternative I came up with was to pass
defaultObjectSize around in every call to Image::size(),
Image::rect(), Image::width() and Image::height() quickly got
unwieldy. It's both really intrusive and it wasn't always obvious to
me how to access the default object size.

See https://codereview.chromium.org/1732563007/ for the magnitude of
changes required. Grep for defaultConcreteObjectSize for places where
the defaultObjectSize has to be conjured up (defaultConcreteObjectSize
uses 300x150 and plays the role of TODO marker). Do you think such an
intrusive change is worth while for this? (Honest question, I'm on the
fence...).

As a middle way I could imagine a context interface you can set on an
Image that provides the default object size. That way a canvas and
others can temporarily override the default default object size. It
suffers from some of the same statefulness problem as the this CL
though.

> There is one thing I would like to resolve in this CL though: does
> CanvasRenderingContext2D.cpp need to know about the notion of Concrete Object
> Size? It is a problem that is specific to <img>, so it could be solved in a
code
> path that is specific to drawing images into canvas. I mean
> updateConcreteObjectSize could just be in HTMLImageElement::elementSize()
(could
> rename it to elementSizeForCanvas, and pass the canvas as an arg.). Would also
> need it in HTMLImageElement::getSourceImageForCanvas, I think.

Propagating defaultObjectSize (as a canvas or FloatSize) to
CanvasImageSource/HTMLImageElement but not further seems a bit
arbitrary to me.

[Justin Novosad, 2016-02-24 15:51:41]

On 2016/02/24 14:53:18, David Vest wrote:
> Responding in reverse order:
> 
> On 2016/02/23 19:40:35, Justin Novosad wrote:
> 
> > Also, I just looked at this section of the WebGL spec for determining the
size
> > of an SVG image uploaded to a texture
> > https://www.khronos.org/registry/webgl/specs/latest/1.0/#TEXTURE_UPLOAD_SIZE
> > It says: "If an SVG image is uploaded, the width and height of the texture
are
> > set to the current values of the width and height properties of the
> > HTMLImageElement object"
> > On the surface, that seems so much saner to use and reason about for both
> > implementors and web devs. Except that... the spec says that the width and
> > height IDL attributes are "0, if the image is not available or does not have
> > intrinsic dimensions" 
> > Source:
> > https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-width
> > 
> > OMG!
> 
> Yep, see also https://www.w3.org/Bugs/Public/show_bug.cgi?id=23581
> which is about naturalWidth/naturalHeight but basically comes down to
> the same thing. I think the best course of action is to define them in
> terms of the default sizing algorithm, since that's the best we have
> so far.
> 
> So let's assume we (still) want the default sizing algorithm for
> WebGL, canvas and other consumers of <img>:

Yes, I agree. I intend to propose this change to the WebGL spec, based on
discussion I just had with kbr@

> 
> > While reviewing this, I think I found an issue with the spec: How is the
image
> > size computed when using CanvasRenderingContext2D.createPattern on a n SVG
> > image?
> > Appears to not be defined:
> >
>
https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-createpa...
> 
> Looks like it. I'm not very familiar with
> CanvasRenderingContext2D.createPattern but the reasonable thing would
> be to size it similarly to drawImage(), default object size being the
> canvas size?

Yes, I just filed this issue against the spec:
https://github.com/whatwg/html/issues/735
> 
> > Also, why is concreteObjectSize a state? I mean why is this something that
> gets
> > stored on the object? I find this confusing because the value is contextual.
I
> > mean an HTMLImageElement can have multiple concurrent concrete object sizes
if
> > is drawn via DOM *and* via canvas, to multiple canvases, etc.
> 
> Since the other alternative I came up with was to pass
> defaultObjectSize around in every call to Image::size(),
> Image::rect(), Image::width() and Image::height() quickly got
> unwieldy. It's both really intrusive and it wasn't always obvious to
> me how to access the default object size.

Okay, that's fair.

> 
> Propagating defaultObjectSize (as a canvas or FloatSize) to
> CanvasImageSource/HTMLImageElement but not further seems a bit
> arbitrary to me.

Yeah... I guess it is a matter of personal preference more than anything else.

I have nothing further.

lgtm.

[davve, 2016-02-26 13:40:39]

Thinking about this patch a bit I don't think it's the right
direction.

For the SVG case we end up in a really strange order of events:

 1. We call updateConcreteObjectSize() on the SVGImage with the
    default object size. SVGImage keeps the concrete object size as
    state.

 2. A while later (for canvas in the getSourceImageForCanvas call) we
    extract the size from the SVGImage and put it in a
    SVGImageForContainer that wraps the SVGImage with that particular
    size.  Bonkers.

    Also, if I try to extrapolate this strategy to backgrounds or
    border-image I get into zooming problems since the default sizing
    algorithm will be done in unzoomed coordinates while it's
    currently done in zoomed coordinates.

Let's find something better. I currently see two ways:

 a. Wrap SVGImage inside SVGImageForContainer earlier, before
    accessing the image size. One way would be to add support to
    CanvasImageSource to start a "default sizing context" so that the
    implementation (HTMLImageElement) may wrap SVGImage with the
    concrete object size inside a SVGImageForContainer while the
    context is alive.

 b. Skip SVGImageForContainer and keep all the state inside
    SVGImage...

Neither is super tempting but a) feels like it would fit best with the
current system.

Closing for now.