Bug 132387 – Web Inspector: clean up and decompose InspectorBackend functionality

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

This code needs to be spruced up. We are using too many indentation levels. :)

[Brian Burg]

Created attachment 230501 [details]
the patch

[Joseph Pecoraro]

Comment on attachment 230501 [details]
the patch

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.

[Brian Burg]

Comment on attachment 230501 [details]
the patch

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

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

I wasn't sure which is preferred. For this one, I agree a map will be better, since there's no "shape" and the code can create sparse arrays if some commands have no callbacks.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:41
>> +    this._agents = {};
> 
> This could probably also be a map.

OTOH, the list of agents is monotonic and we never delete anything.

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

OK, true.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:138
>> +        --this._pendingResponsesCount;
> 
> Nit: console.assert(this._pendingResponsesCount >= 0)

OK

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

Oops, didn't really grasp what it was doing.

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

It does: qualifiedName is Domain.EventName.

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

I don't like it either. That's the 5th name I tried.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:387
>>              return parameter["name"] === parameterName
> 
> Style: Why the local var instance at all? Also missing a semicolon in the inner statement.

So that all methods in BackendCommand are consistently using instance rather than this._instance.

[Brian Burg]

Committed r168040: <http://trac.webkit.org/changeset/168040>