Bug 175400 - Web Inspector: provide way for ShaderPrograms to be enabled/disabled
Summary: Web Inspector: provide way for ShaderPrograms to be enabled/disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 138593
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-09 15:58 PDT by Devin Rousso
Modified: 2017-08-22 11:11 PDT (History)
15 users (show)

See Also:


Attachments
Patch (23.13 KB, patch)
2017-08-09 18:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Video] After Patch is applied (19.41 MB, video/quicktime)
2017-08-09 18:12 PDT, Devin Rousso
no flags Details
Patch (26.98 KB, patch)
2017-08-21 18:12 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (27.11 KB, patch)
2017-08-21 20:38 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-08-09 15:58:47 PDT
This will allow/prevent any visual effect from being shown for the enabled/disabled ShaderProgram.
Comment 1 Devin Rousso 2017-08-09 18:11:27 PDT
Created attachment 317769 [details]
Patch
Comment 2 Devin Rousso 2017-08-09 18:12:36 PDT
Created attachment 317770 [details]
[Video] After Patch is applied
Comment 3 Matt Baker 2017-08-12 09:10:05 PDT
Comment on attachment 317769 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLProgram.h:60
> +    void setDisabled(bool disabled) { m_disabled = disabled; }

We should avoid adding Inspector logic and data to classes in WebCore, unless it's totally unavoidable. In this case we could keep all this state in InspectorCanvasAgent and add a InspectorInstrumentation::isShaderProgramDisabled method. The instrumentation plumbing is inconvenient, but I think it is worth it.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:334
> +    inspectorProgram->program().setDisabled(disabled);

Let's move disabled state into InspectorShaderProgram. See comment above.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:36
> +

Remove blank line.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.css:37
> +    filter: invert();

Cool!

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:56
> +            this._listItemNode.classList.toggle("disabled", !!this.representedObject.disabled);

Doesn't ShaderProgram.disabled always return a bool? The !! shouldn't be necessary.

> LayoutTests/inspector/canvas/setShaderProgramDisabled-expected.txt:6
> +PASS: Disabled shader programs should have no visual effect.

What about "Disabled shader programs should not draw."?

> LayoutTests/inspector/canvas/setShaderProgramDisabled.html:103
> +                                InspectorTest.expectEqual(content, originalContent, "Disabled shader programs should have no visual effect.");

This is testing a single program, and should be singular: "Disabled shader program should...".

> LayoutTests/inspector/canvas/setShaderProgramDisabled.html:105
> +                                InspectorTest.expectNotEqual(content, originalContent, "Re-enabled shader programs should have a visual effect.");

Singular.
Comment 4 Devin Rousso 2017-08-21 18:11:53 PDT
Comment on attachment 317769 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:56
>> +            this._listItemNode.classList.toggle("disabled", !!this.representedObject.disabled);
> 
> Doesn't ShaderProgram.disabled always return a bool? The !! shouldn't be necessary.

This is just a precaution against undefined behavior.  I've run into problems with classList.toggle in the past due to the fact that passing `undefined` causes the className to invert itself (remove if exists, add if not).  It was a nightmare to fix that bug, so I'd rather keep this to avoid that headache (easier to see that the className never gets added rather than it sometimes flip-flops).
Comment 5 Devin Rousso 2017-08-21 18:12:27 PDT
Created attachment 318714 [details]
Patch
Comment 6 Matt Baker 2017-08-21 18:39:27 PDT
Comment on attachment 318714 [details]
Patch

r=me
Comment 7 Devin Rousso 2017-08-21 20:38:27 PDT
Created attachment 318728 [details]
Patch
Comment 8 Matt Baker 2017-08-22 10:25:58 PDT
Comment on attachment 318728 [details]
Patch

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

> LayoutTests/inspector/canvas/setShaderProgramDisabled.html:144
> +            CanvasAgent.setShaderProgramDisabled(programId, disabled, (error) => {

Don't we prefer testing models/controllers rather than directly calling protocol methods? Testing ShaderProgram.disabled and ShaderProgram.toggleDisabled would implicitly exercise the protocol and cover the new ShaderProgram functionality.
Comment 9 Devin Rousso 2017-08-22 10:45:00 PDT
Comment on attachment 318728 [details]
Patch

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

>> LayoutTests/inspector/canvas/setShaderProgramDisabled.html:144
>> +            CanvasAgent.setShaderProgramDisabled(programId, disabled, (error) => {
> 
> Don't we prefer testing models/controllers rather than directly calling protocol methods? Testing ShaderProgram.disabled and ShaderProgram.toggleDisabled would implicitly exercise the protocol and cover the new ShaderProgram functionality.

From what I've seen, we typically hide the error message in the model/controller, so it makes tests like this not very useful as the error message is not retrievable.  I am open to changing this though.
Comment 10 Matt Baker 2017-08-22 10:51:52 PDT
Comment on attachment 318728 [details]
Patch

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

>>> LayoutTests/inspector/canvas/setShaderProgramDisabled.html:144
>>> +            CanvasAgent.setShaderProgramDisabled(programId, disabled, (error) => {
>> 
>> Don't we prefer testing models/controllers rather than directly calling protocol methods? Testing ShaderProgram.disabled and ShaderProgram.toggleDisabled would implicitly exercise the protocol and cover the new ShaderProgram functionality.
> 
> From what I've seen, we typically hide the error message in the model/controller, so it makes tests like this not very useful as the error message is not retrievable.  I am open to changing this though.

Okay that's reasonable!
Comment 11 WebKit Commit Bot 2017-08-22 11:08:54 PDT
Comment on attachment 318728 [details]
Patch

Clearing flags on attachment: 318728

Committed r221025: <http://trac.webkit.org/changeset/221025>
Comment 12 WebKit Commit Bot 2017-08-22 11:08:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-08-22 11:11:12 PDT
<rdar://problem/34016660>