Use CanvasImageSource union type instead of overloading

Source/core/html/canvas/CanvasRenderingContext2D.idl

Index: Source/core/html/canvas/CanvasRenderingContext2D.idl
diff --git a/Source/core/html/canvas/CanvasRenderingContext2D.idl b/Source/core/html/canvas/CanvasRenderingContext2D.idl
index 198cf3ade55877b017f060dcf61292783b4b5a86..1faf6e71c675fe86d4a80dee06562dea7ce6403d 100644
--- a/Source/core/html/canvas/CanvasRenderingContext2D.idl
+++ b/Source/core/html/canvas/CanvasRenderingContext2D.idl
@@ -26,14 +26,11 @@
 // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#canvasrenderingcontext2d
 
 // FIXME: float => double throughout
-// FIXME: Use union type in drawImage and createPattern once supported:
-// http://crbug.com/372891
 typedef (HTMLImageElement or
          HTMLVideoElement or
-         HTMLCanvasElement // or
+         HTMLCanvasElement or
          // CanvasRenderingContext2D or
-         // ImageBitmap
-         ) CanvasImageSource;
+         ImageBitmap) CanvasImageSource;
 
 enum CanvasFillRule { "nonzero", "evenodd" };
 
@@ -71,9 +68,7 @@ enum CanvasFillRule { "nonzero", "evenodd" };
     [Custom] attribute object fillStyle; // (default black)
     CanvasGradient createLinearGradient(float x0, float y0, float x1, float y1);
     [RaisesException] CanvasGradient createRadialGradient(float x0, float y0, float r0, float x1, float y1, float r1);
-    [RaisesException] CanvasPattern createPattern(HTMLCanvasElement canvas, DOMString? repetitionType);
-    [RaisesException] CanvasPattern createPattern(HTMLImageElement image, DOMString? repetitionType);
-    [RaisesException] CanvasPattern createPattern(HTMLVideoElement image, DOMString? repetitionType);
+    [RaisesException] CanvasPattern createPattern(CanvasImageSource image, DOMString? repetitionType);

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

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:52]

Better add layout test coverage for this.

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

Done.

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

 
     // shadows
     attribute unrestricted float shadowOffsetX;
@@ -110,18 +105,9 @@ enum CanvasFillRule { "nonzero", "evenodd" };
     TextMetrics measureText(DOMString text);
 
     // drawing images
-    [RaisesException] void drawImage(HTMLImageElement image, unrestricted float x, unrestricted float y);
-    [RaisesException] void drawImage(HTMLImageElement image, unrestricted float x, unrestricted float y, unrestricted float width, unrestricted float height);
-    [RaisesException] void drawImage(HTMLImageElement image, unrestricted float sx, unrestricted float sy, unrestricted float sw, unrestricted float sh, unrestricted float dx, unrestricted float dy, unrestricted float dw, unrestricted float dh);
-    [RaisesException] void drawImage(HTMLCanvasElement canvas, unrestricted float x, unrestricted float y);
-    [RaisesException] void drawImage(HTMLCanvasElement canvas, unrestricted float x, unrestricted float y, unrestricted float width, unrestricted float height);
-    [RaisesException] void drawImage(HTMLCanvasElement canvas, unrestricted float sx, unrestricted float sy, unrestricted float sw, unrestricted float sh, unrestricted float dx, unrestricted float dy, unrestricted float dw, unrestricted float dh);
-    [RaisesException] void drawImage(HTMLVideoElement video, unrestricted float x, unrestricted float y);
-    [RaisesException] void drawImage(HTMLVideoElement video, unrestricted float x, unrestricted float y, unrestricted float width, unrestricted float height);
-    [RaisesException] void drawImage(HTMLVideoElement video, unrestricted float sx, unrestricted float sy, unrestricted float sw, unrestricted float sh, unrestricted float dx, unrestricted float dy, unrestricted float dw, unrestricted float dh);
-    [RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] void drawImage(ImageBitmap imageBitmap, unrestricted float x, unrestricted float y);

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

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.

[Justin Novosad, 2014/11/05 20:40:52]

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.

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

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.)
Great!

-    [RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] void drawImage(ImageBitmap imageBitmap, unrestricted float x, unrestricted float y, unrestricted float width, unrestricted float height);
-    [RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] void drawImage(ImageBitmap imageBitmap, unrestricted float sx, unrestricted float sy, unrestricted float sw, unrestricted float sh, unrestricted float dx, unrestricted float dy, unrestricted float dw, unrestricted float dh);
+    [RaisesException] void drawImage(CanvasImageSource image, unrestricted float x, unrestricted float y);
+    [RaisesException] void drawImage(CanvasImageSource image, unrestricted float x, unrestricted float y, unrestricted float width, unrestricted float height);
+    [RaisesException] void drawImage(CanvasImageSource image, unrestricted float sx, unrestricted float sy, unrestricted float sw, unrestricted float sh, unrestricted float dx, unrestricted float dy, unrestricted float dw, unrestricted float dh);
 
     // hit regions
     [RuntimeEnabled=ExperimentalCanvasFeatures, RaisesException] void addHitRegion(optional HitRegionOptions options);