Add Image::updateConcreteSize()

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

 /*
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2010 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2007 Alp Toker <alp@atoker.com>
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2008 Dirk Schulze <krit@webkit.org>
  * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
  * Copyright (C) 2012, 2013 Intel Corporation. All rights reserved.
  * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
@@ skipping 1249 matching lines @@
         return value.getAsHTMLCanvasElement().get();
     if (value.isImageBitmap())
         return value.getAsImageBitmap().get();
     ASSERT_NOT_REACHED();
     return nullptr;
 }
 
 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);
 }
 
 void CanvasRenderingContext2D::drawImage(const CanvasImageSourceUnion& imageSource,
     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);
 }
 
 void CanvasRenderingContext2D::drawImage(const CanvasImageSourceUnion& imageSource,
     double sx, double sy, double sw, double sh,
     double dx, double dy, double dw, double dh, ExceptionState& exceptionState)
 {
     CanvasImageSource* imageSourceInternal = toImageSourceInternal(imageSource);

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

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]

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?

     drawImage(imageSourceInternal, sx, sy, sw, sh, dx, dy, dw, dh, exceptionState);
 }
 
 bool CanvasRenderingContext2D::shouldDrawImageAntialiased(const FloatRect& destRect) const
 {
     if (!shouldAntialias())
         return false;
     SkCanvas* c = drawingCanvas();
     ASSERT(c);
 
@@ skipping 1070 matching lines @@
         if (imageType == CanvasRenderingContext2DState::NonOpaqueImage)
             return;
         if (alpha < 0xFF)
             return;
     }
 
     canvas()->buffer()->willOverwriteCanvas();
 }
 
 } // namespace blink