Add Image::updateConcreteSize()

third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp

Index: third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
diff --git a/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp b/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
index fdcfd12a44b4b55eb6ac77b05aa4155e49ea5428..ecb58ef86d6ff9e10daf234b3bf0629acaec1f4a 100644
--- a/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
+++ b/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
@@ -1267,6 +1267,7 @@ static inline CanvasImageSource* toImageSourceInternal(const CanvasImageSourceUn
 void CanvasRenderingContext2D::drawImage(const CanvasImageSourceUnion& imageSource, double x, double y, ExceptionState& exceptionState)
 {
     CanvasImageSource* imageSourceInternal = toImageSourceInternal(imageSource);
+    imageSourceInternal->updateConcreteObjectSize(FloatSize(canvas()->width(), canvas()->height()));

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

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

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

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.
I'll look over testing in dependent CL tomorrow to see that these cases are all
covered.
Sounds good.

     FloatSize sourceRectSize = imageSourceInternal->elementSize();
     FloatSize destRectSize = imageSourceInternal->defaultDestinationSize();
     drawImage(imageSourceInternal, 0, 0, sourceRectSize.width(), sourceRectSize.height(), x, y, destRectSize.width(), destRectSize.height(), exceptionState);
@@ -1276,6 +1277,7 @@ void CanvasRenderingContext2D::drawImage(const CanvasImageSourceUnion& imageSour
     double x, double y, double width, double height, ExceptionState& exceptionState)
 {
     CanvasImageSource* imageSourceInternal = toImageSourceInternal(imageSource);
+    imageSourceInternal->updateConcreteObjectSize(FloatSize(canvas()->width(), canvas()->height()));

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

In this case, the change also seem to comply with the spec AFAICT, but tests are
missing.

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

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.

     FloatSize sourceRectSize = imageSourceInternal->elementSize();
     drawImage(imageSourceInternal, 0, 0, sourceRectSize.width(), sourceRectSize.height(), x, y, width, height, exceptionState);
 }