[webkit-reviews] review granted: [Bug 201470] Web Inspector: unify the interaction of show/hide status icons in Sources and Canvas : [Attachment 378006] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 201470: Web Inspector: unify the interaction of show/hide status icons in
Sources and Canvas
https://bugs.webkit.org/show_bug.cgi?id=201470

Attachment 378006: Patch

https://bugs.webkit.org/attachment.cgi?id=378006&action=review




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 378006
  --> https://bugs.webkit.org/attachment.cgi?id=378006
Patch

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

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:61
> +	   CanvasAgent.setShaderProgramDisabled(this._identifier, disabled,
(error) => {

We should set `this._disabled` immediately.

Currently if someone set the state rapidly before a response was received:

    shaderProgram.disabled = true;
    shaderProgram.disabled = false; // Wouldn't do anything.

Then the 2nd setting would potentially be missed and the frontend would leave
the program in a disabled state, which would not be what we actually want.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:57
> +	   super.detach();

This should be `super.ondetach()`

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:63
>      canSelectOnMouseDown(event)
>      {
> -	   return !this._statusElement.contains(event.target);
> +	   return !this._disabledImageElement.contains(event.target);
> +    }

Given the superclass implementation is TreeElement, maybe this should call the
super class if the specific case here is not true.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTreeElement.js:141
> +    populateContextMenu(contextMenu, event)
> +    {
> +	   if (this._sourceCode.supportsScriptBlackboxing) {
> +	       let isBlackboxed =
WI.debuggerManager.isScriptBlackboxed(this._sourceCode);

Does this show up very high in the context menu or rather low?