Bug 64628 – Remove LegacyDefaultOptionalArguments flag from CanvasRenderingContext2d

• Component: Canvas
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

As discussed with abarth in IRC, we are converting all IDL files to the new code generator, which throws on missing required arguments by default (as required by the WebIDL spec). As most of CanvasRenderingContext2d already did this on a function-by-function basis (using [RequiresAllArguments=Raise]), this conversion was fairly straightforward. There were a small number of layout tests that relied on the old behavior, which have been fixed in this patch.

[Mark Pilgrim (Google)]

Created attachment 101043 [details]
Patch

[Adam Barth]

Comment on attachment 101043 [details]
Patch

I'm a bit worried about changing our behavior here.  <canvas> was originally developed in WebKit and is used by a bunch of WebKit-specific code, such as Dashboard widgets.  Maybe it would be better to aggressively mark arguments as being optional to avoid changing behavior?

[WebKit Review Bot]

Comment on attachment 101043 [details]
Patch

Attachment 101043 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9096507

New failing tests:
canvas/philip/tests/2d.path.arc.default.html

[WebKit Review Bot]

Created attachment 101061 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mark Pilgrim (Google)]

Created attachment 101508 [details]
Patch

[Adam Barth]

Thoughts on Comment #2?

[Mark Pilgrim (Google)]

I have no particular expertise in whether these can be made required after being effectively optional for so long. Who would be qualified to make a decision on that?

[Mark Pilgrim (Google)]

Bump. Who should review this?

[Adam Barth]

Comment on attachment 101508 [details]
Patch

Lets go with a version that doesn't change behavior.  I feel like it's too risky to change some of these functions.

[Mark Pilgrim (Google)]

Created attachment 103017 [details]
Patch

[Adam Barth]

Comment on attachment 103017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103017&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:212
> +        void drawImageFromRect(in HTMLImageElement image,
>                                                              in [Optional] float sx, in [Optional] float sy, in [Optional] float sw, in [Optional] float sh,
>                                                              in [Optional] float dx, in [Optional] float dy, in [Optional] float dw, in [Optional] float dh,
>                                                              in [Optional] DOMString compositeOperation);

Maybe re-indent the rest of the arguments?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:238
> -        void setShadow(in float width, in float height, in float blur, in [Optional] DOMString color, in [Optional] float alpha);
> -        void setShadow(in float width, in float height, in float blur, in float grayLevel, in [Optional] float alpha);
> -        void setShadow(in float width, in float height, in float blur, in float r, in float g, in float b, in float a);
> -        void setShadow(in float width, in float height, in float blur, in float c, in float m, in float y, in float k, in float a);
> +        void setShadow(in float width, 
> +                       in float height, 
> +                       in float blur, 
> +                       in [Optional] DOMString color, 
> +                       in [Optional] float alpha);
> +        void setShadow(in float width, 
> +                       in float height, 
> +                       in float blur, 
> +                       in float grayLevel, 
> +                       in [Optional] float alpha);
> +        void setShadow(in float width, 
> +                       in float height, 
> +                       in float blur, 
> +                       in float r, 
> +                       in float g, 
> +                       in float b, 
> +                       in float a);
> +        void setShadow(in float width, 
> +                       in float height, 
> +                       in float blur, 
> +                       in float c, 
> +                       in float m, 
> +                       in float y, 
> +                       in float k, 
> +                       in float a);

Not optional?

[Erik Arvidsson]

Created attachment 112942 [details]
Patch

[Erik Arvidsson]

Comment on attachment 112942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112942&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:168
>          void setFillColor(in float r, in float g, in float b, in float a);
>          void setFillColor(in float c, in float m, in float y, in float k, in float a);

I started off with a (alpha) being optional but there are tests  that expects to fail when alpha is missing.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:205
>          void setShadow(in float width, in float height, in float blur, in float r, in float g, in float b, in float a);
>          void setShadow(in float width, in float height, in float blur, in float c, in float m, in float y, in float k, in float a);

I started off with a (alpha) being optional but there are tests  that expects to fail when alpha is missing.

[Adam Barth]

Comment on attachment 112942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112942&action=review

This looks great.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:35
>          void save();

The Web Applications 1.0 draft comment above this kind is kind of silly at this point.

[Erik Arvidsson]

Created attachment 112944 [details]
Patch for landing

[Erik Arvidsson]

(In reply to comment #14)
> (From update of attachment 112942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112942&action=review
> 
> This looks great.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:35
> >          void save();
> 
> The Web Applications 1.0 draft comment above this kind is kind of silly at this point.

Removed and landing safely since I saw failures related to canvas on my master branch

[WebKit Review Bot]

Comment on attachment 112944 [details]
Patch for landing

Rejecting attachment 112944 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
gs/gen/webkit/bindings/V8DerivedSources16.o
  CXX(target) out/Debug/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources17.o
In file included from out/Debug/obj/gen/webkit/bindings/V8DerivedSources17.cpp:31:
out/Debug/obj/gen/webcore/bindings/V8CanvasRenderingContext2D.cpp:30: fatal error: V8.h: No such file or directory
compilation terminated.
make: *** [out/Debug/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources17.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10242199

[Erik Arvidsson]

Created attachment 113109 [details]
Patch

[Erik Arvidsson]

Created attachment 113183 [details]
Patch for landing

[Erik Arvidsson]

Created attachment 113185 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113185 [details]
Patch for landing

Clearing flags on attachment: 113185

Committed r98985: <http://trac.webkit.org/changeset/98985>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.