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.
Created attachment 101043 [details] Patch
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 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
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
Created attachment 101508 [details] Patch
Thoughts on Comment #2?
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?
Bump. Who should review this?
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.
Created attachment 103017 [details] Patch
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?
Created attachment 112942 [details] Patch
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 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.
Created attachment 112944 [details] Patch for landing
(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 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
Created attachment 113109 [details] Patch
Created attachment 113183 [details] Patch for landing
Created attachment 113185 [details] Patch for landing
Comment on attachment 113185 [details] Patch for landing Clearing flags on attachment: 113185 Committed r98985: <http://trac.webkit.org/changeset/98985>
All reviewed patches have been landed. Closing bug.