This code needs to be spruced up. We are using too many indentation levels. :)
Created attachment 230501 [details] the patch
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.
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.
Committed r168040: <http://trac.webkit.org/changeset/168040>