Bug 179070 – Web Inspector: Canvas Tab: show activated GL extensions for selected canvas

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

I think this could go in the Canvas details sidebar. It's not that important, but worth enumerating somewhere since there is no way to enumerate extensions directly via JS API.

[Radar WebKit Bug Importer]

<rdar://problem/35278276>

[Devin Rousso]

I'm not sure how useful this is.  Listing supported extensions is as simple as calling `getSupportedExtensions()`.

    document.createElement("canvas").getContext("webgl").getSupportedExtensions()

I think it would be more useful to list what extensions are enabled per context.

[Devin Rousso]

Created attachment 325702 [details]
Patch

Too tired to add comments to ChangeLog :(

[Devin Rousso]

Created attachment 325703 [details]
[Image] After Patch is applied

Not the prettiest, I know, but I'm not really sure what UI to use here.  DataGrid doesn't make a lot of sense as every item shown will be true/on, and that seems like a waste.  I figured that this is better than nothing ¯\_(ツ)_/¯

[Blaze Burg]

(In reply to Devin Rousso from comment #2)
> I'm not sure how useful this is.  Listing supported extensions is as simple
> as calling `getSupportedExtensions()`.
> 
>    
> document.createElement("canvas").getContext("webgl").getSupportedExtensions()
> 
> I think it would be more useful to list what extensions are enabled per
> context.

Yes, this sounds more useful.

[Blaze Burg]

Comment on attachment 325702 [details]
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.

[Blaze Burg]

Thinking a bit more, you could easily capture a stack trace for these getExtension calls and put them in the UI. I think this would be reasonable in the "live" case (Web Inspector is open when the calls happen). There's no point trying to save a call stack if Inspector isn't open.

[Devin Rousso]

Comment on attachment 325702 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:103
>> +        if (is<WebGLRenderingContextBase>(context)) {
> 
> Does this handle WebGL2 as well?

Yes this works for both.  `getSupportedExtensions` is virtual and implemented by each subclass, and `extensionIsEnabled` is defined on WebGLRenderingContextBase.

>> 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.

I did mention this when I uploaded the patch :P

> (In reply to Devin Rousso from comment #3)
>> Created attachment 325702 [details]
>> Patch
>> 
>> Too tired to add comments to ChangeLog :(

[Devin Rousso]

(In reply to Brian Burg from comment #7)
> Thinking a bit more, you could easily capture a stack trace for these
> getExtension calls and put them in the UI. I think this would be reasonable
> in the "live" case (Web Inspector is open when the calls happen). There's no
> point trying to save a call stack if Inspector isn't open.
I don't think that a backtrace for `getExtension` is really all that useful, since AFAIK an enabled extension doesn't really do anything by itself.  Furthermore, the user could just as easily put a breakpoint on any `getExtension` line and see it that way.  I'm happy to add it, but I am skeptical of how useful it would be (and how to present it nicely in the UI).

Regardless, I think it should be a separate change. <https://webkit.org/b/179205>

[Joseph Pecoraro]

Neat! What does it look like when there are no extensions?

[Devin Rousso]

Created attachment 325785 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325785 [details]
Patch

Rejecting attachment 325785 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 325785, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
LayoutTests
:040000 040000 fa6a38837d8e81970cb71cacb32c83aa9dbe0e07 fbe2d210b97d5f1faa5461bbb7037ec9426da33a M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/5082908

[Devin Rousso]

Created attachment 325808 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325808 [details]
Patch

Clearing flags on attachment: 325808

Committed r224370: <https://trac.webkit.org/changeset/224370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.