[webkit-reviews] review granted: [Bug 17240] Web Inspector: implement blackboxing of script resources : [Attachment 377655] 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 17240: Web Inspector: implement blackboxing of script resources
https://bugs.webkit.org/show_bug.cgi?id=17240

Attachment 377655: Patch

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




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

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

r=me!

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTreeElement.js:168
> +	      
WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.BlackboxedURLsChan
ged, this._updateToggleBlackboxImageElementState, this);

It looks like this (updateStatus) can get called multiple times and might add
multiple event listeners.

I also don't see this event listener getting removed, so these would leak.

Perhaps this should create the image element, listener, and set the status
once. Then just call update in the general case.

> LayoutTests/inspector/debugger/setShouldBlackboxURL.html:63
> +	       InspectorProtocol.sendCommand(`Debugger.resume`, {},
function(response) {
> +		   InspectorProtocol.checkForError(response);
> +	       });

I think this pattern can be written in a few places like so:

    InspectorProtocol.sendCommand(`Debugger.resume`, {},
InspectorProtocol.checkForError);

Perhaps even making a `InspectorProtocol` convenience:

    InspectorProtocol.sendCommandAndCheckForError(`Debugger.resume`);