Summary: | [WebIDL] Remove custom bindings for HTMLCanvasElement | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Sam Weinig
2017-07-25 17:26:29 PDT
Created attachment 316412 [details]
Patch
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. Created attachment 316413 [details]
Patch
Comment on attachment 316413 [details]
Patch
My comments are on the last version of the patch.
(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. Committed r219902: <http://trac.webkit.org/changeset/219902> |