Make optional arguments in CanvasRenderingContext2D match the spec [1].

Make optional arguments in CanvasRenderingContext2D match the spec [1].

WebKit r98985 set many arguments optional although the canvas spec does not
indicate. So this patch corrects CanvasRenderingContext2D.idl to match the spec.

[1] http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/

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

[dglazkov, 2013-04-30 17:53:53]

Mis-uploaded patch? Ever side-by-side diff complains about old chunks.

[dshwang, 2013-04-30 18:02:04]

On 2013/04/30 17:51:36, dshwang wrote:

WebKit r98985 is http://trac.webkit.org/changeset/98985
If you see r98985, you can understand why this mistake was made easily.

I remove all "[Default=Undefined] optional" in CanvasRenderingContext2D.idl,
except for one optional: arc(...., [Default=Undefined] optional boolean
anticlockwise)
anticlockwise of arc() must be optional because the spec said.

2d.missingargs.html test already alerted us that our implementation has this
bug, but WebKit made expected file matching to error implementation.
So this patch corrects the expected file about TypeError.

canvas-overloads-strokeRect test thought there were several overloaded
strokeRect functions, but the spec defines only one strokeRect method.
It means that "ctx.strokeRect(0, 0, 0, 0, 0)" should be removed also. I'll do it
another bug.

[dshwang, 2013-04-30 18:03:25]

On 2013/04/30 17:53:53, Dimitri Glazkov wrote:
> Mis-uploaded patch? Ever side-by-side diff complains about old chunks.

I think there is exception during uploading patch. I'll upload again. :)

[dshwang, 2013-04-30 18:23:49]

https://codereview.chromium.org/14654002/diff/9001/LayoutTests/canvas/philip/...
File LayoutTests/canvas/philip/tests/2d.missingargs-expected.txt (left):

https://codereview.chromium.org/14654002/diff/9001/LayoutTests/canvas/philip/...
LayoutTests/canvas/philip/tests/2d.missingargs-expected.txt:76: Failed
assertion: expected exception of type TypeError, got: Error: SyntaxError: DOM
Exception 12
We should get rid of above two assertions also. I'll do it in another issue.

[dshwang, 2013-05-07 16:20:01]

Could you kindly review this issue?

I submitted the same bug in WebKit also:
https://bugs.webkit.org/show_bug.cgi?id=115723

[dglazkov, 2013-05-07 16:39:34]

Is there a compatibility impact from this change? I am not very familiar with
canvas stuff.

[dglazkov, 2013-05-07 16:40:09]

senorblanco, can you look over this and show me your thumbs?

[dshwang, 2013-05-07 16:53:23]

On 2013/05/07 16:39:34, Dimitri Glazkov wrote:
> Is there a compatibility impact from this change? I am not very familiar with
> canvas stuff.

Thank you for review!

It is possible for this change to affect a compatibility.

For example, assume that a web developer uses fillRect api like this:
fillRect();
This is illegal to spec, and other browsers probably fire exception, but WebKit
has accepted it silently until now. Something like those wrong sites can be
affected.

Do you think I need to go through “Intent to Ship” process?

[eseidel, 2013-05-07 17:01:31]

I don't think this needs an "intent to implment" so long as we've tested FF And
IE and confirmed that this new behavior matches theirs.  This is a trivial
change.

Can you confirm that you've tested FF/IE (at least one of them is definitely
needed) and that we now match?  If we're throwing exceptions where they do not,
then we're at risk of breaking sites.

[dshwang, 2013-05-07 17:16:56]

On 2013/05/07 17:01:31, Eric Seidel (Google) wrote:
> I don't think this needs an "intent to implment" so long as we've tested FF
And
> IE and confirmed that this new behavior matches theirs.  This is a trivial
> change.
> 
> Can you confirm that you've tested FF/IE (at least one of them is definitely
> needed) and that we now match?  If we're throwing exceptions where they do
not,
> then we're at risk of breaking sites.

Thank you for explanation. That's reasonable :)

FF behaves correctly (same to Blink after applying this patch). I don't check IE
yet. After check, I'll report.

[dshwang, 2013-05-07 17:26:33]

On 2013/05/07 17:16:56, dshwang wrote:
> On 2013/05/07 17:01:31, Eric Seidel (Google) wrote:
> > Can you confirm that you've tested FF/IE (at least one of them is definitely
> > needed) and that we now match?  If we're throwing exceptions where they do
> not,
> > then we're at risk of breaking sites.

Eric, I checked :)

FF fires exception when the number of arguments is not enough.
IE fires exception when the number of arguments is not enough. However, IE fires
improper exceptions in some cases: e.g. createImageData
Blink/WebKit do not fires exception when the number of arguments is not enough.

We can check it easily thank to Philp:
http://www.w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.missinga...

FYI, Blink, WebKit and FF failed two cases in the above test. It is not related
to this issue, and I'll fix it in another issue if possible.

[Stephen White, 2013-05-07 17:29:19]

This seems reasonable, although I don't know the history of why these were made
optional in the first place.

[dglazkov, 2013-05-07 17:32:16]

LGTM.

[commit-bot: I haz the power, 2013-05-07 17:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14654002/15001

[dshwang, 2013-05-07 17:36:34]

On 2013/05/07 17:29:19, Stephen White wrote:
> This seems reasonable, although I don't know the history of why these were
made
> optional in the first place.

As I mentioned #3, WebKit r98985 is culprit:
http://trac.webkit.org/changeset/98985
I think as I'm the author to the change, I would make the same mistake  perhaps.

Thank Dimitri for review!

[commit-bot: I haz the power, 2013-05-07 18:14:20]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-08 04:22:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14654002/29002

[dshwang, 2013-05-08 04:23:27]

On 2013/05/07 18:14:20, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_layout_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

Patch Set 4 changes
inspector/profiler/canvas2d/canvas2d-profiler-capturing-basics.html
I assumed all canvas tests are in fast/canvas and canvas.

[commit-bot: I haz the power, 2013-05-08 05:16:47]

Change committed as 149933