Bug 175400

Created attachment 317769 [details] Patch

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

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

Created attachment 318714 [details] Patch

Comment on attachment 318714 [details] Patch r=me

Created attachment 318728 [details] Patch

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 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 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 on attachment 318728 [details] Patch Clearing flags on attachment: 318728 Committed r221025: <http://trac.webkit.org/changeset/221025>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34016660>