Bug 174847 – [WebIDL] Remove custom bindings for HTMLCanvasElement

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

[WebIDL] Remove custom bindings for HTMLCanvasElement

[Sam Weinig]

Created attachment 316412 [details]
Patch

[Darin Adler]

Comment on attachment 316412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316412&action=review

Apparently we don’t have enough included for WPE to build successfully.

> Source/WebCore/html/HTMLCanvasElement.cpp:70
> +#if ENABLE(WEBGL2)
> +#include "WebGL2RenderingContext.h"
> +#endif

Can. we put this in a second paragraph rather than nesting it inside #if ENABLE(WEBGL). The separate paragraph can have #if ENABLE(WEBGL) && ENABLE(WEBGL2) or just #if ENABLE(WEBGL2), but would be nicer without the nesting.

> Source/WebCore/html/HTMLCanvasElement.cpp:237
> +        auto scope = DECLARE_THROW_SCOPE(state.vm());
> +        auto attributes = convert<IDLDictionary<WebGLContextAttributes>>(state, !arguments.isEmpty() ? arguments[0].get() : JSC::jsUndefined());
> +        RETURN_IF_EXCEPTION(scope, Exception { ExistingExceptionError });

This is so sad. It’s like we had to move the bindings into the DOM, rather than automatically generating them. I don’t want to overreact though, since there are a lot of benefits to the simplification.

[Sam Weinig]

Created attachment 316413 [details]
Patch

[Darin Adler]

Comment on attachment 316413 [details]
Patch

My comments are on the last version of the patch.

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 316412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316412&action=review
> 
> Apparently we don’t have enough included for WPE to build successfully.
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:70
> > +#if ENABLE(WEBGL2)
> > +#include "WebGL2RenderingContext.h"
> > +#endif
> 
> Can. we put this in a second paragraph rather than nesting it inside #if
> ENABLE(WEBGL). The separate paragraph can have #if ENABLE(WEBGL) &&
> ENABLE(WEBGL2) or just #if ENABLE(WEBGL2), but would be nicer without the
> nesting.

Done.

> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:237
> > +        auto scope = DECLARE_THROW_SCOPE(state.vm());
> > +        auto attributes = convert<IDLDictionary<WebGLContextAttributes>>(state, !arguments.isEmpty() ? arguments[0].get() : JSC::jsUndefined());
> > +        RETURN_IF_EXCEPTION(scope, Exception { ExistingExceptionError });
> 
> This is so sad. It’s like we had to move the bindings into the DOM, rather
> than automatically generating them. I don’t want to overreact though, since
> there are a lot of benefits to the simplification.

Yeah, this is a rare case where WebIDL is not expressive enough and the spec has specific language on what to do. I've opted to move this one into the implementation, since it's so small, but it is conversation that needs to be had as we move toward the final pieces. I will probably come back to this one as we implement more of getContext() (We only implement a bit of it; there are other types of contexts and other attribute dictionaries) and see if there is a generalization we can do that makes sense.

Thanks for the review.

[Sam Weinig]

Committed r219902: <http://trac.webkit.org/changeset/219902>