Bug 124211 – Web Inspector: Canvas: support editing WebGL shaders

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

[Timothy Hatcher]

For WebGL support we are likely going to show the shaders in the Resources sidebar. We should make them editable too.

See: https://hacks.mozilla.org/2013/11/live-editing-webgl-shaders-with-firefox-developer-tools/

[Radar WebKit Bug Importer]

<rdar://problem/15448958>

[Devin Rousso]

Created attachment 317156 [details]
Patch

[Devin Rousso]

Created attachment 317301 [details]
Patch

[Matt Baker]

Comment on attachment 317301 [details]
Patch

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

> LayoutTests/inspector/canvas/updateShader.html:31
> +        description: "Tests updating a program's vertex shader.",

Nit: "Tests updating a program's vertex shader source."

> LayoutTests/inspector/canvas/updateShader.html:45
> +`;

Style: the trailing "`;" should start in the same column as "const source =`".

> LayoutTests/inspector/canvas/updateShader.html:53
> +    suite.addTestCase({

These two test cases could share code, parameterized on shaderType.

> LayoutTests/inspector/canvas/updateShader.html:152
> +    suite.runTestCasesAndFinish();

It would be ideal if the tests could be written against the model/controller layer, instead of the protocol. Using ShaderProgram's update shader methods would exercise both the frontend and protocol. I can see why this might have been necessary, since ShaderProgram doesn't retain errors returned from CanvasAgent or expose them to the outside.

It might be worth looking into making a change to ShaderProgram, and adding an optional callback to the shader update methods to be notified of success/failure info. What do you think?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:314
> +        errorString = contextWebGL->getShaderInfoLog(shader);

Does it matter that we now have two paths for sending shader/linker errors to the frontend? Do they send identical data? Eventually we'll want to show errors inline, in the shader's TextEditor. Where would the error data be taken from to do this? The Console? The Canvas protocol?

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

This patch is introducing new UI, which warrants a brief summary.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:87
> +            console.assert(!error, error);

Shouldn't we do something with errors, or lack there of? The program could track the result of the last operation, and invalidate it once another update is performed. Currently one has to toggle between the Console and Resources tab to see if the edits succeeded.

[Matt Baker]

Do you have a test for two shaders which both compile, but cannot be linked? An example of this is mismatched varying variables.

[Devin Rousso]

Comment on attachment 317301 [details]
Patch

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

>> LayoutTests/inspector/canvas/updateShader.html:45
>> +`;
> 
> Style: the trailing "`;" should start in the same column as "const source =`".

If I indented the "`;" then there would be a lot of trailing whitespace other than just "\n" at the end of the source.  I would prefer to keep it where it is.

>> LayoutTests/inspector/canvas/updateShader.html:152
>> +    suite.runTestCasesAndFinish();
> 
> It would be ideal if the tests could be written against the model/controller layer, instead of the protocol. Using ShaderProgram's update shader methods would exercise both the frontend and protocol. I can see why this might have been necessary, since ShaderProgram doesn't retain errors returned from CanvasAgent or expose them to the outside.
> 
> It might be worth looking into making a change to ShaderProgram, and adding an optional callback to the shader update methods to be notified of success/failure info. What do you think?

I'm not sure I like the idea of adding an optional since it won't be used in any case other than tests.  We follow this pattern of testing directly on the protocol in many other tests.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:314
>> +        errorString = contextWebGL->getShaderInfoLog(shader);
> 
> Does it matter that we now have two paths for sending shader/linker errors to the frontend? Do they send identical data? Eventually we'll want to show errors inline, in the shader's TextEditor. Where would the error data be taken from to do this? The Console? The Canvas protocol?

I will just use the console to send errors.  Linking the error line/column to the TextEditor will come in a later patch.

>> Source/WebInspectorUI/ChangeLog:7
>> +        Reviewed by NOBODY (OOPS!).
> 
> This patch is introducing new UI, which warrants a brief summary.

This patch doesn't actually introduce any new UI.  It just makes the existing shader TextEditors editable.  Also, I think the title is pretty self-explanatory.

>> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:87
>> +            console.assert(!error, error);
> 
> Shouldn't we do something with errors, or lack there of? The program could track the result of the last operation, and invalidate it once another update is performed. Currently one has to toggle between the Console and Resources tab to see if the edits succeeded.

I will be adding inline errors in a later patch.

[Devin Rousso]

Created attachment 317649 [details]
Patch

[Matt Baker]

Comment on attachment 317649 [details]
Patch

r=me

[Devin Rousso]

Created attachment 317662 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 317662 [details]
Patch

Clearing flags on attachment: 317662

Committed r220436: <http://trac.webkit.org/changeset/220436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.