Bug 179070

Summary: Web Inspector: Canvas Tab: show activated GL extensions for selected canvas
Product: WebKit Reporter: Brian Burg <bburg>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, buildbot, cdumez, commit-queue, dino, drousso, esprehn+autocc, graouts, gyuyoung.kim, inspector-bugzilla-changes, joepeck, keith_miller, kondapallykalyan, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 175485, 179205    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
Patch none

Description Brian Burg 2017-10-31 12:43:04 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-10-31 12:43:37 PDT
<rdar://problem/35278276>
Comment 2 Devin Rousso 2017-10-31 13:18:05 PDT
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.
Comment 3 Devin Rousso 2017-11-02 03:42:05 PDT
Created attachment 325702 [details]
Patch

Too tired to add comments to ChangeLog :(
Comment 4 Devin Rousso 2017-11-02 03:43:11 PDT
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 ¯\_(ツ)_/¯
Comment 5 Brian Burg 2017-11-02 09:48:14 PDT
(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.
Comment 6 Brian Burg 2017-11-02 10:51:36 PDT
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.
Comment 7 Brian Burg 2017-11-02 10:53:18 PDT
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.
Comment 8 Devin Rousso 2017-11-02 13:41:57 PDT
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 :(
Comment 9 Devin Rousso 2017-11-02 15:42:17 PDT
(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>
Comment 10 Joseph Pecoraro 2017-11-02 15:42:53 PDT
Neat! What does it look like when there are no extensions?
Comment 11 Devin Rousso 2017-11-02 15:49:14 PDT
Created attachment 325785 [details]
Patch
Comment 12 WebKit Commit Bot 2017-11-02 17:54:42 PDT
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
Comment 13 Devin Rousso 2017-11-02 18:10:10 PDT
Created attachment 325808 [details]
Patch
Comment 14 WebKit Commit Bot 2017-11-02 18:53:21 PDT
Comment on attachment 325808 [details]
Patch

Clearing flags on attachment: 325808

Committed r224370: <https://trac.webkit.org/changeset/224370>
Comment 15 WebKit Commit Bot 2017-11-02 18:53:23 PDT
All reviewed patches have been landed.  Closing bug.