Bug 23809 – Return CanvasRenderingContext2D instead of DOMObject in IDL to avoid V8 #ifdefs

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Darin Fisher (:fishd, Google)]

Return CanvasRenderingContext2D instead of DOMObject in IDL to avoid V8 #ifdefs

The code generator for JSC and V8 is a bit simpler when the return type of a method matches the actual type in the code.  I know that getContext and getCSSCanvasContext are both spec'd to return DOMObject, but presently our code generators go through some extra steps to force that to mean CanvasRenderingContext2D.  We might as well just change the IDL.

[Darin Fisher (:fishd, Google)]

Created attachment 27432 [details]
v1 patch

[Darin Adler]

Comment on attachment 27432 [details]
v1 patch

Long term, this is wrong. The context could be other classes depending on the passed-in context ID. So we may want to make a different type of change here.

[Eric Seidel (no email)]

Comment on attachment 27432 [details]
v1 patch

Agreed.  This would make it impossible to implement Context3D for example (which I figure we'll eventually do to match moz and opera)

[Darin Fisher (:fishd, Google)]

Yeah, that's correct, but we don't have any Context3D yet.  When we do, we can figure out how to abstract this return value.  Why figure that out now?  Seems like premature effort to me.

My change just moves "hard coded logic" from the code generator into the IDL.  That doesn't seem like a step back to me :-)

[Darin Fisher (:fishd, Google)]

Would you guys prefer if the getContext return type were changed to CanvasRenderingContext instead of CanvasRenderingContext2D?  I could then add a CanvasRenderingContext header that includes the typedef that currently lives in CanvasRenderingContext2D.h.  Would that be better?  I'm striving for avoiding hacks in the code generator :)

[Eric Seidel (no email)]

Comment on attachment 27432 [details]
v1 patch

I agree, it's overkill.  The header with a separate CanvasRenderingContext would be cleanest, but we can solve that when we get a Context3d.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/40797

I left the patch as-is.  It turns out that toString() currently returns "[object CanvasRenderingContext2D]", so the v1 patch also has the property of making the IDL match the JS better.

[Darin Fisher (:fishd, Google)]

Re-opening since Sam didn't like this fix.

Reverted as:
http://trac.webkit.org/changeset/40810

[Darin Fisher (:fishd, Google)]

Closing this out in favor of the solution in bug 24311.