[webkit-reviews] review granted: [Bug 200384] Web Inspector: rework frontend agent construction to allow commands/events to be controlled by the related target's type : [Attachment 381115] 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 200384: Web Inspector: rework frontend agent construction to allow
commands/events to be controlled by the related target's type
https://bugs.webkit.org/show_bug.cgi?id=200384

Attachment 381115: Patch

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




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

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

r=me, Awesome work!

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:86
> -    return Protocol::Target::TargetInfo::Type::JavaScript;
> +    return Protocol::Target::TargetInfo::Type::Page;

Still seems like we should return `JavaScript` in the default case, since that
should be guaranteed to work no matter what: Runtime/Debugger/Console domains
are always supplied.

> Source/JavaScriptCore/inspector/scripts/codegen/models.py:49
> +	   elif target_type == 'page' or target_type == 'worker':
> +	       if not 'page' in debuggable_types:
> +		   return False

Nice =)

> Source/WebInspectorUI/UserInterface/Base/Multimap.js:76
> +	   for (let value of iterable)
> +	       valueSet.add(value);

Given that `delete` generally deletes the set if there are no more values left,
what would you expect to happen if the iterable has no values? Perhaps we
should just assert that iterable is not empty (or that `valueSet.size` is
non-zero at the end).

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:1290
> -	   if (event.data.domains.includes("Page") && window.PageAgent)
> -	      
PageAgent.getResourceTree(this._processMainFrameResourceTreePayload.bind(this))
;
> +	   let target = WI.assumingMainTarget();
> +	   if (target.hasDomain("Page"))
> +	      
target.PageAgent.getResourceTree(this._processMainFrameResourceTreePayload.bind
(this));

This lost the `event.data.domains.includes("Page")` check

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:91
> +	   if (messageObject["error"] && messageObject["error"].code !==
-32000)
> +	       console.error("Request with id = " + messageObject["id"] + "
failed. " + JSON.stringify(messageObject["error"]));

Lets just update these property accesses to `messageObject.error` instead of
`messageObject["error"]`

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:154
>	   let qualifiedName = messageObject["method"];

Lets just update this property access to `messageObject.method` instead of
`messageObject["method"]`

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:66
> +    get Enum()
>      {
> -	   return this._agents;
> +	   return this._activeDomains;
>      }

I wonder if this should use `_registeredDomains`. Any enum checks we make using
this probably don't need to be gated on whether or a domain is active. Though
in practice it should always be active if we are looking at an Enum value for
the domain.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:160
> +	   let [domainName, enumName] = qualifiedName.split(".", 2);

Does the `2` add anything to any of these? The `register` calls should always
have exactly the right number of periods. So I think this is just clutter.
Unless you can show this speeds things up I'd just drop them.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:253
> +	   return domain.VERSION;

>From Devin: `|| -Infinity`

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:273
> +    _domainForName(domainName)
> +    {
> +	   if (!this._registeredDomains[domainName])
> +	       this._registeredDomains[domainName] = new
InspectorBackend.Domain(domainName);
> +	   return this._registeredDomains[domainName];
> +    }

Now that we have a `registerDomain` for each of the domains, it seems like we
should be able to fold this into registerDomain and eliminate it from all other
register functions `registerCommand`, `registerEnum`, `registerEvent`, etc.
Should help streamline initialization!

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:397
>	   if (!this._dispatcher) {
>	       console.error(`No domain dispatcher registered for domain
'${this._domainName}', for event '${this._domainName}.${eventName}'`);
>	       return false;
>	   }
>  
> -	   if (!(eventName in this._dispatcher)) {
> +	   let handler = this._dispatcher[eventName];
> +	   if (!handler) {
>	       console.error(`Protocol Error: Attempted to dispatch an
unimplemented method '${this._domainName}.${eventName}'`);
>	       return false;
>	   }

Will these errors work anymore? it doesn't look like `this._domainName` exists
anymore.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:399
> +	   let params = messageObject["params"] || {};

Lets just update this property access to `messageObject.params` instead of
`messageObject["params"]`

>
Source/WebInspectorUI/UserInterface/Protocol/Legacy/10.0/InspectorBackendComman
ds.js:40
> +InspectorBackend.registerDomain("ApplicationCache", ["page"]);
> +InspectorBackend.registerCommand("ApplicationCache.getFramesWithManifests",
null, [], ["frameIds"]);
> +InspectorBackend.registerCommand("ApplicationCache.enable", null, [], []);
> +InspectorBackend.registerCommand("ApplicationCache.getManifestForFrame",
null, [{"name": "frameId", "type": "string"}], ["manifestURL"]);
>
+InspectorBackend.registerCommand("ApplicationCache.getApplicationCacheForFrame
", null, [{"name": "frameId", "type": "string"}], ["applicationCache"]);
>
+InspectorBackend.registerEvent("ApplicationCache.applicationCacheStatusUpdated
", null, ["frameId", "manifestURL", "status"]);
> +InspectorBackend.registerEvent("ApplicationCache.networkStateUpdated", null,
["isNowOnline"]);
> +InspectorBackend.registerApplicationCacheDispatcher =
InspectorBackend.registerDispatcher.bind(InspectorBackend, "ApplicationCache");
> +InspectorBackend.activateDomain("ApplicationCache", ["page"]);

Gosh... given we are doing this why do we even do the split at all?

We can just generate `registerCommand(domain, commandName, ...)`,
`registerEvent(domain, commandName, ...)`.

I suspect a split is more expensive than a combine for a cache key, but maybe
not.

> Source/WebInspectorUI/UserInterface/Test.html:289
> +	   // Only needed when debugging issues related to the Target domain.
> +	   InspectorBackend.filterMultiplexingBackendInspectorProtocolMessages
= true;

Love it. I don't know why I didn't just do that when first enabling it...

> Source/WebInspectorUI/UserInterface/Views/AuditTabContentView.js:49
> -	   return WI.sharedApp.debuggableType === WI.DebuggableType.Web;
> +	   return WI.sharedApp.debuggableType === WI.DebuggableType.Page ||
WI.sharedApp.debuggableType === WI.DebuggableType.WebPage;

I wonder if we should make a helper for this, since I suspect it will be common
enough that we would want to avoid mistakes and have a function that checks
both. Maybe `WI.sharedApp.isWebDebuggableType()` or
`WI.sharedApp.isPageDebuggableType()` something like that?

    Models/TimelineRecording.js
    75:        return WI.sharedApp.debuggableType === WI.DebuggableType.Web;

    Controllers/TargetManager.js
    105:	if (WI.sharedApp.debuggableType === WI.DebuggableType.Web)

    Views/TimelineRecordingContentView.js
    59:        if (WI.sharedApp.debuggableType === WI.DebuggableType.Web) {

    Views/AuditTabContentView.js
    49:        return WI.sharedApp.debuggableType === WI.DebuggableType.Web;

    Views/Toolbar.js
    104:	if (WI.sharedApp.debuggableType !== WI.DebuggableType.Web) {

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:640
> +	       if (target.hasDomain("Page"))

Should we just upgrade this to `hasCommand("Page.setEmulatedMedia")`?

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:438
> +	       if (target.hasDomain("Page"))

Should we just upgrade this to
`hasCommand("Page.setCompositingBordersVisible")`?

Basically any `for (let target of WI.targets)` loop that immediately has a
`target.hasDomain` check with a single function could probably be upgraded.
That might work out well if ITML starts dropping support for some commands that
it has never supported. I only pointed out a few places where this pattern
happens and we could just upgrade the loop.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:861
> +	       if (target.hasDomain("Heap"))

Seems this should be `hasCommand("Heap.gc")` and not just assume, since we do
that deeper check above.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:403
> +	       if (target.hasDomain("DOM"))

This also seems like an upgradable check to `hasCommand(DOM.markUndoableState)`

> Source/WebInspectorUI/UserInterface/Views/StorageTabContentView.js:51
> +	   return InspectorBackend.hasDomain("ApplicationCache") ||
InspectorBackend.hasDomain("DOMStorage") ||
InspectorBackend.hasDomain("Database") ||
InspectorBackend.hasDomain("IndexedDB");

Style: Now that the individual expressions are larger it might be nice to
spread this out over multiple lines, it'll line up nicely.

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:112
> +	   return InspectorBackend.hasDomain("Timeline") ||
InspectorBackend.hasDomain("ScriptProfiler");

Style: Now that the individual expressions are larger it might be nice to
spread this out over multiple lines, it'll line up nicely.