Bug 191052 – Web Inspector: Separate target initialization from frontend initialization

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Web Inspector: Separate target initialization from frontend initialize

This will be useful when we want to allow the frontend to connect to multiple targets at the same time. Managers still won't have complete support for dealing with multiple targets of the same advanced type, but this is a good step to get use there.

[Radar WebKit Bug Importer]

<rdar://problem/45658384>

[Joseph Pecoraro]

Created attachment 353347 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

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

Attachment 353347 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9775183

New failing tests:
inspector/worker/debugger-pause.html
http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html

[EWS Watchlist]

Created attachment 353352 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

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

Attachment 353347 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9775222

New failing tests:
inspector/worker/debugger-pause.html
http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html

[EWS Watchlist]

Created attachment 353353 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

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

Attachment 353347 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9775289

New failing tests:
inspector/worker/debugger-pause.html
http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html

[EWS Watchlist]

Created attachment 353356 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

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

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

I'll hold off on giving it a r+ until the function duplicate issues are resolved.

> Source/WebInspectorUI/ChangeLog:15
> +        messages during frontend initialize now happen with an explicit target.

NIT: "initialize" => "initialization"

> Source/WebInspectorUI/UserInterface/Base/Main.js:100
> +    this.managers = [

😏 <https://webkit.org/b/190165>

> Source/WebInspectorUI/UserInterface/Base/Main.js:101
> +        this.targetManager = new WI.TargetManager,

NIT: I know we tend to use `this` inside Main.js, but I find that it makes things harder to search for globally.  I'd prefer if you named these `WI.` instead of `this.`.

> Source/WebInspectorUI/UserInterface/Base/Main.js:-177
> -        PageAgent.setShowPaintRects(true);

Should this be re-added somewhere?

> Source/WebInspectorUI/UserInterface/Base/Main.js:485
> +WI.performOneTimeInitializationWithTarget = function(target)

This name implies that each one-time-initialization is per-target.  Are these initialize functions per-target, or across all targets?  If the latter, I think something like `performOneTimeInitializationsUsingTarget` may be more accurate.

> Source/WebInspectorUI/UserInterface/Base/Main.js:668
> +    if (WI.mainTarget && WI.mainTarget.CSSAgent)
> +        WI.CSSCompletions.initializeCSSCompletions(WI.assumingMainTarget());

I realize that this was here already, but is it actually needed?

> Source/WebInspectorUI/UserInterface/Controllers/ApplicationCacheManager.js:26
> +// FIXME: ApplicationCacheManager lacks advanced multi-target support. (ApplciationCache objects per-target)

NIT: I'd prefer putting this FIXME inside `initializeTarget` as that's where it's most likely to be "noticed" (in addition to where it will most likely involve any work).

> Source/WebInspectorUI/UserInterface/Controllers/ConsoleManager.js:171
> +            for (let channel of channels) {
> +                console.assert(this._loggingChannelSources.includes(channel.source));
> +            }

Instead of including braces for when we strip `console.assert`, use `Array.prototype.every`:

    console.assert(channels.every((channel) => this._loggingChannelSources.includes(channel.source));

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:119
>          if (DebuggerAgent.setPauseOnAssertions)

Should this check that `target.DebuggerAgent` exists?  Additionally, shouldn't it be `target.DebuggerAgent.setPauseOnAssertions`?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:123
>          if (DebuggerAgent.setAsyncStackTraceDepth)

Ditto (119)

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:127
> +        if (DebuggerAgent.setPauseForInternalScripts)

Ditto (119)

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:556
>      initializeTarget(target)

I think you meant to remove this function, as it's somewhat a duplicate of the one above.

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

You could move `WI.showJavaScriptTypeInformationSetting` into `WI.settings` and avoid this first check.

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

Ditto (44)

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:72
> +    initializeTargetsWithMainTarget()

This makes me think that it will loop over `WI.managers` and call `initializeTarget` with `WI.mainTarget` for each.  If that is not the case, it could use a better name 😅

[Blaze Burg]

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

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

Looks almost ready, setting r- due to EWS failures.

> Source/WebInspectorUI/ChangeLog:78
> +        only needs to be performed once on a target that supports it.

👍🏻

> Source/WebInspectorUI/ChangeLog:85
> +        each of the managers to do initializtion work for this target.

Nit: initialization

>> Source/WebInspectorUI/UserInterface/Base/Main.js:485
>> +WI.performOneTimeInitializationWithTarget = function(target)
> 
> This name implies that each one-time-initialization is per-target.  Are these initialize functions per-target, or across all targets?  If the latter, I think something like `performOneTimeInitializationsUsingTarget` may be more accurate.

I had the same comment. It seems to be once-per-frontend-app, except the ensureDocument call would be per-target?

> Source/WebInspectorUI/UserInterface/Base/Main.js:497
> +    // FIXME: This should move to DOMManager.initializeTarget when adding better DOM multi-target support since it is really per-target.

I'd prefer we just move this code over now. The initializeTarget method can be filled in more later.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:668
>> +        WI.CSSCompletions.initializeCSSCompletions(WI.assumingMainTarget());
> 
> I realize that this was here already, but is it actually needed?

Ditto, isn't it redundant with the call in WI.performOneTimeInitializationWithTarget()?

> Source/WebInspectorUI/UserInterface/Controllers/ApplicationCacheManager.js:44
> +    initializeTarget(target)

I really like this pattern.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:131
> +            targetData.pauseIfNeeded();

This reads poorly, what is the type of targetData?

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:66
> +        // initialization messages will have their responses come in before a potential bulk

Nit: This sentence is ungrammatical.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:70
> +        setTimeout(() => {

Seems weird to use two setTimeouts back to back. Should this use Promise.all?

> Source/WebInspectorUI/UserInterface/Protocol/WorkerTarget.js:32
> +        this._executionContext = new WI.ExecutionContext(this, WI.RuntimeManager.TopLevelContextExecutionIdentifier, this.displayName, false, null);

This is a lot nicer now!

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:101
>> +        this.targetManager = new WI.TargetManager,
> 
> NIT: I know we tend to use `this` inside Main.js, but I find that it makes things harder to search for globally.  I'd prefer if you named these `WI.` instead of `this.`.

I agree, changed these to `WI`.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:-177
>> -        PageAgent.setShowPaintRects(true);
> 
> Should this be re-added somewhere?

Oops, yes, Great catch.  This was lost in this change but was in my larger set of patches. Let me add it back. We just didn't happen to have a Manager for "PageAgent" stuff that this fits well into. The NetworkManager is a bit of a mess right now and this doesn't belong there. Maybe LayerTreeManager, but for now I'll just put it directly into Target.initialize.

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:485
>>> +WI.performOneTimeInitializationWithTarget = function(target)
>> 
>> This name implies that each one-time-initialization is per-target.  Are these initialize functions per-target, or across all targets?  If the latter, I think something like `performOneTimeInitializationsUsingTarget` may be more accurate.
> 
> I had the same comment. It seems to be once-per-frontend-app, except the ensureDocument call would be per-target?

I renamed this to `WI.performOneTimeFrontendInitializationsUsingTarget`

>> Source/WebInspectorUI/UserInterface/Base/Main.js:497
>> +    // FIXME: This should move to DOMManager.initializeTarget when adding better DOM multi-target support since it is really per-target.
> 
> I'd prefer we just move this code over now. The initializeTarget method can be filled in more later.

I moved this, but its still a little weird and needs a FIXME. I'm not actually sure how important this ensureDocument call really is, but as we approach multiple DOMAgent targets it'll be more clear.

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:668
>>> +        WI.CSSCompletions.initializeCSSCompletions(WI.assumingMainTarget());
>> 
>> I realize that this was here already, but is it actually needed?
> 
> Ditto, isn't it redundant with the call in WI.performOneTimeInitializationWithTarget()?

This is the `activateExtraDomains` path. This is ITMLKit enabling the CSS domain dynamically. It is such a rare case, this is the safest approach that should work for them.

>> Source/WebInspectorUI/UserInterface/Controllers/ConsoleManager.js:171
>> +            }
> 
> Instead of including braces for when we strip `console.assert`, use `Array.prototype.every`:
> 
>     console.assert(channels.every((channel) => this._loggingChannelSources.includes(channel.source));

Great suggestion.

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:119
>>          if (DebuggerAgent.setPauseOnAssertions)
> 
> Should this check that `target.DebuggerAgent` exists?  Additionally, shouldn't it be `target.DebuggerAgent.setPauseOnAssertions`?

This is something I'm still debating. Ultimately I want every to have some kind of "<prefix>.FooAgent.method", and over time as we reduce the number of unprefixed agent calls we will solve all multi-target issues / open questions.

Currently all targets will have the same features per backend. I think that is unlikely to change. So this could be done in a number of ways:

    FooAgent.method
        - Bad. Implicitly uses the global agent, not good since lacking a prefix it looks like a mistake

    window.FooAgent.method
        - Bad. We have the concept of activating / disabling top level agents so this isn't perfect and would need `window.FooAgent &&`.

    target.FooAgent.method
        - Okay. But this also needs to check `target.FooAgent &&` since the agent might not exist on the target.
        - What if you want to enable/disable something based on another agent feature existing? Probably not a problem we need to care about.

    <name>.FooAgent.method
        - We could expose a top level name just for feature checking for example:
            "featureCheck.FooAgent.method", "protocol.FooAgent.method", "check.FooAgent.method", "test.FooAgent.method",
        - This would have all of the agents, all of the time, and should only be used for feature checking.
        - Only drawback would be if we supported connecting to different targets with different backend features, which seems unlikely right now.

I've been thinking `featureCheck.FooAgent.method`, but I'm still trying to think through implications of this. It'll be a step later on as we start to eliminate all unprefixed Agent accesses.

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:131
>> +            targetData.pauseIfNeeded();
> 
> This reads poorly, what is the type of targetData?

Yeah this sucks but it isn't changing. The DebuggerData class has these methods =(. DebuggerAgent though is one of the ones with complete multi-target support right now though!

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:556
>>      initializeTarget(target)
> 
> I think you meant to remove this function, as it's somewhat a duplicate of the one above.

Oops yes, again a fault of trying to extract this from a much larger diff.

>> Source/WebInspectorUI/UserInterface/Controllers/RuntimeManager.js:44
>> +        if (WI.showJavaScriptTypeInformationSetting && WI.showJavaScriptTypeInformationSetting.value && RuntimeAgent.enableTypeProfiler)
> 
> You could move `WI.showJavaScriptTypeInformationSetting` into `WI.settings` and avoid this first check.

Good idea, lets move all of these in one patch.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:72
>> +    initializeTargetsWithMainTarget()
> 
> This makes me think that it will loop over `WI.managers` and call `initializeTarget` with `WI.mainTarget` for each.  If that is not the case, it could use a better name 😅

Hah, true. I've named this `initializeMainTarget()`. This may go away soon so I don't care much about the name.

[Joseph Pecoraro]

I've also considered:

    all.FooAgent.method(...)

To become shorthand for:

    for (let target of WI.targets) {
        if (target.FooAgent)
            target.FooAgent.method(...);
    }

But that seems likely to be poorer performance than just writing the for loop ourselves in case we want to perform multiple agent operations in 1 for loop instead of multiple implicit for loops.

[Joseph Pecoraro]

Created attachment 353412 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Main.js:150
> +    this.printStylesEnabled = false;

It's weird that this is not a WI.Setting like other settings above whose value would need to be re-sent to each new target.

> Source/WebInspectorUI/UserInterface/Controllers/ConsoleManager.js:165
> +        target.ConsoleAgent.getLoggingChannels((error, channels) => {

I would have used a promise, but I don't care either way.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:46
> +        WI.settings.pauseForInternalScripts.addEventListener(WI.Setting.Event.Changed, this._pauseForInternalScriptsDidChange);

Why not pass `this`?

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:150
>> +    this.printStylesEnabled = false;
> 
> It's weird that this is not a WI.Setting like other settings above whose value would need to be re-sent to each new target.

We intentionally do not save this across inspector opening / closing. Too many people got confused when this state persisted, because the page would change appearance when Web Inspector opened and they didn't know how to disable it.

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:46
>> +        WI.settings.pauseForInternalScripts.addEventListener(WI.Setting.Event.Changed, this._pauseForInternalScriptsDidChange);
> 
> Why not pass `this`?

Oops, yes!

[Joseph Pecoraro]

<https://trac.webkit.org/r237652>