[webkit-reviews] review granted: [Bug 132387] Web Inspector: clean up and decompose InspectorBackend functionality : [Attachment 230501] the patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg
<burg at cs.washington.edu>'s request for review:
Bug 132387: Web Inspector: clean up and decompose InspectorBackend
functionality
https://bugs.webkit.org/show_bug.cgi?id=132387

Attachment 230501: the patch
https://bugs.webkit.org/attachment.cgi?id=230501&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=230501&action=review


Difficult to read the diff with it being scattered red/green. Not much that can
be done about that given the diff algorithm. But looks good. r=me

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:40
> +    this._callbackData = {};

I wonder if we should make this a Map instead of an object. Oliver keeps
telling me that `delete` makes objects slow. Well, with a Map has built in
set/delete methods.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:41
> +    this._agents = {};

This could probably also be a map.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:63
> +	   agent.addEnum(new InspectorBackend.Enum(enumName, enumValues));

Nit: Seems kind of a waste to construct this object, which just immediately
gets garbage collected. Unlike the others, this one doesn't stay around.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:122
> +	       var callbackData =  {

Style: double space

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:138
> +	   --this._pendingResponsesCount;

Nit: console.assert(this._pendingResponsesCount >= 0)

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:164
> +	       callbackArguments.unshift(messageObject["error"] ?
messageObject["error"].message : null);

Instead of this being an unshift after pushing all the result pieces we should
make pushing the error / null the first push. In testing that can would speed
up this array creation by ~2x.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:198
> +	       console.error("Protocol Error: Attempted to dispatch an
unspecified method '" + qualifiedName + "'");

Nit: Could include the domain it tried to evalute the method on.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:233
> +	   messageObject["id"] = this._registerSentCommand(command, callback);

Nit: "_registerSentCommand" seems like the wrong name for this function since
it is happening before we are sending. Some alternatives,
"this._sequenceCommandForBackend" or "this._willSendMessageToBackend".

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:387
>	   var instance = this._instance;
> -	   return instance._callSignature.any(function(parameter) {
> +	   return instance.callSignature.any(function(parameter) {
>	       return parameter["name"] === parameterName

Style: Why the local var instance at all? Also missing a semicolon in the inner
statement.