Use CanvasImageSource union type instead of overloading

Use CanvasImageSource union type instead of overloading

In CanvasRenderingContext2D.idl, use the already defined union type
CanvasImageSource for the argument to drawImage() and createPattern(),
instead of having one overload of each per union member type.

This reduces the number of overloads for drawImage() from 12 to 3 and
thus greatly reduces the amount of generated bindings code for it.

BUG=430337
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185247

[Jens Widell, 2014-11-05 13:18:35]

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

[Jens Widell, 2014-11-05 13:18:36]

PTAL

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (left):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.idl:122:
[RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] void
drawImage(ImageBitmap imageBitmap, unrestricted float x, unrestricted float y);
We're losing the runtime enabledness of ImageBitmap versions of drawImage() with
this patch. I believe this is a non-issue in practice because creation of
ImageBitmap object is itself runtime enabled. IOW, it's impossible to call the
function with an ImageBitmap argument if ExperimentalCanvasFeatures isn't
enabled.

If we think it is an issue (and it may be in other cases in the future), some
possible solutions that spring to mind:

1) Support runtime enabled union members.

2) Keep drawImage(ImageBitmap, ...) versions as separate overloads.

3) Just to be sure: Add a custom check in the implementation and throw an
exception if an ImageBitmap is encountered but ExperimentalCanvasFeatures isn't
enabled.

1 makes sense to me, but is something we probably want to delay until we really
need it, hoping that we never actually do.

2 would be sad: getting rid of the overloading completely neatly solves a
performance issue (we type-check the first argument multiple times) that we
haven't found any other elegant solutions for.

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.idl:71: [RaisesException]
CanvasPattern createPattern(CanvasImageSource image, DOMString? repetitionType);
I'm adding support for an ImageBitmap argument to createPattern() here (if
ExperimentalCanvasFeatures is enabled.) This is per spec.

I have no idea if this actually works (IOW if createPattern() actually supports
ImageBitmap objects already) but can't see anything that directly indicates it
doesn't.

[Justin Novosad, 2014-11-05 20:40:53]

This is great! The only thing missing is a test for createPattern(ImageBitmap)

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1496: ASSERT_NOT_REACHED();
Just to be sure, this is not reached even if the argument is a bogus object,
right?  Do the bindings take care of throwing an exception in that case?

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.h:68: typedef
HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrImageBitmap
CanvasImageSourceUnion;
What is this long type name? Is this something that is auto-generated by the IDL
parser?

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (left):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.idl:122:
[RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] void
drawImage(ImageBitmap imageBitmap, unrestricted float x, unrestricted float y);
2) is tricky. We had something similar in the past and it exposed weird bugs in
the binding code generator.

I think we can keep this code as is, without any special checks or disabling.
The fact that the ImageBitmap constructors are hidden is good enough for me.
It's a conversation I had with API owners last year, and we came to that
conclusion.

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.idl:71: [RaisesException]
CanvasPattern createPattern(CanvasImageSource image, DOMString? repetitionType);
Better add layout test coverage for this.

[Jens Widell, 2014-11-06 11:44:40]

Test added for createPattern(ImageBitmap, ...). (Largely copied from the
canvas-createImageBitmap-drawImage.html test.)

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1496: ASSERT_NOT_REACHED();
On 2014/11/05 20:40:52, junov wrote:
> Just to be sure, this is not reached even if the argument is a bogus object,
> right?  Do the bindings take care of throwing an exception in that case?

Yes, the generated code for converting a V8 value to the union container type
will throw an exception if the V8 value doesn't match any of the member types.

There are layout tests that call there functions with various invalid arguments
(the ones whose expected output I modified in this CL.)

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.h:68: typedef
HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrImageBitmap
CanvasImageSourceUnion;
On 2014/11/05 20:40:52, junov wrote:
> What is this long type name? Is this something that is auto-generated by the
IDL
> parser?

Yes. It's simply the type's name according to WebIDL (member type names joined
by "Or".)

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (left):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.idl:122:
[RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] void
drawImage(ImageBitmap imageBitmap, unrestricted float x, unrestricted float y);
On 2014/11/05 20:40:52, junov wrote:
> 2) is tricky. We had something similar in the past and it exposed weird bugs
in
> the binding code generator.

Yes. In fact, currently I believe 2 would expose the bug "union types can't be
distinguishing arguments in overloading". :-)

Since union types and overloading solve the same problem, one can hope that no
spec will ever combine the two this way, and that we never need to implement
support for it.

(Overloading methods with union arguments that aren't the distinguishing
argument, like drawImage() here, is of course fine.)

> I think we can keep this code as is, without any special checks or disabling.
> The fact that the ImageBitmap constructors are hidden is good enough for me.
> It's a conversation I had with API owners last year, and we came to that
> conclusion.

Great!

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.idl (right):

https://codereview.chromium.org/703783004/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.idl:71: [RaisesException]
CanvasPattern createPattern(CanvasImageSource image, DOMString? repetitionType);
On 2014/11/05 20:40:52, junov wrote:
> Better add layout test coverage for this.

Done.

Beware I'm a complete noob when it comes to actually using any kind of canvas
APIs. :-)

[Jens Widell, 2014-11-11 11:11:35]

junov@: ping? :-)

[Justin Novosad, 2014-11-12 20:39:11]

On 2014/11/11 11:11:35, Jens Widell wrote:
> junov@: ping? :-)

Sorry, I was OOO.  lgtm

[Jens Widell, 2014-11-12 20:48:01]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-11-12 20:49:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/703783004/60001

[commit-bot: I haz the power, 2014-11-12 21:55:46]

Committed patchset #4 (id:60001) as 185247

[Nils Barth (inactive), 2014-11-14 21:29:40]

Woo-hoo!
This is great on so many levels:
- simplifies interface
- reduces size of bindings
- faster!
See:
https://codereview.chromium.org/611953003/
...for speed benefit.