Bug 191345 – Web Inspector: Start moving toward better multi-target support

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

[Joseph Pecoraro]

Start moving toward better multi-target support

A world where we do:

    if (PageAgent.foo)
        PageAgent.foo(...);

Is ambiguous in a multi-target world:

  • Which target is the `PageAgent.foo` command going to go to?
  • Which target is the `PageAgent.foo` feature check happening on? Is that important?

Ultimately, the behavior that we would like to have is to always do:

  • target.PageAgent

Then it is clear exactly which target we are operating on.

Another possibility is that we don't care or have a Target, we just want to "feature check" (common for enums as well)

    if (FooAgent.hasEvent("someEvent"))
       ... support something else ...

In these cases, we should have an agnostic way to just get the complete list of whatever the backend supports:

    InspectorBackend.domains.Foo.hasEvent("someEvent")

The key part being:

    InspectorBackend.domains.<DomainName>

---

For now we have hundreds of places in our code that are using unprefixed or window prefixed agents. As we bring up multi-target support we should start eliminating these. Currently the behavior is they use the "Main Target Connection". That ends up being fine right now as the main target is limited to being a Page or ServiceWorker debuggable. But as we move toward supporting multiple such targets each unprefixed access to "PageAgent", "DOMAgent", "NetworkAgent", etc will need to be explicit per-target.

[Joseph Pecoraro]

Created attachment 354057 [details]
[PATCH] Proposed Fix

I assume this won't apply, but I'll get it up for now.

[Devin Rousso]

Comment on attachment 354057 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:692
> +        if (!InspectorBackend.domains.Debugger.setPauseOnAssertions) {

Why not use `target.DebuggerAgent` here?

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:65
> +            if (target.NetworkAgent.setResourceCachingDisabled && WI.settings.resourceCachingDisabled && WI.settings.resourceCachingDisabled.value)

The second check for `WI.settings.resourceCachingDisabled` is unnecessary as it always exists

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:66
> +                target.NetworkAgent.setResourceCachingDisabled(true);

You could just call this function with the `WI.Setting`'s value instead of using `true`:

    if (target.NetworkAgent.setResourceCachingDisabled)
        target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value);

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:44
> +        if (target.RuntimeAgent.enableTypeProfiler && WI.settings.showJavaScriptTypeInformation && WI.settings.showJavaScriptTypeInformation.value)

Ditto (NetworkManager.js:65)

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:48
> +        if (target.RuntimeAgent.enableControlFlowProfiler && WI.settings.enableControlFlowProfiler && WI.settings.enableControlFlowProfiler.value)

Ditto (NetworkManager.js:65)

> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:135
> +        let executionContextId = this._activeExecutionContext.id;

Considering that this is only used in one place, I'd be fine just inlining it there

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:726
> +        if (!window.TimelineAgent)

Shouldn't this follow the `domains` approach?

    if (!InspectorBackend.domains.Timeline)

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1095
> +            target.TimelineAgent.setInstruments([...instrumentSet]);

`Array.from` should be faster/simpler than constructing an array using spread
<https://jsperf.com/set-iterator-vs-foreach/4>

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:178
> +        if (!InspectorBackend.domains.DOM.hasEvent("pseudoElementAdded"))

While we're doing this change, is there no other protocol check we can perform for iOS9?  Something in one of the "always available" domains?  Perhaps that `RuntimeAgent.enableTypeProfiler` exists?

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:75
> +            if (this.PageAgent.setShowPaintRects && WI.settings.showPaintRects.value)

Ditto (NetworkManager.js:65)

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:176
> +                    result.release();

Shouldn't we still release in the case that the `result.type !== "function"`?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104
> +        if (window.PageAgent && PageAgent.setShowRulers)

Ditto (TimelineManager.js:726)

    if (InspectorBackend.domains.Page.setShowRulers)

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:131
> +        if (window.HeapAgent && HeapAgent.gc)

Ditto (TimelineManager.js:726)

    if (InspectorBackend.domains.Heap.gc)

[Joseph Pecoraro]

Comment on attachment 354057 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:692
>> +        if (!InspectorBackend.domains.Debugger.setPauseOnAssertions) {
> 
> Why not use `target.DebuggerAgent` here?

This is using `setPauseOnAssertions` as an agnostic feature check for "we are after iOS 10 or not". I think its more of an agnostic feature check then a per-target check. But I'll still switch to the target version here.

>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:66
>> +                target.NetworkAgent.setResourceCachingDisabled(true);
> 
> You could just call this function with the `WI.Setting`'s value instead of using `true`:
> 
>     if (target.NetworkAgent.setResourceCachingDisabled)
>         target.NetworkAgent.setResourceCachingDisabled(WI.settings.resourceCachingDisabled.value);

Done. Some of these we probably just wanted to avoid a message to the backend if we know we were matching the default value, like sending false when the backend should be false.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:726
>> +        if (!window.TimelineAgent)
> 
> Shouldn't this follow the `domains` approach?
> 
>     if (!InspectorBackend.domains.Timeline)

Ultimately for multi-target support this code will need to attempt to enable auto capture on one or more targets. So this will have to be revisited later. So I used `window.TargetAgent` to make it clear that we should revisit it.

>> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:1095
>> +            target.TimelineAgent.setInstruments([...instrumentSet]);
> 
> `Array.from` should be faster/simpler than constructing an array using spread
> <https://jsperf.com/set-iterator-vs-foreach/4>

Done.

>> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:178
>> +        if (!InspectorBackend.domains.DOM.hasEvent("pseudoElementAdded"))
> 
> While we're doing this change, is there no other protocol check we can perform for iOS9?  Something in one of the "always available" domains?  Perhaps that `RuntimeAgent.enableTypeProfiler` exists?

This is a difference between 9.0 and 9.3 and later... So our options are limited (none in the big 3 Runtime/Debugger/Console). I'm going to keep this because it was the most accurate at the time.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:75
>> +            if (this.PageAgent.setShowPaintRects && WI.settings.showPaintRects.value)
> 
> Ditto (NetworkManager.js:65)

I think here we are avoiding sending `false` to the backend when we know the backend should already be starting out `false`.

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:176
>> +                    result.release();
> 
> Shouldn't we still release in the case that the `result.type !== "function"`?

Agreed, I cleaned this up to:

    remoteObject.getProperty("constructor", (error, result, wasThrown) => {
        if (error)
            return;
        if (result.type === "function")
            remoteObject.target.DebuggerAgent.getFunctionDetails(result.objectId, didGetFunctionDetails);
        result.release();
    });

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:104
>> +        if (window.PageAgent && PageAgent.setShowRulers)
> 
> Ditto (TimelineManager.js:726)
> 
>     if (InspectorBackend.domains.Page.setShowRulers)

Same thing, regarding multi-target support.

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:131
>> +        if (window.HeapAgent && HeapAgent.gc)
> 
> Ditto (TimelineManager.js:726)
> 
>     if (InspectorBackend.domains.Heap.gc)

Same thing regarding multi-target support.

[Joseph Pecoraro]

Created attachment 354261 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 354261 [details]
[PATCH] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 354261 [details]
[PATCH] Proposed Fix

Rejecting attachment 354261 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 354261, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebInspectorUI/ChangeLog is not at the top of the file.

Full output: https://webkit-queues.webkit.org/results/9920061

[Joseph Pecoraro]

https://trac.webkit.org/changeset/238048/webkit

[Radar WebKit Bug Importer]

<rdar://problem/45950197>