Bug 175400 – Web Inspector: provide way for ShaderPrograms to be enabled/disabled

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This will allow/prevent any visual effect from being shown for the enabled/disabled ShaderProgram.

[Devin Rousso]

Created attachment 317769 [details]
Patch

[Devin Rousso]

Created attachment 317770 [details]
[Video] After Patch is applied

[Matt Baker]

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.

[Devin Rousso]

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

[Devin Rousso]

Created attachment 318714 [details]
Patch

[Matt Baker]

Comment on attachment 318714 [details]
Patch

r=me

[Devin Rousso]

Created attachment 318728 [details]
Patch

[Matt Baker]

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.

[Devin Rousso]

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.

[Matt Baker]

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!

[WebKit Commit Bot]

Comment on attachment 318728 [details]
Patch

Clearing flags on attachment: 318728

Committed r221025: <http://trac.webkit.org/changeset/221025>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34016660>