Bug 64628 - Remove LegacyDefaultOptionalArguments flag from CanvasRenderingContext2d
Summary: Remove LegacyDefaultOptionalArguments flag from CanvasRenderingContext2d
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-15 14:30 PDT by Mark Pilgrim (Google)
Modified: 2011-11-01 11:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.34 KB, patch)
2011-07-15 14:32 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.43 MB, application/zip)
2011-07-15 16:00 PDT, WebKit Review Bot
no flags Details
Patch (10.96 KB, patch)
2011-07-20 14:17 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (21.67 KB, patch)
2011-08-04 18:22 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (16.97 KB, patch)
2011-10-28 16:58 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (16.98 KB, patch)
2011-10-28 17:12 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (16.17 KB, patch)
2011-10-31 17:09 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (14.56 KB, patch)
2011-11-01 10:10 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (14.55 KB, patch)
2011-11-01 10:12 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2011-07-15 14:30:41 PDT
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.
Comment 1 Mark Pilgrim (Google) 2011-07-15 14:32:53 PDT
Created attachment 101043 [details]
Patch
Comment 2 Adam Barth 2011-07-15 14:47:36 PDT
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?
Comment 3 WebKit Review Bot 2011-07-15 15:59:58 PDT
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
Comment 4 WebKit Review Bot 2011-07-15 16:00:04 PDT
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
Comment 5 Mark Pilgrim (Google) 2011-07-20 14:17:12 PDT
Created attachment 101508 [details]
Patch
Comment 6 Adam Barth 2011-07-21 10:17:47 PDT
Thoughts on Comment #2?
Comment 7 Mark Pilgrim (Google) 2011-07-21 10:39:43 PDT
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?
Comment 8 Mark Pilgrim (Google) 2011-08-03 15:50:16 PDT
Bump. Who should review this?
Comment 9 Adam Barth 2011-08-03 17:43:16 PDT
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.
Comment 10 Mark Pilgrim (Google) 2011-08-04 18:22:57 PDT
Created attachment 103017 [details]
Patch
Comment 11 Adam Barth 2011-08-04 18:47:08 PDT
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?
Comment 12 Erik Arvidsson 2011-10-28 16:58:09 PDT
Created attachment 112942 [details]
Patch
Comment 13 Erik Arvidsson 2011-10-28 17:05:27 PDT
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.
Comment 14 Adam Barth 2011-10-28 17:09:53 PDT
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.
Comment 15 Erik Arvidsson 2011-10-28 17:12:11 PDT
Created attachment 112944 [details]
Patch for landing
Comment 16 Erik Arvidsson 2011-10-28 17:16:01 PDT
(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
Comment 17 WebKit Review Bot 2011-10-28 17:48:29 PDT
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
Comment 18 Erik Arvidsson 2011-10-31 17:09:02 PDT
Created attachment 113109 [details]
Patch
Comment 19 Erik Arvidsson 2011-11-01 10:10:59 PDT
Created attachment 113183 [details]
Patch for landing
Comment 20 Erik Arvidsson 2011-11-01 10:12:18 PDT
Created attachment 113185 [details]
Patch for landing
Comment 21 WebKit Review Bot 2011-11-01 11:45:03 PDT
Comment on attachment 113185 [details]
Patch for landing

Clearing flags on attachment: 113185

Committed r98985: <http://trac.webkit.org/changeset/98985>
Comment 22 WebKit Review Bot 2011-11-01 11:45:18 PDT
All reviewed patches have been landed.  Closing bug.