[webkit-reviews] review granted: [Bug 179070] Web Inspector: Canvas Tab: show supported GL extensions for selected canvas : [Attachment 325702] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 179070: Web Inspector: Canvas Tab: show supported GL extensions for
selected canvas
https://bugs.webkit.org/show_bug.cgi?id=179070

Attachment 325702: Patch

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




--- Comment #6 from Brian Burg <bburg at apple.com> ---
Comment on attachment 325702
  --> https://bugs.webkit.org/attachment.cgi?id=325702
Patch

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

r=me, looks awesome! You may want to adjust the bug title and changelog to
reflect what the patch actually does, though.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1084
> +#define ENABLE_EXTENSION(type, variable, nameLiteral, ternaryCondition) \

This is a great cleanup, but the macro name is nonsensical as it conditionally
enables an extension, but this reads as if we are enabling all extensions.
Maybe ENABLE_EXTENSION_IF_REQUESTED(...).

I also think ternaryCondition is a bit obtuse, perhaps it reads better as
"canEnable":

variable = (canEnable) ? std::make_unique<type>(*this) : nullptr;

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:103
> +	   if (is<WebGLRenderingContextBase>(context)) {

Does this handle WebGL2 as well?

> LayoutTests/inspector/canvas/extensions-expected.txt:1
> +Test that CanvasManager tracks canvas memory costs and is notified of
changes.

Nope, lol. Please add subtly different boilerplate text.

> LayoutTests/inspector/canvas/extensions.html:48
> +    <p>Test that CanvasManager tracks canvas memory costs and is notified of
changes.</p>

Ditto.