Bug 174847

Summary: [WebIDL] Remove custom bindings for HTMLCanvasElement
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review+

Description Sam Weinig 2017-07-25 17:26:29 PDT
[WebIDL] Remove custom bindings for HTMLCanvasElement
Comment 1 Sam Weinig 2017-07-25 17:28:12 PDT
Created attachment 316412 [details]
Patch
Comment 2 Darin Adler 2017-07-25 18:09:42 PDT
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.
Comment 3 Sam Weinig 2017-07-25 18:10:40 PDT
Created attachment 316413 [details]
Patch
Comment 4 Darin Adler 2017-07-25 18:12:20 PDT
Comment on attachment 316413 [details]
Patch

My comments are on the last version of the patch.
Comment 5 Sam Weinig 2017-07-25 21:30:40 PDT
(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.
Comment 6 Sam Weinig 2017-07-25 21:31:43 PDT
Committed r219902: <http://trac.webkit.org/changeset/219902>