Bug 191345 - Web Inspector: Start moving toward better multi-target support
Summary: Web Inspector: Start moving toward better multi-target support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-06 20:22 PST by Joseph Pecoraro
Modified: 2018-11-09 11:25 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (37.23 KB, patch)
2018-11-06 20:38 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (37.34 KB, patch)
2018-11-08 12:29 PST, Joseph Pecoraro
drousso: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2018-11-06 20:22:30 PST
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.
Comment 1 Joseph Pecoraro 2018-11-06 20:38:37 PST
Created attachment 354057 [details]
[PATCH] Proposed Fix

I assume this won't apply, but I'll get it up for now.
Comment 2 Devin Rousso 2018-11-06 22:53:52 PST
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)
Comment 3 Joseph Pecoraro 2018-11-07 12:39:20 PST
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.
Comment 4 Joseph Pecoraro 2018-11-08 12:29:28 PST
Created attachment 354261 [details]
[PATCH] Proposed Fix
Comment 5 Devin Rousso 2018-11-08 22:55:36 PST
Comment on attachment 354261 [details]
[PATCH] Proposed Fix

r=me
Comment 6 WebKit Commit Bot 2018-11-08 22:57:31 PST
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
Comment 7 Joseph Pecoraro 2018-11-09 11:24:37 PST
https://trac.webkit.org/changeset/238048/webkit
Comment 8 Radar WebKit Bug Importer 2018-11-09 11:25:23 PST
<rdar://problem/45950197>