Bug 138593 – Web Inspector: add source view for WebGL shader programs

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Selecting canvas shader programs from the resources sidebar should bring up side-by-side source code views displaying the vertex and fragment shader source. Live editing of shader source should be possible, with compilation errors displayed in the editor.

[Radar WebKit Bug Importer]

<rdar://problem/18936194>

[Devin Rousso]

Created attachment 316816 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

This will need to be rebased once https://bugs.webkit.org/show_bug.cgi?id=172624 lands.

[Devin Rousso]

Created attachment 317180 [details]
Patch

[Devin Rousso]

Created attachment 317211 [details]
Patch

[Build Bot]

Comment on attachment 317211 [details]
Patch

Attachment 317211 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4250843

New failing tests:
inspector/canvas/requestShaderSource.html

[Build Bot]

Created attachment 317214 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 317217 [details]
Patch

[Matt Baker]

Comment on attachment 317217 [details]
Patch

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

r=me, with comments.

> Source/WebInspectorUI/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Please add a short summary of the frontend bits.

> Source/WebCore/inspector/InspectorShaderProgram.cpp:69
> +        return nullptr;

This should never happen so we should assert:

else {
    ASSERT_NOT_REACHED();
    return nullptr;
}

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:58
> +    }

Style: newline after.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:55
> +    padding: 0.1em 0.5em;

Nit: we usually use pixels for margin/padding. Our layout doesn't adapt to font size, and ems are convenient but more difficult to reason about.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:61
> +    top: 1.5em;

See above.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:48
> +

Nit: remove space.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:51
> +                break;

Nit: space after.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:54
> +

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:66
> +

Remove space.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:93
> +        super.hidden();

In general calls to super should always be first, unless there is good reason.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:101
> +        super.closed();

Ditto.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:124
> +    get supportsSearch()

Inline.

> LayoutTests/inspector/canvas/requestShaderSource.html:61
> +    // ------

Remove comment.

[Devin Rousso]

Comment on attachment 317217 [details]
Patch

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

>> Source/WebCore/inspector/InspectorShaderProgram.cpp:69
>> +        return nullptr;
> 
> This should never happen so we should assert:
> 
> else {
>     ASSERT_NOT_REACHED();
>     return nullptr;
> }

I didn't do this because then one of the tests I wrote fails.  I don't think it's necessary to have this assert.  It's already possible for this to return a nullptr if no shaders have been attached, so I think that callers will already have to check for nullptr.

>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:93
>> +        super.hidden();
> 
> In general calls to super should always be first, unless there is good reason.

I typically put hidden/closed super calls last in case they do something to the element that I don't expect.  I follow a general->specific approach

super.shown()
this.shown()
...
this.hidden()
super.hidden()

>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:124
>> +    get supportsSearch()
> 
> Inline.

I thought we only did this for obvious getters/setters, such as ones that assign/return a variable of the same name?  I'm fine either way though.

>> LayoutTests/inspector/canvas/requestShaderSource.html:61
>> +    // ------
> 
> Remove comment.

I've done this in most of my tests to distinguish that the tests below the line are for error cases.

[Matt Baker]

Comment on attachment 317217 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:124
>>> +    get supportsSearch()
>> 
>> Inline.
> 
> I thought we only did this for obvious getters/setters, such as ones that assign/return a variable of the same name?  I'm fine either way though.

Our criteria in the past has been whether the body of the getter is trivial/does work. It is subjective, but usually we consider getters that don't call class methods or methods on members trivial.

>>> LayoutTests/inspector/canvas/requestShaderSource.html:61
>>> +    // ------
>> 
>> Remove comment.
> 
> I've done this in most of my tests to distinguish that the tests below the line are for error cases.

I don't think it is needed, but if you feel strongly about it at least mention that in the comment. As is, this looks like it was included by accident.

[Devin Rousso]

Created attachment 317265 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 317265 [details]
Patch

Clearing flags on attachment: 317265

Committed r220294: <http://trac.webkit.org/changeset/220294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 317265 [details]
Patch

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

Sweet!

> Source/WebInspectorUI/Scripts/update-codemirror-resources.rb:44
> +  mode/clike/clike.js

Nice!! I didn't think anyone else knew about this script! =)

[Ebrahim Byagowi]

This looks just great. Thanks!

Should it enabled by a build flag or something? I couldn't find the feature on the place depicted on the screenshot on Safari Technology Preview 45 on http://acko.net or https://ebraminio.github.io/extra/WebGL1.html so some help would be nice I guess

[Ebrahim Byagowi]

Guys, any pointer on how to see result of this? I still can't see result of this on "Release 47 (Safari 11.1, WebKit 13605.1.19.1)" maybe some build or runtime flag is needed?

[Matt Baker]

(In reply to Ebrahim Byagowi from comment #18)
> Guys, any pointer on how to see result of this? I still can't see result of
> this on "Release 47 (Safari 11.1, WebKit 13605.1.19.1)" maybe some build or
> runtime flag is needed?

Viewable shader programs were broken but are being brought back in <https://webkit.org/b/178744>. This should be available in the next Safari Technology Preview (release 50).

[Ebrahim Byagowi]

Tested this yesterday with `brew cask reinstall webkit-build-archive`, works flawlessly, even doesn't need page reload and works great. Thanks!