Bug 200384 – Web Inspector: rework frontend agent construction to allow commands/events to be controlled by the related target's type

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

[Devin Rousso]

`InspectorBackend.domains.${domain}` isn't a truly valid way to feature check, as it indicates what's supported by the WebKit framework underlying whatever's currently being inspected, not what the current inspection target supports.

On iOS, for example, inspecting a `JSContext` will still show `InspectorBackend.domains.DOM` as the `DOMAgent` is supported by WebKit, even though `JSContext`s have no concept of the DOM.  In this example, however, `window.DOMAgent` would NOT exist, as the `availability` check for the `DOM` domain wouldn't pass, meaning that the agent never gets connected.

In order to do more proper feature checking, we should go back to separate InspectorBackendCommands.js for each debuggable type ("itml", "javascript", "web", "service-worker"), and only include what's actually supported by an inspected target of that type.

[Radar WebKit Bug Importer]

<rdar://problem/53850352>

[Devin Rousso]

Created attachment 375398 [details]
[Patch] WIP

[EWS Watchlist]

Attachment 375398 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 91 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 375400 [details]
[Patch] WIP

[EWS Watchlist]

Attachment 375400 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 91 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 375403 [details]
[Patch] WIP

[EWS Watchlist]

Attachment 375403 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 91 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 375403 [details]
[Patch] WIP

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

As far as I can tell, debuggableType == targetType for all domains. What is the motivation for making them separate?

> Source/JavaScriptCore/inspector/protocol/DOM.json:-71
> -        {

Why is this removed?

> Source/JavaScriptCore/inspector/protocol/DOM.json:-269
> -            "name": "getDataBindingsForNode",

Ditto

> Source/WebKit/InspectorGResources.cmake:-39
> -    # DerivedSources/JavaScriptCore/inspector/InspectorBackendCommands.js is

You should make a function to reduce the boilerplate here.

> Source/WebKit/PlatformWin.cmake:92
> +# DerivedSources/JavaScriptCore/inspector/InspectorBackendCommands_itml.js is

You should make a function to reduce the boilerplate here.

[Joseph Pecoraro]

Comment on attachment 375403 [details]
[Patch] WIP

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

> Source/JavaScriptCore/API/JSContext.mm:175
> +- (void)_setITMLDebuggableType

This shouldn't be in the same patch.

Lets make clean individual patches:

    1. Update WebKit InspectorBackendCommands generation
    2. Provide ITML Support (JSContext SPI, Remote Inspector Support, ITML BackendCommands Generation)
        - We should be able to avoid this SPI and assume if someone uses (JSGlobalObjectInspectorController::appendExtraAgent) then we can set this bit.
        - That will allow us to support legacy backends with an ITML JSON file and ultimately remove extra agents from the frontend.
    3. Modernize Alternative Dispatchers (INSPECTOR_ALTERNATE_DISPATCHERS)
        - This will let us remove all of the extra agents frontend code

I think those are each nearly standalone and make good progress.

> Source/JavaScriptCore/CMakeLists.txt:1113
> +    ${JAVASCRIPTCORE_DIR}/inspector/protocol/ITML.json

ITML is going to want to provide their own Combined JSON for a set of domains, because they do a subset of DOM/CSS/DOMStorage etc.

This is another reason why I think we should tackle that in another patch. It'll be hard enough to review the _javascript _web _service-worker and make sure we don't break remote inspector support in other platforms these additional changes on top of it.

>> Source/JavaScriptCore/inspector/protocol/DOM.json:-71
>> -        {
> 
> Why is this removed?

Ultimately these are likely to either:

    (1) Move to an ITMLKit specific DOM domain
    (2) Move to an ITMLKit specific ITML domain

But they are not needed in WebKit's DOM domain. We added it there temporarily (WebKit never does anything with these).

> Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:-56
> -    enum class Type { JavaScript, ServiceWorker, Web, Automation };

I still like the idea of giving a SubType so that ITML can be a sub-type of JavaScript. So everything that works today with the JSContext works, but the remote inspector can use the subtype to provide the better backend commands file.

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:387
>          [listing setObject:WIRTypeJavaScript forKey:WIRTypeKey];

And here is where we would send a subtype.

> Source/WebInspectorUI/Scripts/update-LegacyInspectorBackendCommands.rb:60
> +      if match[1] == "tvOS"
> +        tasks << Task.new("itml", version_path, output_path)

We should do this when we add a tvOS. It should be "ITML" and we should wait to do this until we add such a versioned protocol.

[Joseph Pecoraro]

(In reply to Brian Burg from comment #8)
> Comment on attachment 375403 [details]
> [Patch] WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375403&action=review
> 
> As far as I can tell, debuggableType == targetType for all domains. What is
> the motivation for making them separate?

This is a bit of a clarification on things we had discussed for PSON / PPO in the past. I'll make my case for these.

Descriptions:

    * Target Type
      - An individual WI.Target that can be connected to
      - Used in the protocol file to note which domains should be available for a target of this type.
      - In the frontend this is what specifies whether `target.FooAgent` exists.

    * Debuggable Type
      - An individual Remote Inspector Debuggable Target (RemoteControllableTarget). This is the type broadcast over the remote inspection protocol
      - Used in the protocol file to note which domains could be available when connected to a debug target of this type
      - In the frontend this is what specifies whether `InspectorBackend.domains.Foo` exists.
      - In practice this only affects a multi-target world like a "web" debuggable which would likely ultimately be "page", "frame", and "service-worker" sub-targets

I imagine a PPO future with:

    Target Types:
    - javascript (JSC::JSGlobalObject)
    - service-worker (WebCore::ServiceWorkerThreadProxy)
    - worker (WebCore::WorkerGlobalScope)
    - page (WebKit::WebPageProxy)
    - frame (WebCore::Frame)
    
    Debuggable Types:
    - "javascript"     - Direct connection to a "javascript" target
    - "service-worker" - Direct connection to a "service-worker" target
    - "web"            - Multi-target connection to a "page" target, multiple "frame" targets, any possibly associated "service-worker" / "worker" targets

Even though technically, we could support connecting directly to an individual Worker, or an individual Frame, I don't think we should care about or work towards such a goal.

[Blaze Burg]

(In reply to Joseph Pecoraro from comment #10)
> (In reply to Brian Burg from comment #8)
> > Comment on attachment 375403 [details]
> > [Patch] WIP
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=375403&action=review
> > 
> > As far as I can tell, debuggableType == targetType for all domains. What is
> > the motivation for making them separate?
> 
> This is a bit of a clarification on things we had discussed for PSON / PPO
> in the past. I'll make my case for these.
> 
> Descriptions:
> 
>     * Target Type
>       - An individual WI.Target that can be connected to
>       - Used in the protocol file to note which domains should be available
> for a target of this type.
>       - In the frontend this is what specifies whether `target.FooAgent`
> exists.
> 
>     * Debuggable Type
>       - An individual Remote Inspector Debuggable Target
> (RemoteControllableTarget). This is the type broadcast over the remote
> inspection protocol
>       - Used in the protocol file to note which domains could be available
> when connected to a debug target of this type
>       - In the frontend this is what specifies whether
> `InspectorBackend.domains.Foo` exists.
>       - In practice this only affects a multi-target world like a "web"
> debuggable which would likely ultimately be "page", "frame", and
> "service-worker" sub-targets
> 
> I imagine a PPO future with:
> 
>     Target Types:
>     - javascript (JSC::JSGlobalObject)
>     - service-worker (WebCore::ServiceWorkerThreadProxy)
>     - worker (WebCore::WorkerGlobalScope)
>     - page (WebKit::WebPageProxy)
>     - frame (WebCore::Frame)
>     
>     Debuggable Types:
>     - "javascript"     - Direct connection to a "javascript" target
>     - "service-worker" - Direct connection to a "service-worker" target
>     - "web"            - Multi-target connection to a "page" target,
> multiple "frame" targets, any possibly associated "service-worker" /
> "worker" targets
> 
> Even though technically, we could support connecting directly to an
> individual Worker, or an individual Frame, I don't think we should care
> about or work towards such a goal.

Ah, this makes sense. I wish we had some place to document architecture-level details like this.

[Devin Rousso]

Created attachment 377268 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS Watchlist]

Attachment 377268 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:511:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:518:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:550:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:557:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:563:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 6 in 208 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Comment on attachment 377268 [details]
Patch

I also need to update the legacy files, as well as rebase the generator tests.

[Devin Rousso]

Created attachment 377295 [details]
Patch

Fix typo

[EWS Watchlist]

Attachment 377295 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:511:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:518:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:550:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:557:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:563:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 6 in 208 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 377309 [details]
Patch

[EWS Watchlist]

Attachment 377309 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:511:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:518:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:550:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:557:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:563:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 6 in 208 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 377405 [details]
Patch

Allow for multiple commands/events with the same name in the frontend.  Also rebase/add inspector protocol tests.

[Devin Rousso]

Created attachment 377417 [details]
Patch

Propagate the "parent" target when receiving worker/target messages so the right connection is used for dispatching them.

[Devin Rousso]

Created attachment 377450 [details]
Patch

Remove legacy `Target.exists` since we can do proper feature checking via `InspectorBackend.domains.Target`.

[Devin Rousso]

Created attachment 377553 [details]
Patch

Split the "web" debuggable target into "page" (WK1) and "web-page" (WK2) so that we can properly feature check for PSON multi-target support.

[Greg Marriott]

perhaps legacy-page (WK1) and page (WK2)

[Yury Semikhatsky]

(In reply to Joseph Pecoraro from comment #10)
> I imagine a PPO future with:
> 
>     Target Types:
>     - javascript (JSC::JSGlobalObject)
>     - service-worker (WebCore::ServiceWorkerThreadProxy)
>     - worker (WebCore::WorkerGlobalScope)
>     - page (WebKit::WebPageProxy)
>     - frame (WebCore::Frame)
>     
>     Debuggable Types:
>     - "javascript"     - Direct connection to a "javascript" target
>     - "service-worker" - Direct connection to a "service-worker" target
>     - "web"            - Multi-target connection to a "page" target,
> multiple "frame" targets, any possibly associated "service-worker" /
> "worker" targets
> 
> Even though technically, we could support connecting directly to an
> individual Worker, or an individual Frame, I don't think we should care
> about or work towards such a goal.

What advantages do you see in separating Target and Debuggable Types?

To me it looks simpler from the client stand point if the client can connect directly
to individual targets be it page, service-worker, dedicated worker or what not.
In that case target type defines all possible capabilities of the target and
the client just needs to support flat list of connections to the target rather
than routing messages say from worker differently than from a page. If target
agent in the UIProcess is aware of all the targets it should be able to route
messages directly to them. In case of remote debugger it's even easier (to the
extend I understand current implementation).

IIUC, chief advantage of debuggable types is that one can discover page first
and then nested targets underneath, i.e. they will give information about nesting
structure of the targets. If that's the goal, would it make sense to extend TargetInfo
with a notion of 'owner' (perhaps not the best term given the shared ownership model
of servive-workers)?

Another way to learn about targets' nesting structure is to use something like
Target.setAutoAttach[1] in Chrome. On the page target after inspector connected to it,
it would be notified about all 'nested' targets and have a chance to connect to them.
I don't quite like that approach because it essentially introduces nested level of target discovery into otherwise flat list of targets in the protocol.


[1] https://vanilla.aslushnikov.com/?Target.setAutoAttach

[Yury Semikhatsky]

Comment on attachment 377553 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:54
> +        (-[JSContext _setITMLDebuggableType]): Added.

What does ITML stand for, can you provide some pointers?

In any case it sounds like a new feature which has little to do with the refactoring and could be factored out into a separate change.

> Source/JavaScriptCore/inspector/protocol/Target.json:12
> +                { "name": "type", "type": "string", "enum": ["page", "service-worker", "worker"] }

If current fronte

Do I understand correctly that the protocol

> Source/WebCore/inspector/InspectorFrontendClient.h:63
> +    virtual String debuggableType() const = 0;

Should it return an enum value so that it's clear what are the all supported debuggable types?

> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:32
> +        switch (InspectorFrontendHost.debuggableType()) {

Why does debuggable type have to come from InspectorFrontendHost? Can't it be derived from the inspected target type?

> Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json:3267
> +    "debuggableTypes": ["page", "web-page"],

Looking at this code it's quite hard to guess how web-page is different from page. Maybe legacy-page like Greg suggests or wk1-page/wk2-page ?

> LayoutTests/inspector/dom/inspect.html:35
> +    InspectorProtocol.sendCommand("DOM.getDocument", {}, (response) => {

You can write the test in a more concise and linear way using await, something like:

const documentResponse = await InspectorProtocol.awaitCommand("DOM.getDocument", {});
assertResponse(documentResponse, "Got document.");

const nodeResponse = await InspectorProtocol.awaitCommand("DOM.querySelector", {nodeId: response.result.root.nodeId, selector: "#target"});
assertResponse(nodeResponse, "Queryied for target node.");

const inspectedNodeResponse =  await InspectorProtocol.awaitCommand("DOM.setInspectedNode", {nodeId: nodeResponse.result.nodeId});
assertResponse(inspectedNodeResponse, "Set $0 to the target node.");

const $0Response =  await InspectorProtocol.awaitCommand("Runtime.evaluate", {"expression": "$0.id", "includeCommandLineAPI": true});
assertResponse($0Response, "Evaluate $0.");
ProtocolTest.expectEqual($0Response.result.result.value, "target", "Id of $0 is \"target\".");
ProtocolTest.completeTest();

[Yury Semikhatsky]

Comment on attachment 377553 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:19
> +        depending on the debuggable type. Furthermore, each target underneath that debuggable needs

Looks like each target will always have more accurate information about supported domains/commands. Is it possible to get rid of the global InspectorBackend.domains completely and instead always consult with corresponding target and have some meaningful defaults when there is no target yet?

[Devin Rousso]

Comment on attachment 377553 [details]
Patch

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

(In reply to Yury Semikhatsky from comment #25)
> What advantages do you see in separating Target and Debuggable Types?
> 
> To me it looks simpler from the client stand point if the client can connect directly to individual targets be it page, service-worker, dedicated worker or what not.
> In that case target type defines all possible capabilities of the target and the client just needs to support flat list of connections to the target rather than routing messages say from worker differently than from a page. If target agent in the UIProcess is aware of all the targets it should be able to route messages directly to them. In case of remote debugger it's even easier (to the extend I understand current implementation).
> 
> IIUC, chief advantage of debuggable types is that one can discover page first and then nested targets underneath, i.e. they will give information about nesting structure of the targets. If that's the goal, would it make sense to extend TargetInfo with a notion of 'owner' (perhaps not the best term given the shared ownership model of servive-workers)?
The main issue here is that there is only one UI that's shared for ALL targets, so we need a way at the very beginning of determining "is it ever possible to connect to a sub-target that would support this?" so we can decide what UI to create.  This is the idea behind `InspectorBackend.domains.*`, which is sort of a "description" of "here's everything that _could_ be supported for this main target (`debuggableType`) by some type of sub-target (`targetType`)".

Creating a flat map of all targets only tells you the capabilities of everything you _are_ connected to, not what you _could_ connect to.  As an example (somewhat contrived), how do you know whether or not a `WebCore::Page` ("page") has the capaibility of spawning and connecting to a related `WebCore::Worker ("worker") _before_ the `WebCore::Worker` is created?

The idea of the `debuggableType` is to definitively list the full capabilities of anything that _could_ appear as a target, and allow Web Inspector to present a UI as such, regardless of whether any targets are actually connected that have those supported capabilities.

>> Source/JavaScriptCore/ChangeLog:54
>> +        (-[JSContext _setITMLDebuggableType]): Added.
> 
> What does ITML stand for, can you provide some pointers?
> 
> In any case it sounds like a new feature which has little to do with the refactoring and could be factored out into a separate change.

This is basically TVML [1], but also helps some other internal clients.

For a while now (not sure exactly how long), you can use Web Inspector to inspect TVML apps.  This has been done via the "extra domains" concept (follow from `JSGlobalObjectInspectorController::appendExtraAgent` and the generated `RWIProtocolConfiguration.mm`).  They're basically a JSContext with extra capabilities (DOM, Network, Page, CSS, DOMStorage).  We'd like to be able to treat them as a separate type, not an augmented JSContext.  This is primarily beneficial for the reasons described above.

[1]: https://developer.apple.com/documentation/tvml?language=objc

>> Source/WebInspectorUI/ChangeLog:19
>> +        depending on the debuggable type. Furthermore, each target underneath that debuggable needs
> 
> Looks like each target will always have more accurate information about supported domains/commands. Is it possible to get rid of the global InspectorBackend.domains completely and instead always consult with corresponding target and have some meaningful defaults when there is no target yet?

The target will know what it supports, but we can be simultaneously connected to multiple targets.  See above comment.

>> Source/JavaScriptCore/inspector/protocol/Target.json:12
>> +                { "name": "type", "type": "string", "enum": ["page", "service-worker", "worker"] }
> 
> If current fronte
> 
> Do I understand correctly that the protocol

?

>> Source/WebCore/inspector/InspectorFrontendClient.h:63
>> +    virtual String debuggableType() const = 0;
> 
> Should it return an enum value so that it's clear what are the all supported debuggable types?

Ideally, yes, but then it would complicate the logic of `RemoteWebInspectorProxy` and `InspectorFrontendHost.debuggableType()`.  We "fall back" to being a JavaScript debuggable if an unknown/invalid type is supplied, which is OK since all other types support JavaScript at the very least.  It would also make it ever-so-slightly more difficult for forks of WebKit (or other clients) to add support for Web Inspector debugging as they'd have to add an enum value.

>> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:32
>> +        switch (InspectorFrontendHost.debuggableType()) {
> 
> Why does debuggable type have to come from InspectorFrontendHost? Can't it be derived from the inspected target type?

At this point, we haven't connected to any targets yet, so we don't know anything about it.  Theoretically, we can determine the `debuggableType` information from the first target we connect to, but we need to know about the `debuggableType` _before_ that happens too (see above for more in-depth explanation).

>> Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json:3267
>> +    "debuggableTypes": ["page", "web-page"],
> 
> Looking at this code it's quite hard to guess how web-page is different from page. Maybe legacy-page like Greg suggests or wk1-page/wk2-page ?

It's not WK1-page vs WK2-page, because a sub-target of "web-page" is "page" (so my bad for suggesting that).  It really just mirrors the type of object that we're directly connecting to (`WebCore::Page` vs `WebKit::WebPage`).  The same is true of the other types too (e.g. `JSC::JSContext`).

>> LayoutTests/inspector/dom/inspect.html:35
>> +    InspectorProtocol.sendCommand("DOM.getDocument", {}, (response) => {
> 
> You can write the test in a more concise and linear way using await, something like:
> 
> const documentResponse = await InspectorProtocol.awaitCommand("DOM.getDocument", {});
> assertResponse(documentResponse, "Got document.");
> 
> const nodeResponse = await InspectorProtocol.awaitCommand("DOM.querySelector", {nodeId: response.result.root.nodeId, selector: "#target"});
> assertResponse(nodeResponse, "Queryied for target node.");
> 
> const inspectedNodeResponse =  await InspectorProtocol.awaitCommand("DOM.setInspectedNode", {nodeId: nodeResponse.result.nodeId});
> assertResponse(inspectedNodeResponse, "Set $0 to the target node.");
> 
> const $0Response =  await InspectorProtocol.awaitCommand("Runtime.evaluate", {"expression": "$0.id", "includeCommandLineAPI": true});
> assertResponse($0Response, "Evaluate $0.");
> ProtocolTest.expectEqual($0Response.result.result.value, "target", "Id of $0 is \"target\".");
> ProtocolTest.completeTest();

I could, yes, but I don't think we have any precedent with having `test` be `async`, so I'd rather have that discussion first.  I'd also like to keep this as similar to what we had as possible, so the diff is more grok-able.

[Devin Rousso]

Comment on attachment 377553 [details]
Patch

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

>>> Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json:3267
>>> +    "debuggableTypes": ["page", "web-page"],
>> 
>> Looking at this code it's quite hard to guess how web-page is different from page. Maybe legacy-page like Greg suggests or wk1-page/wk2-page ?
> 
> It's not WK1-page vs WK2-page, because a sub-target of "web-page" is "page" (so my bad for suggesting that).  It really just mirrors the type of object that we're directly connecting to (`WebCore::Page` vs `WebKit::WebPage`).  The same is true of the other types too (e.g. `JSC::JSContext`).

The reason I referred to them as WK1 and WK2 is simply because that's where they're used (e.g. you won't see a "web-page" when trying to inspect something using WK1).

[Yury Semikhatsky]

Comment on attachment 377553 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:54
>>> +        (-[JSContext _setITMLDebuggableType]): Added.
>> 
>> What does ITML stand for, can you provide some pointers?
>> 
>> In any case it sounds like a new feature which has little to do with the refactoring and could be factored out into a separate change.
> 
> This is basically TVML [1], but also helps some other internal clients.
> 
> For a while now (not sure exactly how long), you can use Web Inspector to inspect TVML apps.  This has been done via the "extra domains" concept (follow from `JSGlobalObjectInspectorController::appendExtraAgent` and the generated `RWIProtocolConfiguration.mm`).  They're basically a JSContext with extra capabilities (DOM, Network, Page, CSS, DOMStorage).  We'd like to be able to treat them as a separate type, not an augmented JSContext.  This is primarily beneficial for the reasons described above.
> 
> [1]: https://developer.apple.com/documentation/tvml?language=objc

Thanks for the explanation. Would it make sense to call corresponding target type TVML? I may
also be worth adding a comment in the code explaining what this type is for.

>>> Source/JavaScriptCore/inspector/protocol/Target.json:12
>>> +                { "name": "type", "type": "string", "enum": ["page", "service-worker", "worker"] }
>> 
>> If current fronte
>> 
>> Do I understand correctly that the protocol
> 
> ?

Nevermind, I was going to ask about backward/forward compatibility requirements for the protocol, but then I noticed that the front-end code now handles both constants and stays compatible with older backends.

>>> Source/WebCore/inspector/InspectorFrontendClient.h:63
>>> +    virtual String debuggableType() const = 0;
>> 
>> Should it return an enum value so that it's clear what are the all supported debuggable types?
> 
> Ideally, yes, but then it would complicate the logic of `RemoteWebInspectorProxy` and `InspectorFrontendHost.debuggableType()`.  We "fall back" to being a JavaScript debuggable if an unknown/invalid type is supplied, which is OK since all other types support JavaScript at the very least.  It would also make it ever-so-slightly more difficult for forks of WebKit (or other clients) to add support for Web Inspector debugging as they'd have to add an enum value.

Can you elaborate on how it'd complicate it? There is already debuggableTypeFromHost
which translates from the constants used in c++ code to the ones used in the frontend.

If the forks cannot add support for their own type to the front-end they
will face the same problem. So it seems that they'd have to either fall back
to basic JavaScript inspector, pick from the existing types or modify WebKit to
add new type anyway.

>>> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:32
>>> +        switch (InspectorFrontendHost.debuggableType()) {
>> 
>> Why does debuggable type have to come from InspectorFrontendHost? Can't it be derived from the inspected target type?
> 
> At this point, we haven't connected to any targets yet, so we don't know anything about it.  Theoretically, we can determine the `debuggableType` information from the first target we connect to, but we need to know about the `debuggableType` _before_ that happens too (see above for more in-depth explanation).

The UI could start assuming minimal feature set (i.e. JSContext) and extend it later
depending on the main target type. But as you mentioned it may lead to undesirable
UI transformations. To avoid that you want to learn event before the connection is
established what kind of target you are going to inspect. Using a native binding helps
achieve this but seems like a side channel. Imagine e.g. connection to the remote
target over a web socket from a frontend loaded as a regular page (no RemoteInspectorUI
bindings at all). Native InspectorFrontendHost would not be available but the same
functionality could be achieved via e.g. passing arguments to the front-end url.
Anyway, it's not directly related to this change.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:295
> +        console.assert(this._commandParameters.has(commandName) === this._commandReturns.has(commandName));

It was impossible to have such problem in the previous code where each command was represented by an object instead of several multimaps that have to be kept in sync. Also
  if (InspectorBackend.domains.Timeline.setAutoCaptureEnabled)
is a shorter than
  if (InspectorBackend.domains.Timeline.hasCommand("setAutoCaptureEnabled"))
and it would be possible to validate former with a js compiler should you decide to use one. 
I wonder what is the motivation for changing this to string keyed multimaps?

>>>> Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json:3267
>>>> +    "debuggableTypes": ["page", "web-page"],
>>> 
>>> Looking at this code it's quite hard to guess how web-page is different from page. Maybe legacy-page like Greg suggests or wk1-page/wk2-page ?
>> 
>> It's not WK1-page vs WK2-page, because a sub-target of "web-page" is "page" (so my bad for suggesting that).  It really just mirrors the type of object that we're directly connecting to (`WebCore::Page` vs `WebKit::WebPage`).  The same is true of the other types too (e.g. `JSC::JSContext`).
> 
> The reason I referred to them as WK1 and WK2 is simply because that's where they're used (e.g. you won't see a "web-page" when trying to inspect something using WK1).

I had an impression that WebPage is WK2 concept used to communicate with its proxy in UIProcess and it doesn't exist in WK1.

Is there any significant difference between page and web-page except for the Target domain?

>>> LayoutTests/inspector/dom/inspect.html:35
>>> +    InspectorProtocol.sendCommand("DOM.getDocument", {}, (response) => {
>> 
>> You can write the test in a more concise and linear way using await, something like:
>> 
>> const documentResponse = await InspectorProtocol.awaitCommand("DOM.getDocument", {});
>> assertResponse(documentResponse, "Got document.");
>> 
>> const nodeResponse = await InspectorProtocol.awaitCommand("DOM.querySelector", {nodeId: response.result.root.nodeId, selector: "#target"});
>> assertResponse(nodeResponse, "Queryied for target node.");
>> 
>> const inspectedNodeResponse =  await InspectorProtocol.awaitCommand("DOM.setInspectedNode", {nodeId: nodeResponse.result.nodeId});
>> assertResponse(inspectedNodeResponse, "Set $0 to the target node.");
>> 
>> const $0Response =  await InspectorProtocol.awaitCommand("Runtime.evaluate", {"expression": "$0.id", "includeCommandLineAPI": true});
>> assertResponse($0Response, "Evaluate $0.");
>> ProtocolTest.expectEqual($0Response.result.result.value, "target", "Id of $0 is \"target\".");
>> ProtocolTest.completeTest();
> 
> I could, yes, but I don't think we have any precedent with having `test` be `async`, so I'd rather have that discussion first.  I'd also like to keep this as similar to what we had as possible, so the diff is more grok-able.

If you don't want to mark test() method async you could wrap the code in something
like this: 
  (async () => {<test code>})();
You are switching the test from inspector-test.js to protocol-test.js leaving
no unchanged lines in the diff anyway. That is why I'm suggesting to also make
it more readable. But if you feel like it may cause some issues I'm fine with
writing it the old way.

[Devin Rousso]

Comment on attachment 377553 [details]
Patch

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

>>>> Source/WebCore/inspector/InspectorFrontendClient.h:63
>>>> +    virtual String debuggableType() const = 0;
>>> 
>>> Should it return an enum value so that it's clear what are the all supported debuggable types?
>> 
>> Ideally, yes, but then it would complicate the logic of `RemoteWebInspectorProxy` and `InspectorFrontendHost.debuggableType()`.  We "fall back" to being a JavaScript debuggable if an unknown/invalid type is supplied, which is OK since all other types support JavaScript at the very least.  It would also make it ever-so-slightly more difficult for forks of WebKit (or other clients) to add support for Web Inspector debugging as they'd have to add an enum value.
> 
> Can you elaborate on how it'd complicate it? There is already debuggableTypeFromHost
> which translates from the constants used in c++ code to the ones used in the frontend.
> 
> If the forks cannot add support for their own type to the front-end they
> will face the same problem. So it seems that they'd have to either fall back
> to basic JavaScript inspector, pick from the existing types or modify WebKit to
> add new type anyway.

Thinking about this again (it's been a while), I think the only real "issue" with using an enum is handling the case where there is no value set (see `InspectorFrontendHost::debuggableType` for an example of how this may happen).  I think we can work around this though, so we probably should make this into an enum.

> Source/WebInspectorUI/UserInterface/Base/Multimap.js:68
> +    addIterable(key, iterable)

I should rename this to `addAll` to be consistent with <https://webkit.org/b/201082>.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:297
> +        if (this._target.CanvasAgent.startRecording.hasParameter("singleFrame")) {

I should probably also add `hasCommand`/`hasCommandParameter`/`hasCommandReturn` to `InspectorBackend.Agent` in the case that `this._target.CanvasAgent` doesn't have a `startRecording` for some reason.  It is slightly more complex for `InspectorBackend.Agent`, since the commands are stored directly on the object instead of in some sort of map.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:295
>> +        console.assert(this._commandParameters.has(commandName) === this._commandReturns.has(commandName));
> 
> It was impossible to have such problem in the previous code where each command was represented by an object instead of several multimaps that have to be kept in sync. Also
>   if (InspectorBackend.domains.Timeline.setAutoCaptureEnabled)
> is a shorter than
>   if (InspectorBackend.domains.Timeline.hasCommand("setAutoCaptureEnabled"))
> and it would be possible to validate former with a js compiler should you decide to use one. 
> I wonder what is the motivation for changing this to string keyed multimaps?

I can't remember all the reasons why I decided to take this approach, but yes it does make global feature checking slightly more verbose.

I personally prefer a more verbose/explicit approach, as it more clearly distinguishes callsites from feature-checks, and there is some precedent with `hasEvent`.  I would rather us have a very clear distinction between those two use cases.

FWIW, `hasCommandReturn` also isn't used right now, so we could remove the code related to that in this patch, but it does make a more complete API surface.

>>>>> Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json:3267
>>>>> +    "debuggableTypes": ["page", "web-page"],
>>>> 
>>>> Looking at this code it's quite hard to guess how web-page is different from page. Maybe legacy-page like Greg suggests or wk1-page/wk2-page ?
>>> 
>>> It's not WK1-page vs WK2-page, because a sub-target of "web-page" is "page" (so my bad for suggesting that).  It really just mirrors the type of object that we're directly connecting to (`WebCore::Page` vs `WebKit::WebPage`).  The same is true of the other types too (e.g. `JSC::JSContext`).
>> 
>> The reason I referred to them as WK1 and WK2 is simply because that's where they're used (e.g. you won't see a "web-page" when trying to inspect something using WK1).
> 
> I had an impression that WebPage is WK2 concept used to communicate with its proxy in UIProcess and it doesn't exist in WK1.
> 
> Is there any significant difference between page and web-page except for the Target domain?

As far as the protocol, I don't think there are any other differences, but there could be, especially since we support a "featureGuard" concept.

There can be implicit differences, however, like WK2 having the concept of user interaction (different from user gesture) <https://webkit.org/b/197269>.

[Yury Semikhatsky]

Comment on attachment 377553 [details]
Patch

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

Overall the approach looks good to me. At the same time there are too many things being changed in the same CL which could be done in separate smaller patches with clear scope (this has already been suggested above I believe), e.g. changing how the code checks if given command/event/parameter is present on a domain could be addressed separately. Splitting the patch could help expedite review process in general as there would be less friction on each individual patch and would be easier to comprehend for reviewers. I also noticed that there is a fair amount of feedback from reviewers and myself on the parts that are not directly related to the main goal of the CL which could be avoided by moving iteratively.

>>>>> Source/WebCore/inspector/InspectorFrontendClient.h:63
>>>>> +    virtual String debuggableType() const = 0;
>>>> 
>>>> Should it return an enum value so that it's clear what are the all supported debuggable types?
>>> 
>>> Ideally, yes, but then it would complicate the logic of `RemoteWebInspectorProxy` and `InspectorFrontendHost.debuggableType()`.  We "fall back" to being a JavaScript debuggable if an unknown/invalid type is supplied, which is OK since all other types support JavaScript at the very least.  It would also make it ever-so-slightly more difficult for forks of WebKit (or other clients) to add support for Web Inspector debugging as they'd have to add an enum value.
>> 
>> Can you elaborate on how it'd complicate it? There is already debuggableTypeFromHost
>> which translates from the constants used in c++ code to the ones used in the frontend.
>> 
>> If the forks cannot add support for their own type to the front-end they
>> will face the same problem. So it seems that they'd have to either fall back
>> to basic JavaScript inspector, pick from the existing types or modify WebKit to
>> add new type anyway.
> 
> Thinking about this again (it's been a while), I think the only real "issue" with using an enum is handling the case where there is no value set (see `InspectorFrontendHost::debuggableType` for an example of how this may happen).  I think we can work around this though, so we probably should make this into an enum.

Yeah, when there is no frontend client debuggableType should have some meaningful fall back anyway, current return String() just defers that decision.

>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:295
>>> +        console.assert(this._commandParameters.has(commandName) === this._commandReturns.has(commandName));
>> 
>> It was impossible to have such problem in the previous code where each command was represented by an object instead of several multimaps that have to be kept in sync. Also
>>   if (InspectorBackend.domains.Timeline.setAutoCaptureEnabled)
>> is a shorter than
>>   if (InspectorBackend.domains.Timeline.hasCommand("setAutoCaptureEnabled"))
>> and it would be possible to validate former with a js compiler should you decide to use one. 
>> I wonder what is the motivation for changing this to string keyed multimaps?
> 
> I can't remember all the reasons why I decided to take this approach, but yes it does make global feature checking slightly more verbose.
> 
> I personally prefer a more verbose/explicit approach, as it more clearly distinguishes callsites from feature-checks, and there is some precedent with `hasEvent`.  I would rather us have a very clear distinction between those two use cases.
> 
> FWIW, `hasCommandReturn` also isn't used right now, so we could remove the code related to that in this patch, but it does make a more complete API surface.

I agree that hasCommand/hasParameter etc better reflect the intent of the call, I'm just not a fan of using raw constants especially when we can generate them and utilize some automatic checks, otherwise the code relies solely on the tests and reviewers. If you are changing the signatures of feature checkers anyway, would it make sense to make the calls less verbose by supporting something like this:

Backend.hasCommand("Target.exists");

instead of 

InspectorBackend.domains.Target && InspectorBackend.domains.Target.hasCommand("exists")

and similar methods for checking parameters and return values?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:301
> +        return this._commandParameters.has(commandName, commandParameterName);

What is the value of creating _commandParameters and _commandReturns multimaps over storing a map name->InspectorBackend.Command and then
writing something like

const command = this._command[commandName];
return command && command.hasParameter(commandParameterName);

This is way you'd have only one copy of the command metadata and would use that for both feature detection and agent construction.

> Source/WebInspectorUI/UserInterface/Protocol/Legacy/10.3/InspectorBackendCommands.js:32
> +InspectorBackend.registerDomain("ApplicationCache", ["page"]);

Would be nice to preserve order of the calls in this file to make comparison with the previous version easier. Otherwise it is quite hard to see if there are any inadvertent changes.

[Devin Rousso]

Comment on attachment 377553 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:295
>>>> +        console.assert(this._commandParameters.has(commandName) === this._commandReturns.has(commandName));
>>> 
>>> It was impossible to have such problem in the previous code where each command was represented by an object instead of several multimaps that have to be kept in sync. Also
>>>   if (InspectorBackend.domains.Timeline.setAutoCaptureEnabled)
>>> is a shorter than
>>>   if (InspectorBackend.domains.Timeline.hasCommand("setAutoCaptureEnabled"))
>>> and it would be possible to validate former with a js compiler should you decide to use one. 
>>> I wonder what is the motivation for changing this to string keyed multimaps?
>> 
>> I can't remember all the reasons why I decided to take this approach, but yes it does make global feature checking slightly more verbose.
>> 
>> I personally prefer a more verbose/explicit approach, as it more clearly distinguishes callsites from feature-checks, and there is some precedent with `hasEvent`.  I would rather us have a very clear distinction between those two use cases.
>> 
>> FWIW, `hasCommandReturn` also isn't used right now, so we could remove the code related to that in this patch, but it does make a more complete API surface.
> 
> I agree that hasCommand/hasParameter etc better reflect the intent of the call, I'm just not a fan of using raw constants especially when we can generate them and utilize some automatic checks, otherwise the code relies solely on the tests and reviewers. If you are changing the signatures of feature checkers anyway, would it make sense to make the calls less verbose by supporting something like this:
> 
> Backend.hasCommand("Target.exists");
> 
> instead of 
> 
> InspectorBackend.domains.Target && InspectorBackend.domains.Target.hasCommand("exists")
> 
> and similar methods for checking parameters and return values?

My only hesitance to your suggestion is that it would then likely involve a `String.prototype.split`, which is more work than just a Map/Object lookup.  I'd prefer to avoid doing work where we can, but other than that, I'm fine with it.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:301
>> +        return this._commandParameters.has(commandName, commandParameterName);
> 
> What is the value of creating _commandParameters and _commandReturns multimaps over storing a map name->InspectorBackend.Command and then
> writing something like
> 
> const command = this._command[commandName];
> return command && command.hasParameter(commandParameterName);
> 
> This is way you'd have only one copy of the command metadata and would use that for both feature detection and agent construction.

Ah I remember why I did this!  So one reason for this is that we could theoretically have multiple versions of the same command/event so long as there's no overlap in the `targetTypes`, meaning that the same command/event name could map to multiple `InspectorBackend.Command`/`InspectorBackend.Event`.  This would be a very useful thing to have for compatibility reasons for ITML (WebKit can change without breaking ITML).

>> Source/WebInspectorUI/UserInterface/Protocol/Legacy/10.3/InspectorBackendCommands.js:32
>> +InspectorBackend.registerDomain("ApplicationCache", ["page"]);
> 
> Would be nice to preserve order of the calls in this file to make comparison with the previous version easier. Otherwise it is quite hard to see if there are any inadvertent changes.

FWIW, all of these files are autogenerated (or more accurately copies of autogenerated files).  There are some things that would need to change (`InspectorBackend.registerDomain` needs to be first), and I do think this order is better, but if I could split that into another patch if people prefer the current ordering.

[Yury Semikhatsky]

Comment on attachment 377553 [details]
Patch

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

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

nit: here and in other places use const instead of let when possible?

>>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:295
>>>>> +        console.assert(this._commandParameters.has(commandName) === this._commandReturns.has(commandName));
>>>> 
>>>> It was impossible to have such problem in the previous code where each command was represented by an object instead of several multimaps that have to be kept in sync. Also
>>>>   if (InspectorBackend.domains.Timeline.setAutoCaptureEnabled)
>>>> is a shorter than
>>>>   if (InspectorBackend.domains.Timeline.hasCommand("setAutoCaptureEnabled"))
>>>> and it would be possible to validate former with a js compiler should you decide to use one. 
>>>> I wonder what is the motivation for changing this to string keyed multimaps?
>>> 
>>> I can't remember all the reasons why I decided to take this approach, but yes it does make global feature checking slightly more verbose.
>>> 
>>> I personally prefer a more verbose/explicit approach, as it more clearly distinguishes callsites from feature-checks, and there is some precedent with `hasEvent`.  I would rather us have a very clear distinction between those two use cases.
>>> 
>>> FWIW, `hasCommandReturn` also isn't used right now, so we could remove the code related to that in this patch, but it does make a more complete API surface.
>> 
>> I agree that hasCommand/hasParameter etc better reflect the intent of the call, I'm just not a fan of using raw constants especially when we can generate them and utilize some automatic checks, otherwise the code relies solely on the tests and reviewers. If you are changing the signatures of feature checkers anyway, would it make sense to make the calls less verbose by supporting something like this:
>> 
>> Backend.hasCommand("Target.exists");
>> 
>> instead of 
>> 
>> InspectorBackend.domains.Target && InspectorBackend.domains.Target.hasCommand("exists")
>> 
>> and similar methods for checking parameters and return values?
> 
> My only hesitance to your suggestion is that it would then likely involve a `String.prototype.split`, which is more work than just a Map/Object lookup.  I'd prefer to avoid doing work where we can, but other than that, I'm fine with it.

I wouldn't worry about performance here, this code runs very rarely (mostly once for some small number of commands?) and 'split' call itself is very cheap. I'd be very surprised if any of these methods appeared in a CPU profile. Unless there is profile data indicating performance bottleneck I personally would prefer more readable code to premature optimizations.

>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:301
>>> +        return this._commandParameters.has(commandName, commandParameterName);
>> 
>> What is the value of creating _commandParameters and _commandReturns multimaps over storing a map name->InspectorBackend.Command and then
>> writing something like
>> 
>> const command = this._command[commandName];
>> return command && command.hasParameter(commandParameterName);
>> 
>> This is way you'd have only one copy of the command metadata and would use that for both feature detection and agent construction.
> 
> Ah I remember why I did this!  So one reason for this is that we could theoretically have multiple versions of the same command/event so long as there's no overlap in the `targetTypes`, meaning that the same command/event name could map to multiple `InspectorBackend.Command`/`InspectorBackend.Event`.  This would be a very useful thing to have for compatibility reasons for ITML (WebKit can change without breaking ITML).

How is that possible? Can a backend talk several different versions of the protocol?

If it's possible to have different versions of the same domain on different target types should registerVersion take that into account and version individual commands / sets of commands?

[Devin Rousso]

Comment on attachment 377553 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:158
>> +        let [domainName, enumName] = qualifiedName.split(".");
> 
> nit: here and in other places use const instead of let when possible?

Our usual style is to only use `const` for values that don't ever change between invocations of the parent function or Web Inspector itself (e.g. actual constant values).

>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:301
>>>> +        return this._commandParameters.has(commandName, commandParameterName);
>>> 
>>> What is the value of creating _commandParameters and _commandReturns multimaps over storing a map name->InspectorBackend.Command and then
>>> writing something like
>>> 
>>> const command = this._command[commandName];
>>> return command && command.hasParameter(commandParameterName);
>>> 
>>> This is way you'd have only one copy of the command metadata and would use that for both feature detection and agent construction.
>> 
>> Ah I remember why I did this!  So one reason for this is that we could theoretically have multiple versions of the same command/event so long as there's no overlap in the `targetTypes`, meaning that the same command/event name could map to multiple `InspectorBackend.Command`/`InspectorBackend.Event`.  This would be a very useful thing to have for compatibility reasons for ITML (WebKit can change without breaking ITML).
> 
> How is that possible? Can a backend talk several different versions of the protocol?
> 
> If it's possible to have different versions of the same domain on different target types should registerVersion take that into account and version individual commands / sets of commands?

There's nothing preventing this from happening.  Fundamentally, the protocol is just an organizing "framework" around JSON-RPC-like payloads being sent over a connection.  Each backend would likely only actually have one "version" of each command/event, but the protocol JSON files are effectively a combination of the capabilities all of the various "platforms" that are inspectable.  As an example, `DOM.getDataBindingsForNode` and `DOM.getAssociatedDataForNode` are both ITML-specific features that we would likely not want to show for non-ITML targets, so we'd only want those to be created if `debuggableType === "ITML"`.  The primary reason we'd want to do this, however, is for situations where WebKit changes any of the parameters/returns for a command/event that ITML doesn't want, so we'd be able to copy the current command/event and change the `targetTypes` to just be "ITML", and then edit the original command/event to have `targetTypes` for everything else.

Theoretically, C++ supports function overloading, and the various `*BackendDispatcherHandler` objects are generated from the JSON files anyways, so as long as two commands with the same name don't have the same signature (e.g. the types of the parameters/returns are the same) we could support switching based on the types of the arguments.  The intention for now isn't to support this, as that needs other support in C++ to do that switching, but it is a possibility that we may want to have in the future.

[Joseph Pecoraro]

Comment on attachment 377553 [details]
Patch

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

Awesome!

I think this patch does a little too much at once (ITML, assumingMainTarget sometimes, node highlighter changes, node query changes, and the core protocol changes). But now that it has been reviewed I'm still fine with keeping most of these in one patch. However, please extract the ITMLKit specific changes (APIs), as I didn't review that in detail and it should really be separate.

I'm ready to r+, but I want to see an updated and rebaselined patch without the ITMLKit SPIs.

I'd probably also want to play with it just a bit to sanity check some things (like JSContext inspection and older iOS device inspection).

> Source/JavaScriptCore/ChangeLog:74
> +        * inspector/scripts/tests/*:
> +        Update test results, as well as added new tests for `debuggableTypes` and `targetTypes`.

You could enumerate the "Added" tests and * the rest.

> Source/WebInspectorUI/ChangeLog:9
> +        `InspectorBackend.domains.${domain}` isn't a truly valid way to feature check, as it

This ChangeLog is great!

There should also be a section listing out the debuggable types and target types akin to:
https://bugs.webkit.org/show_bug.cgi?id=200384#c10

That way when this lands it will forever document the state of the world at this point.

>>> Source/WebInspectorUI/ChangeLog:19
>>> +        depending on the debuggable type. Furthermore, each target underneath that debuggable needs
>> 
>> Looks like each target will always have more accurate information about supported domains/commands. Is it possible to get rid of the global InspectorBackend.domains completely and instead always consult with corresponding target and have some meaningful defaults when there is no target yet?
> 
> The target will know what it supports, but we can be simultaneously connected to multiple targets.  See above comment.

Yeah, I think this was discussed offline but our goal is to move to `target.FooAgent.command` feature checks as much as possible, as that will be the most accurate for that target. However there are still cases where we want `InspectorBackend.domains.FooAgent.command` checks to know if some UI should even be shown given what we are connected to, as that will indicate whether some UI feature is even relevant or not.

> Source/JavaScriptCore/API/JSContextPrivate.h:114
> +- (void)_setITMLDebuggableType JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));

Have we tried ITML with this? Can we keep the ITMLKit parts of the patch separate to reduce the large scope of this?

NOTE: I wrote some of these comments a long time ago but forgot to submit the early review. I agree with Yury that smaller aspects, like this, can be extracted out of this patch and done separately.

> Source/JavaScriptCore/inspector/protocol/Audit.json:3
>      "description": "",

We should have given this a description back in the day!

> Source/JavaScriptCore/inspector/protocol/Audit.json:4
> +    "targetTypes": ["itml", "jscontext", "page", "service-worker", "worker"],

Do Audits work in a Worker today?

I know they work in a jscontext / service-worker though I still think that is probably non-ideal. Unless we can produce `unsupported` results based on the target.

> Source/JavaScriptCore/inspector/protocol/Security.json:5
> +    "targetTypes": ["itml", "service-worker", "page"],

Nit: You normally do these in alphabetical order. "page" < "service-worker".

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:381
> +    case RemoteInspectionTarget::Type::ITML:

More ITML code that could happen separately.

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:-144
> -    else if (target.type() == RemoteInspectionTarget::Type::ServiceWorker)
> -        targetListing->setString("type"_s, "service-worker"_s);

It looks like they previously supported service-worker, so we should probably keep that case.

> Source/JavaScriptCore/inspector/scripts/codegen/models.py:453
> +            allowed_strings = set(['itml', 'jscontext', 'service-worker', 'page', 'worker'])

Given this is used in 3 places maybe we should put "allowed_target_type_strings" somewhere more global in this file.

> Source/JavaScriptCore/inspector/scripts/codegen/models.py:457
> +

We should probably also validate that this is a subset of the Domain's target_types.

> Source/JavaScriptCore/inspector/scripts/codegen/models.py:460
> +                raise ParseException("Malformed domain specification: call parameter list for command %s has duplicate parameter names" % json['name'])

Bad error string. Should say "target types list for command %s has duplicate target names"

> Source/JavaScriptCore/inspector/scripts/codegen/models.py:499
> +

We should probably also validate that this is a subset of the Domain's target_types.

> Source/JavaScriptCore/inspector/scripts/codegen/models.py:502
> +                raise ParseException("Malformed domain specification: call parameter list for command %s has duplicate parameter names" % json['name'])

Bad error string. Should say "target types list for event %s has duplicate target names"

> Source/JavaScriptCore/inspector/scripts/tests/generic/domain-targetTypes.json:7
> +{
> +    "domain": "Domain",
> +    "targetTypes": ["page"],
> +    "commands": [{"name": "Command", "targetTypes": ["page"]}],
> +    "events": [{"name": "Event", "targetTypes": ["page"]}]
> +}

Would be nice to see tests when Event/Command targetsTypes do not match the Domain's targetTypes:

Valid:

    {
        "domain": "Domain",
        "debuggableTypes": ["itml", "page", "web-page"],
        "targetTypes": ["itml", "page"],
        "commands": [
            {"name": "Command1"},
            {"name": "Command2", "targetTypes": ["itml"]},
        ],
        "events": [
            {"name": "Event1"},
            {"name": "Event2", "targetTypes": ["itml"]},
        ]
  }

Invalid Command target:

    {
        "domain": "Domain",
        "targetTypes": ["page"],
        "commands": [
            {"name": "ValidCommand"},
            {"name": "InvalidCommand", "targetTypes": ["itml"]},
        ],
  }

Invalid Event target:

    {
        "domain": "Domain",
        "targetTypes": ["page"],
        "events": [
            {"name": "ValidEvent"},
            {"name": "InvalidEvent", "targetTypes": ["itml"]},
        ],
  }

>>>>>> Source/WebCore/inspector/InspectorFrontendClient.h:63
>>>>>> +    virtual String debuggableType() const = 0;
>>>>> 
>>>>> Should it return an enum value so that it's clear what are the all supported debuggable types?
>>>> 
>>>> Ideally, yes, but then it would complicate the logic of `RemoteWebInspectorProxy` and `InspectorFrontendHost.debuggableType()`.  We "fall back" to being a JavaScript debuggable if an unknown/invalid type is supplied, which is OK since all other types support JavaScript at the very least.  It would also make it ever-so-slightly more difficult for forks of WebKit (or other clients) to add support for Web Inspector debugging as they'd have to add an enum value.
>>> 
>>> Can you elaborate on how it'd complicate it? There is already debuggableTypeFromHost
>>> which translates from the constants used in c++ code to the ones used in the frontend.
>>> 
>>> If the forks cannot add support for their own type to the front-end they
>>> will face the same problem. So it seems that they'd have to either fall back
>>> to basic JavaScript inspector, pick from the existing types or modify WebKit to
>>> add new type anyway.
>> 
>> Thinking about this again (it's been a while), I think the only real "issue" with using an enum is handling the case where there is no value set (see `InspectorFrontendHost::debuggableType` for an example of how this may happen).  I think we can work around this though, so we probably should make this into an enum.
> 
> Yeah, when there is no frontend client debuggableType should have some meaningful fall back anyway, current return String() just defers that decision.

Yeah this could be an enum. Could do this separately, given this is pre-existing.

> Source/WebCore/testing/Internals.cpp:331
> +    String debuggableType() const final { return "protocol"_s; };

Would this trip the frontend up at all? Could be "page".

> Source/WebInspectorUI/.eslintrc:-37
>      "globals": {
> -        // Agents

Nice! Then ESLint can just tell use where they are!

> Source/WebInspectorUI/UserInterface/Base/Main.js:172
> +        if (InspectorBackend.domains.Target.hasCommand("exists")) {

This deserves a compatibility comment. Backends before a certain time had "exists" but now it can be feature checked immediately since a Target domain only exists for a WebPage debuggable (and not a Page debuggable). This can eventually go away!

> Source/WebInspectorUI/UserInterface/Base/Main.js:2759
> +    let target = WI.assumingMainTarget();
> +    target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value);

Whoa! NetworkAgent should work across multiple targets. So this is probably an existing bug and should just iterate all targets. (this could be pulled out into its own bug fix)

    for (let target of WI.targets) {
        if (target.NetworkAgent)
            target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value);
    }

> Source/WebInspectorUI/UserInterface/Base/Main.js:3043
> +    let target = WI.assumingMainTarget();
> +    target.DOMAgent.undo();

Heh, this may be difficult across multiple targets. We might end up with a stack in the frontend of targets to call undo/redo on.

>>>> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:32
>>>> +        switch (InspectorFrontendHost.debuggableType()) {
>>> 
>>> Why does debuggable type have to come from InspectorFrontendHost? Can't it be derived from the inspected target type?
>> 
>> At this point, we haven't connected to any targets yet, so we don't know anything about it.  Theoretically, we can determine the `debuggableType` information from the first target we connect to, but we need to know about the `debuggableType` _before_ that happens too (see above for more in-depth explanation).
> 
> The UI could start assuming minimal feature set (i.e. JSContext) and extend it later
> depending on the main target type. But as you mentioned it may lead to undesirable
> UI transformations. To avoid that you want to learn event before the connection is
> established what kind of target you are going to inspect. Using a native binding helps
> achieve this but seems like a side channel. Imagine e.g. connection to the remote
> target over a web socket from a frontend loaded as a regular page (no RemoteInspectorUI
> bindings at all). Native InspectorFrontendHost would not be available but the same
> functionality could be achieved via e.g. passing arguments to the front-end url.
> Anyway, it's not directly related to this change.

"it may lead to undesirable UI transformations" - Precisely!

"... functionality could be achieved via e.g. passing arguments to the front-end url" - Correct, in such a frontend configuration the state would need to be passed in another way, like you're suggesting.

> Source/WebInspectorUI/UserInterface/Controllers/AppController.js:52
> +        case "web": // COMPATIBILITY (iOS 13): replaced by "page" (WK1) and "web-page" (WK2).
> +        case "web-page":
> +            this._debuggableType = WI.DebuggableType.WebPage;
> +            break;

Maybe instead of (WK1) and (WK2) you could say (WebCore::Page) and (WebKit::WebPage).

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:319
> +        let target = WI.assumingMainTarget();

Can these all be `frame.target` or are we waiting to eliminate assumingMainTarget's in the future?

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:339
> -        RuntimeAgent.evaluate.invoke({expression, objectGroup: "", includeCommandLineAPI: false, doNotPauseOnExceptionsAndMuteConsole: true, contextId, returnByValue: false, generatePreview: false}, documentAvailable);
> +        target.RuntimeAgent.evaluate.invoke({expression, objectGroup: "", includeCommandLineAPI: false, doNotPauseOnExceptionsAndMuteConsole: true, contextId, returnByValue: false, generatePreview: false}, documentAvailable);

The reason I suggested that is because we should have implemented RuntimeAgent to be multi-target aware. I see this was not done because it is assuming the page / frame target.

> Source/WebInspectorUI/UserInterface/Controllers/ConsoleManager.js:49
> +        return !!InspectorBackend.domains.Console.hasCommand("getLoggingChannels");

We can remove the `!!` from these cases.

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:458
> -    highlightDOMNode(nodeId, mode)
> +    highlightDOMNode(node, mode)

A change like this could be split out into its own patch.

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:50
> +        return !!InspectorBackend.domains.Runtime.hasCommand("awaitPromise");

Nit: You can drop the `!!`.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:561
> +        let target = WI.assumingMainTarget();

Nit: Why not `this.target` for this one?

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:581
> +        let target = WI.assumingMainTarget();

Nit: Why not `this.target` for this one?

> Source/WebInspectorUI/UserInterface/Protocol/ConsoleObserver.js:38
> -        WI.consoleManager.messageWasAdded(this.target, message.source, message.level, message.text, message.type, message.url, message.line, message.column || 0, message.repeatCount, message.parameters, message.stackTrace, message.networkRequestId);
> +        WI.consoleManager.messageWasAdded(this._target, message.source, message.level, message.text, message.type, message.url, message.line, message.column || 0, message.repeatCount, message.parameters, message.stackTrace, message.networkRequestId);

Seems okay to switch to `this._target` for Dispatcher subclasses, but `this.target` would be our normal style.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:167
> +        domain.addCommand(targetTypes, new InspectorBackend.Command(qualifiedName, callSignature, replySignature));

This could pass the `commandName` here to avoid another `split` in the Command constructor.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:174
> +        domain.addEvent(targetTypes, new InspectorBackend.Event(qualifiedName, signature));

This could pass the `eventName` here to avoid another `split` in the Event constructor.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:273
> +        targetTypes = targetTypes || Object.values(WI.TargetType);

Shouldn't the targetTypes be limited to the domain's target types? If a domain is limited to "service-worker" I'd expect the commands to only be limited to "service-worker" as well, not all targets types.

It seems like this is the common case, so currently this produces a lot of duplication (a map per-service type).

>>>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:295
>>>>>> +        console.assert(this._commandParameters.has(commandName) === this._commandReturns.has(commandName));
>>>>> 
>>>>> It was impossible to have such problem in the previous code where each command was represented by an object instead of several multimaps that have to be kept in sync. Also
>>>>>   if (InspectorBackend.domains.Timeline.setAutoCaptureEnabled)
>>>>> is a shorter than
>>>>>   if (InspectorBackend.domains.Timeline.hasCommand("setAutoCaptureEnabled"))
>>>>> and it would be possible to validate former with a js compiler should you decide to use one. 
>>>>> I wonder what is the motivation for changing this to string keyed multimaps?
>>>> 
>>>> I can't remember all the reasons why I decided to take this approach, but yes it does make global feature checking slightly more verbose.
>>>> 
>>>> I personally prefer a more verbose/explicit approach, as it more clearly distinguishes callsites from feature-checks, and there is some precedent with `hasEvent`.  I would rather us have a very clear distinction between those two use cases.
>>>> 
>>>> FWIW, `hasCommandReturn` also isn't used right now, so we could remove the code related to that in this patch, but it does make a more complete API surface.
>>> 
>>> I agree that hasCommand/hasParameter etc better reflect the intent of the call, I'm just not a fan of using raw constants especially when we can generate them and utilize some automatic checks, otherwise the code relies solely on the tests and reviewers. If you are changing the signatures of feature checkers anyway, would it make sense to make the calls less verbose by supporting something like this:
>>> 
>>> Backend.hasCommand("Target.exists");
>>> 
>>> instead of 
>>> 
>>> InspectorBackend.domains.Target && InspectorBackend.domains.Target.hasCommand("exists")
>>> 
>>> and similar methods for checking parameters and return values?
>> 
>> My only hesitance to your suggestion is that it would then likely involve a `String.prototype.split`, which is more work than just a Map/Object lookup.  I'd prefer to avoid doing work where we can, but other than that, I'm fine with it.
> 
> I wouldn't worry about performance here, this code runs very rarely (mostly once for some small number of commands?) and 'split' call itself is very cheap. I'd be very surprised if any of these methods appeared in a CPU profile. Unless there is profile data indicating performance bottleneck I personally would prefer more readable code to premature optimizations.

I also thought while reading this that `InspectorBackend.hasCommand("Target.exists")` might work out well. It certainly reads a bit nicer than the long combo, and there is likely to be human error forgetting the domain check. Both approaches have the possibility of human error mistyping the last piece (aside from a JS compiler / static check approach Yury mentions). I agree that performance shouldn't be a concern if we did this (every protocol message is doing this).

>>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:301
>>>>> +        return this._commandParameters.has(commandName, commandParameterName);
>>>> 
>>>> What is the value of creating _commandParameters and _commandReturns multimaps over storing a map name->InspectorBackend.Command and then
>>>> writing something like
>>>> 
>>>> const command = this._command[commandName];
>>>> return command && command.hasParameter(commandParameterName);
>>>> 
>>>> This is way you'd have only one copy of the command metadata and would use that for both feature detection and agent construction.
>>> 
>>> Ah I remember why I did this!  So one reason for this is that we could theoretically have multiple versions of the same command/event so long as there's no overlap in the `targetTypes`, meaning that the same command/event name could map to multiple `InspectorBackend.Command`/`InspectorBackend.Event`.  This would be a very useful thing to have for compatibility reasons for ITML (WebKit can change without breaking ITML).
>> 
>> How is that possible? Can a backend talk several different versions of the protocol?
>> 
>> If it's possible to have different versions of the same domain on different target types should registerVersion take that into account and version individual commands / sets of commands?
> 
> There's nothing preventing this from happening.  Fundamentally, the protocol is just an organizing "framework" around JSON-RPC-like payloads being sent over a connection.  Each backend would likely only actually have one "version" of each command/event, but the protocol JSON files are effectively a combination of the capabilities all of the various "platforms" that are inspectable.  As an example, `DOM.getDataBindingsForNode` and `DOM.getAssociatedDataForNode` are both ITML-specific features that we would likely not want to show for non-ITML targets, so we'd only want those to be created if `debuggableType === "ITML"`.  The primary reason we'd want to do this, however, is for situations where WebKit changes any of the parameters/returns for a command/event that ITML doesn't want, so we'd be able to copy the current command/event and change the `targetTypes` to just be "ITML", and then edit the original command/event to have `targetTypes` for everything else.
> 
> Theoretically, C++ supports function overloading, and the various `*BackendDispatcherHandler` objects are generated from the JSON files anyways, so as long as two commands with the same name don't have the same signature (e.g. the types of the parameters/returns are the same) we could support switching based on the types of the arguments.  The intention for now isn't to support this, as that needs other support in C++ to do that switching, but it is a possibility that we may want to have in the future.

I do think that the multi maps are overkill. Even with the `DOM.getDataBindingsForNode` case which is restricted to "ITML", then if the frontend were to do a straight forward feature check:

    InspectorBackend.domains.DOM.hasCommand("getDataBindingsForNode")

... it seems to me that this would always appear appear to always be supported regardless of an ITML target or not. So we'd still end up doing an extra check at feature detection points (assuming we don't have a Target), right?

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:332
> +        let command = Array.from(commands).find((command) => command.commandName === commandName);
> +        console.assert(command);

I think we can avoid allocations from `Array.from(commands)` and iterate the Set ourselves. The temporary array seems wasteful.

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:427
> +        "use strict";

This `"use strict"` no longer makes sense. We are inside of a ClassMethod so it is strict.

> Source/WebInspectorUI/UserInterface/Protocol/Legacy/13.0/InspectorBackendCommands.js:160
> +InspectorBackend.registerCommand("DOM.getDataBindingsForNode", ["itml"], [{"name": "nodeId", "type": "number"}], ["dataBindings"]);
> +InspectorBackend.registerCommand("DOM.getAssociatedDataForNode", ["itml"], [{"name": "nodeId", "type": "number"}], ["associatedData"]);

Are these the only commands with a specific target type?

> Source/WebInspectorUI/UserInterface/Test/Test.js:207
> +// FIXME: <https://webkit.org/b/201149> Web Inspector: replace all uses of `window.*Agent` with a target-specific call
> +(function() {
> +    function makeAgentGetter(domainName) {

Nice!

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:239
> -        let domNode = this.domNode;
> +        let target = WI.assumingMainTarget();

Another case where it seems like we should do:

    let target = domNode.target;

Assuming you want to replace many of these `assumingMainTarget` in this patch now. It can be done later.

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:117
> -        if (!WI.mainTarget || !WI.mainTarget.executionContext)
> +        if (!WI.mainTarget || WI.mainTarget instanceof WI.MultiplexingBackendTarget)
>              return;

Seems fine. I wonder if we should have done something smarter here. In general I recall this was a particularly ugly corner of UI initialization =(

>>>>>> Source/WebInspectorUI/Versions/Inspector-iOS-9.3.json:3267
>>>>>> +    "debuggableTypes": ["page", "web-page"],
>>>>> 
>>>>> Looking at this code it's quite hard to guess how web-page is different from page. Maybe legacy-page like Greg suggests or wk1-page/wk2-page ?
>>>> 
>>>> It's not WK1-page vs WK2-page, because a sub-target of "web-page" is "page" (so my bad for suggesting that).  It really just mirrors the type of object that we're directly connecting to (`WebCore::Page` vs `WebKit::WebPage`).  The same is true of the other types too (e.g. `JSC::JSContext`).
>>> 
>>> The reason I referred to them as WK1 and WK2 is simply because that's where they're used (e.g. you won't see a "web-page" when trying to inspect something using WK1).
>> 
>> I had an impression that WebPage is WK2 concept used to communicate with its proxy in UIProcess and it doesn't exist in WK1.
>> 
>> Is there any significant difference between page and web-page except for the Target domain?
> 
> As far as the protocol, I don't think there are any other differences, but there could be, especially since we support a "featureGuard" concept.
> 
> There can be implicit differences, however, like WK2 having the concept of user interaction (different from user gesture) <https://webkit.org/b/197269>.

Correct, today there are no differences.

A "web-page" with a Target domain today just holds a top level Page target (which is a "page" target) and transitions between pages.

One step forward would be extending that Target domain to include another top level object related to the page, e.g. a Service Worker. In this scenario both the Page and Service Worker are just normal "page" and "service-worker" targets:

           Target
             |
            / \
           /   \
         Page  Service Worker

Another step forward, would be when we have a process per-frame, there will likely still be the concept of a Page and a new concept of a Frame target:

           Target
             |---------------------------
            / \                          \
           /   \                         |
         Page  Page (provisional)   Service Worker
          |
         / \
        /   \
     Frame  Frame
       |      |
       |      |
     Frame  Worker

We haven't yet mapped this out in complete detail yet. For example in this scenario if the Page with out-of-process frames will need to be a different Page type or not. I don't think we'll know that until that implementation comes closer to reality.

[Devin Rousso]

Comment on attachment 377553 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/Audit.json:4
>> +    "targetTypes": ["itml", "jscontext", "page", "service-worker", "worker"],
> 
> Do Audits work in a Worker today?
> 
> I know they work in a jscontext / service-worker though I still think that is probably non-ideal. Unless we can produce `unsupported` results based on the target.

Yes they're supported.  We choose to not show them the frontend, as there's not much they can currently do for non-web contexts.

>> Source/JavaScriptCore/inspector/scripts/codegen/models.py:457
>> +
> 
> We should probably also validate that this is a subset of the Domain's target_types.

This wouldn't entirely work, as "worker" isn't considered a valid `debuggableType`.  Also, the current design has it such that the `Domain` doesn't exist until after all `Command`/`Event` are created, so we'd instead have to provide the `debuggableTypes` as a parameter.  I'll work something out.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:2759
>> +    target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value);
> 
> Whoa! NetworkAgent should work across multiple targets. So this is probably an existing bug and should just iterate all targets. (this could be pulled out into its own bug fix)
> 
>     for (let target of WI.targets) {
>         if (target.NetworkAgent)
>             target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value);
>     }

<https://webkit.org/b/203025>

>> Source/WebInspectorUI/UserInterface/Protocol/ConsoleObserver.js:38
>> +        WI.consoleManager.messageWasAdded(this._target, message.source, message.level, message.text, message.type, message.url, message.line, message.column || 0, message.repeatCount, message.parameters, message.stackTrace, message.networkRequestId);
> 
> Seems okay to switch to `this._target` for Dispatcher subclasses, but `this.target` would be our normal style.

`InspectorBackend.Dispatcher` doesn't have a `get target`, so `this.target` doesn't exist.  I don't think we "need" to expose the associated `WI.Target`, as nothing should ever be referring directly to this object anyways.

>>>>>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:301
>>>>>> +        return this._commandParameters.has(commandName, commandParameterName);
>>>>> 
>>>>> What is the value of creating _commandParameters and _commandReturns multimaps over storing a map name->InspectorBackend.Command and then
>>>>> writing something like
>>>>> 
>>>>> const command = this._command[commandName];
>>>>> return command && command.hasParameter(commandParameterName);
>>>>> 
>>>>> This is way you'd have only one copy of the command metadata and would use that for both feature detection and agent construction.
>>>> 
>>>> Ah I remember why I did this!  So one reason for this is that we could theoretically have multiple versions of the same command/event so long as there's no overlap in the `targetTypes`, meaning that the same command/event name could map to multiple `InspectorBackend.Command`/`InspectorBackend.Event`.  This would be a very useful thing to have for compatibility reasons for ITML (WebKit can change without breaking ITML).
>>> 
>>> How is that possible? Can a backend talk several different versions of the protocol?
>>> 
>>> If it's possible to have different versions of the same domain on different target types should registerVersion take that into account and version individual commands / sets of commands?
>> 
>> There's nothing preventing this from happening.  Fundamentally, the protocol is just an organizing "framework" around JSON-RPC-like payloads being sent over a connection.  Each backend would likely only actually have one "version" of each command/event, but the protocol JSON files are effectively a combination of the capabilities all of the various "platforms" that are inspectable.  As an example, `DOM.getDataBindingsForNode` and `DOM.getAssociatedDataForNode` are both ITML-specific features that we would likely not want to show for non-ITML targets, so we'd only want those to be created if `debuggableType === "ITML"`.  The primary reason we'd want to do this, however, is for situations where WebKit changes any of the parameters/returns for a command/event that ITML doesn't want, so we'd be able to copy the current command/event and change the `targetTypes` to just be "ITML", and then edit the original command/event to have `targetTypes` for everything else.
>> 
>> Theoretically, C++ supports function overloading, and the various `*BackendDispatcherHandler` objects are generated from the JSON files anyways, so as long as two commands with the same name don't have the same signature (e.g. the types of the parameters/returns are the same) we could support switching based on the types of the arguments.  The intention for now isn't to support this, as that needs other support in C++ to do that switching, but it is a possibility that we may want to have in the future.
> 
> I do think that the multi maps are overkill. Even with the `DOM.getDataBindingsForNode` case which is restricted to "ITML", then if the frontend were to do a straight forward feature check:
> 
>     InspectorBackend.domains.DOM.hasCommand("getDataBindingsForNode")
> 
> ... it seems to me that this would always appear appear to always be supported regardless of an ITML target or not. So we'd still end up doing an extra check at feature detection points (assuming we don't have a Target), right?

It depends on the connected `debuggableType`.  We wouldn't even create an `InspectorBackend.Command` for `DOM.getDataBindingsForNode` unless the `debuggableType` is ITML (as specified in 'DOM.json').  If we removed that restriction, then yes, that would return `true`.

But I think this is useful, as it allows us to answer multiple questions at the same time (assuming we have a `WI.Target`):
 - `InspectorBackend.domains.DOM.hasCommand("getDataBindingsForNode")` answers whether I will ever need this command, if I connect to an ITML `WI.Target`?
 - `target.hasCommand("getDataBindingsForNode")` answers whether the current `WI.Target` supports this command (e.g. I need it right now)?

>> Source/WebInspectorUI/UserInterface/Protocol/Legacy/13.0/InspectorBackendCommands.js:160
>> +InspectorBackend.registerCommand("DOM.getAssociatedDataForNode", ["itml"], [{"name": "nodeId", "type": "number"}], ["associatedData"]);
> 
> Are these the only commands with a specific target type?

For now, I believe so.  I'd love to be able to move all of `DOMDebugger` to `Debugger`, which would use a similar concept.

[Devin Rousso]

Created attachment 381064 [details]
Patch

[Devin Rousso]

Created attachment 381088 [details]
Patch

Added some extra `console.assert` in potentially accident prone places, like `hasCommand` being used instead of `hasDomain`.

[Devin Rousso]

Created attachment 381095 [details]
Patch

Fixed more mistakes, this time with Canvas.

[Devin Rousso]

Created attachment 381102 [details]
Patch

Caught a few more related to Network

[Devin Rousso]

Created attachment 381115 [details]
Patch

Another one for DOM

[Joseph Pecoraro]

> >> Source/WebInspectorUI/UserInterface/Protocol/ConsoleObserver.js:38
> >> +        WI.consoleManager.messageWasAdded(this._target, message.source, message.level, message.text, message.type, message.url, message.line, message.column || 0, message.repeatCount, message.parameters, message.stackTrace, message.networkRequestId);
> > 
> > Seems okay to switch to `this._target` for Dispatcher subclasses, but `this.target` would be our normal style.
> 
> `InspectorBackend.Dispatcher` doesn't have a `get target`, so `this.target`
> doesn't exist.  I don't think we "need" to expose the associated
> `WI.Target`, as nothing should ever be referring directly to this object
> anyways.

We could make `this._target` just be `this.target`. We've done this in a few places. It seems simpler in my opinion (a property access not going through a getter). I've promoted that for some of our Model objects. I don't know the ultimate performance impact, I believe JSC has an optimization in place for `get foo() { return this._foo; }` but there is still several levels of waste.

[Joseph Pecoraro]

Comment on attachment 381115 [details]
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/InspectorBackendCommands.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.

[Joseph Pecoraro]

Comment on attachment 381115 [details]
Patch

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

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

To be clear, your change is primarily a transformation so I'm find with not doing any upgrades now. But perhaps this is a worthy follow-up: upgrading the checks in a lot of these loops to be a little stricter, for the reason I've stated above.

[Devin Rousso]

Comment on attachment 381115 [details]
Patch

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

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:86
>> +    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.

We no longer have a `JavaScript` type for this, since it's expected that a `WebPage` debuggable will never have a `JavaScript` sub-target.  Also, we should never hit this anyways, so I think it's fine to leave this as a `Page`.

>> Source/WebInspectorUI/UserInterface/Base/Multimap.js:76
>> +            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).

Interestingly, the current patch actually needs this functionality, as many commands/events don't have any parameters, and therefore would have an empty `valueSet`.  I guess we probably shouldn't use a `Multimap`, even though the underlying idea is what we want.  I'll make a similar data structure.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:66
>>      }
> 
> 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.

I think we should keep it as `_activeDomains`.  I'd rather keep the "contract" that you cannot refer to _anything_ for a domain that isn't activated, almost as if it doesn't even exist.

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:397
>>          }
> 
> Will these errors work anymore? it doesn't look like `this._domainName` exists anymore.

Good catch!  I think we can move most of this logic to 'Connection.js' where we do have a `domainName`.

>> Source/WebInspectorUI/UserInterface/Protocol/Legacy/10.0/InspectorBackendCommands.js:40
>> +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.

We could totally do an optimization for this =D

>>> 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.
> 
> To be clear, your change is primarily a transformation so I'm find with not doing any upgrades now. But perhaps this is a worthy follow-up: upgrading the checks in a lot of these loops to be a little stricter, for the reason I've stated above.

We definitely should do this, especially for the ITML situation you described.  I didn't upgrade these mainly because `Page.setCompositingBordersVisible` (and things like it) exist in every version of the protocol that supports the `Page` domain, so if `Page` exists then so must this function.  I'll likely address many of these in followups for the ITML cases.

[Devin Rousso]

Created attachment 381165 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 381165 [details]:

imported/w3c/web-platform-tests/IndexedDB/fire-error-event-exception.html bug 202799 (author: [e-mail])
imported/w3c/web-platform-tests/mathml/presentation-markup/operators/mo-form-fallback.html bug 203076 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 381165 [details]
Patch

Clearing flags on attachment: 381165

Committed r251227: <https://trac.webkit.org/changeset/251227>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.