Bug 39962 – Remove hand-rolled JSC bindings for CanvasRenderingContext2D

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore JavaScript

[Andreas Kling]

CodeGeneratorJS.pm supports overloading now, so we can get rid of most of JSCanvasRenderingContext2DCustom.cpp

[Andreas Kling]

Created attachment 57479 [details]
Proposed patch

[Andreas Kling]

Note that this won't compile until bug 39960 is fixed.

[Early Warning System Bot]

Attachment 57479 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2777014

[Darin Adler]

Comment on attachment 57479 [details]
Proposed patch

Great change!

> +        No new tests as there is no new functionality.

Are there tests for each of these overload cases? If not, then I think we do need tests, even though these code paths are not new. It looks to me like some of these hand-written bindings have different handling for things like too many arguments. It would be straightforward to write some tests that cover all these cases, and I'd like to see that to provde that the hand-rolled code did not behave any differently.

[WebKit Review Bot]

Attachment 57479 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2752009

[WebKit Review Bot]

Attachment 57479 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2825014

[Andreas Kling]

I looked into how functions should behave when called with the wrong number of arguments, and opened bug 40057 to make WebKit do the right thing bottom-up.

[Darin Adler]

*** This bug has been marked as a duplicate of bug 21257 ***

[Darin Adler]

Oops, did this to the wrong bug.

[Andreas Kling]

Created attachment 58566 [details]
Proposed patch v2

I wasn't sure what to do with this, so I test for the previous behavior, and expect the new behavior which won't match in many cases.
That said, the discrepancies look fairly harmless to me.

[Darin Adler]

Comment on attachment 58566 [details]
Proposed patch v2

The way I see it we have two separate issues:

    1) Hand-generation vs. auto-generation.

    2) Handling of missing arguments.

I would like to see a patch that only changes use from hand-generation to auto-generation, and then a separate patch to change handling of missing arguments. The former patch should be done right away! The latter, requires some though about how to keep existing code working. I believe there is quite a bit of code that depends on the current behavior, although mainly in Apple-specific products. Longer term it would be best to match other browsers, but the path from here to there may require some sort of compatibility mode.

I can’t tell from this patch whether it does or does not change the handling of missing arguments.

[Andreas Kling]

(In reply to comment #11)
> I would like to see a patch that only changes use from hand-generation to auto-generation, and then a separate patch to change handling of missing arguments.

And how should we do this without custom bindings? I doubt we'd want to specialize CodeGeneratorJS.pm for the idiosyncratic behaviors formerly found in  JSCanvasRenderingContext2DCustom.cpp

> The former patch should be done right away!

So you'd be OK with an intermediate state (auto-generated code that behaves slightly differently) while we think up a better solution for missing arguments?

> Longer term it would be best to match other browsers, but the path from here to there may require some sort of compatibility mode.

I like the sound of that. And count bug 21257 in as well, please ;-)

> I can’t tell from this patch whether it does or does not change the handling of missing arguments.

It does, implicitly, since generated bindings handle things differently than the hand-rolled code.

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #11)
> > I would like to see a patch that only changes use from hand-generation to auto-generation, and then a separate patch to change handling of missing arguments.
> 
> And how should we do this without custom bindings? I doubt we'd want to specialize CodeGeneratorJS.pm for the idiosyncratic behaviors formerly found in  JSCanvasRenderingContext2DCustom.cpp

I think we can get 99% of the behavior by having many arguments optional. Maybe you can give me examples of old behavior we can't reproduce with that alone. Those may be changes we can live with.

> > The former patch should be done right away!
> 
> So you'd be OK with an intermediate state (auto-generated code that behaves slightly differently) while we think up a better solution for missing arguments?

No.

> > Longer term it would be best to match other browsers, but the path from here to there may require some sort of compatibility mode.
> 
> I like the sound of that. And count bug 21257 in as well, please ;-)

I wish we could just wave a magic wad and do it the way that seems cleanest now. But keeping compatible does end up being hard work.

[Andreas Kling]

(In reply to comment #13)
> I think we can get 99% of the behavior by having many arguments optional. Maybe you can give me examples of old behavior we can't reproduce with that alone. Those may be changes we can live with.

Right, I'll sort this tomorrow (4AM here..) Thanks for your input thus far.

> I wish we could just wave a magic wad and do it the way that seems cleanest now. But keeping compatible does end up being hard work.

Absolutely. A compatibility mode is a fair compromise IMO. And I'm not at all opposed to doing said work myself, though I may need some spiritual guidance.

[Andreas Kling]

Created attachment 58593 [details]
Patch to replace hand-rolled overloads with autogenerated ones

Part 1 of 2 - handling of missing arguments coming soon..

[Andreas Kling]

Created attachment 58601 [details]
Patch to ignore superfluous arguments to IDL overloads

Part 2 of X (!) - make auto-generated overloads behave as the current custom code - ignore any extra arguments.

[Andreas Kling]

Remaining failures from original test:

> ctx.fillText() should throw SyntaxError: Syntax error. Was undefined.
> ctx.fillText('moo') should throw SyntaxError: Syntax error. Was undefined.
> ctx.fillText('moo',0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.fillText('moo',0,0,0,0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.strokeText() should throw SyntaxError: Syntax error. Was undefined.
> ctx.strokeText('moo') should throw SyntaxError: Syntax error. Was undefined.
> ctx.strokeText('moo',0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.strokeText('moo',0,0,0,0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.setStrokeColor(0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.setStrokeColor(0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.setFillColor(0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.setFillColor(0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.drawImage(imageElement, 0, 0) should be undefined. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1
> ctx.drawImage(canvasElement, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.drawImage(canvasElement, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.drawImage(canvasElement, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.drawImage(canvasElement, 0, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.drawImage(canvasElement, 0, 0, 0, 0, 0, 0, 0, 0) should throw Error: INDEX_SIZE_ERR: DOM Exception 1. Was undefined.
> ctx.drawImage(canvasElement, 0, 0, 0, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.drawImage(canvasElement, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Was undefined.
> ctx.createImageData(0, 0, 0) should be null. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1

Auto-generated code is more permissive than custom bindings.

> ctx.setStrokeColor() should throw SyntaxError: Syntax error. Threw exception TypeError: Type error.
> ctx.setFillColor() should throw SyntaxError: Syntax error. Threw exception TypeError: Type error.
> ctx.createImageData(0) should throw Error: NOT_SUPPORTED_ERR: DOM Exception 9. Threw exception TypeError: Type error.
> ctx.drawImage(imageElement) should throw SyntaxError: Syntax error. Threw exception TypeError: Type error.
> ctx.drawImage(imageElement, 0) should throw SyntaxError: Syntax error. Threw exception TypeError: Type error.
> ctx.drawImage(imageElement, 0, 0, 0) should throw SyntaxError: Syntax error. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1.
> ctx.drawImage(imageElement, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1.
> ctx.drawImage(imageElement, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1.
> ctx.drawImage(imageElement, 0, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1.
> ctx.drawImage(imageElement, 0, 0, 0, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1.
> ctx.drawImage(imageElement, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) should throw SyntaxError: Syntax error. Threw exception Error: INDEX_SIZE_ERR: DOM Exception 1.
> ctx.drawImage(canvasElement) should throw SyntaxError: Syntax error. Threw exception TypeError: Type error.
> ctx.drawImage(canvasElement, 0) should throw SyntaxError: Syntax error. Threw exception TypeError: Type error.
> ctx.createPattern(imageElement) should throw Error: SYNTAX_ERR: DOM Exception 12. Threw exception TypeError: Type error.
> ctx.createPattern(canvasElement) should throw Error: SYNTAX_ERR: DOM Exception 12. Threw exception TypeError: Type error.

Auto-generated and custom bindings both throw, but different exceptions.

> ctx.createImageData() should be null. Threw exception TypeError: Type error

This one would need some finagling with making the first argument optional etc.

> ctx.putImageData(imageData, 0, 0, 0) should be undefined. Threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9
> ctx.putImageData(imageData, 0, 0, 0, 0) should be undefined. Threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9
> ctx.putImageData(imageData, 0, 0, 0, 0, 0) should be undefined. Threw exception Error: NOT_SUPPORTED_ERR: DOM Exception 9

Missing arguments become NaN instead of 0, hence the NOT_SUPPORTED_ERR.
I guess we could add some custom IDL attribute like [PassZeroForMissingArguments].

[Andreas Kling]

Created attachment 58602 [details]
Patch that just adds the LayoutTest

Part 3 of X - The LayoutTest. should*() values are the old behavior, -expected.txt is the new behavior.

[Andreas Kling]

*** This bug has been marked as a duplicate of bug 39243 ***