[webkit-reviews] review granted: [Bug 174847] [WebIDL] Remove custom bindings for HTMLCanvasElement : [Attachment 316412] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 174847: [WebIDL] Remove custom bindings for HTMLCanvasElement
https://bugs.webkit.org/show_bug.cgi?id=174847

Attachment 316412: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 316412
  --> https://bugs.webkit.org/attachment.cgi?id=316412
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.