Bug 191494 – Web Inspector: Keep Web Inspector window alive across process swaps (PSON) (Remote Inspector)

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

[Joseph Pecoraro]

Keep Web Inspector window alive across process swaps (PSON) (Remote Inspector)

Web Inspector needs to:

  - be capable of detaching and reattaching to a page
  - connecting to a page through the UIProcess instead of the WebContentProcess

[Joseph Pecoraro]

<rdar://problem/45469854>

[Joseph Pecoraro]

Starting with Remote Web Inspector support, then will follow-up with Local Inspector. This is because the remote path is simpler.

[Joseph Pecoraro]

Created attachment 354420 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

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


ERROR: Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.h:65:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
ERROR: Source/WebKit/UIProcess/InspectorTargetProxy.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/WebPageInspectorTargetAgent.h:38:  "virtual" is redundant since function is already declared as "final"  [readability/inheritance] [4]
ERROR: Source/WebKit/UIProcess/WebPageInspectorTargetAgent.cpp:32:  "virtual" is redundant since function is already declared as "final"  [readability/inheritance] [4]
ERROR: Source/WebKit/UIProcess/WebPageInspectorTargetAgent.cpp:33:  "virtual" is redundant since function is already declared as "final"  [readability/inheritance] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorTarget.h:37:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebKit/UIProcess/WebPageInspectorController.cpp:160:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 8 in 71 files


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

[Joseph Pecoraro]

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

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

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:103
> +        this._addTarget(target);

It is debatable whether or not we should trigger a TargetAdded event for the MultiplexingBackendTarget. Since it doesn't appear in `WI.targets`.

[Joseph Pecoraro]

Created attachment 354444 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

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


ERROR: Source/WebKit/UIProcess/WebPageInspectorController.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 71 files


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

[Devin Rousso]

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

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

Overall, this looks good.  I am somewhat new at the WebInspector process architecture, so you might want to seek another opinion on those parts of the patch.  I'd like a few of my comments/concerns addressed if possible.

Generally speaking, I've been "screwed" in the past by trying to add support for things before they are needed (e.g. Canvas protocol changes not compiling once actually used somewhere).  For those changes, I'd rather there be a good comment explaining why they're added in this "implementation" patch, or moved into the a different "usage" patch.

> Source/JavaScriptCore/inspector/InspectorTarget.h:52
> +    virtual void connect(FrontendChannel*) = 0;
> +    virtual void disconnect(FrontendChannel*) = 0;

From what I can tell, it seems like these will never be `nullptr`.  Is that true?  If so, make them references.

> Source/JavaScriptCore/inspector/InspectorTarget.h:56
> +    virtual void resumeIfPaused() { }

This doesn't appear to be used.  Is it needed (e.g. a future patch)?

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:64
> +        errorString = "Target not found."_s;

NIT: our usual error string style is "No target for given identifier", but I also like the succinct-ness of this, so I'm fine with it as is

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:122
> +    m_frontendDispatcher->targetCreated(buildTargetInfoObject(target));

Should this only be dispatched when `m_isConnected`?

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:127
> +    m_frontendDispatcher->targetTerminated(target.identifier());

Ditto (122)

NIT: is there a reason this isn't the last call in this function?

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:139
> +    for (auto& entry : m_targets) {

NIT: you could use `m_targets.values()` to iterate over just the values instead

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:150
> +    for (auto& entry : m_targets) {

Ditto (139)

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.h:49
> +    virtual FrontendChannel* frontendChannel() = 0;

Make this a reference

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.h:52
> +    void exists(ErrorString&) final;

This could use an explanation somewhere as to why it's really needed.

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.h:58
> +    void targetTerminated(InspectorTarget&);

NIT: is there a reason to use "terminated"?  You could call this `targetDestroyed` to match the lifecycle of other agent objects.

> Source/JavaScriptCore/inspector/protocol/Target.json:3
> +    "availability": ["web"],

IIUC, this may eventually become available everywhere, but for now it's limited to actual pages?

> Source/WebCore/ChangeLog:26
> +        When a frontend connects just always enable the developer extras for the Page.

Typo: "connects, always"

> Source/WebCore/ChangeLog:27
> +        This is pretty much only for the remote path, which allowed inspection if developer

Typo" which allows inspection"

> Source/WebInspectorUI/ChangeLog:10
> +        Specifically a backend connection that is persistent, but the ability to connect

Typo: "but has the"

> Source/WebInspectorUI/ChangeLog:32
> +          - In a direct-target world, there is a single target which is
> +            should be used for global agents and in WI.targets.

Typo: "which should be"

> Source/WebInspectorUI/ChangeLog:38
> +        In the multi-target world, there are now commonly two Targets. The

Typo: "In a multi-target" (consistent with next paragraph)

> Source/WebInspectorUI/ChangeLog:44
> +        as soon as it opens (via the Target.targetCreated). In order to support

Typo: "via Target.targetCreated"

> Source/WebInspectorUI/ChangeLog:45
> +        this frontend initialization happens without a main target being available.

Typo: "this, frontend"

> Source/WebInspectorUI/ChangeLog:53
> +        WI.pageTarget. When a pageTranition happens the WI.pageTarget changes, and the

Typo: "a page transition happens"

> Source/WebInspectorUI/ChangeLog:56
> +        For the most part the UI behaves fine as long once there is a main frame

Typo: "fine once there are main"

> Source/WebInspectorUI/UserInterface/Base/Main.js:154
> +    WI.pageTarget = null;

While I do understand the "need" for something like this, I feel like our usage of `WI.pageTarget` is such that it doesn't need to be a different object from `WI.backendTarget`.  Furthermore, it seems like `WI.backendTarget` isn't actually directly needed, so could we move `WI.pageTarget` into `WI.mainTarget`.  Is it ever necessary that `WI.mainTarget` be the `WI.backendTarget` (e.g. some caller of `WI.mainTarget` expects that case to be true)?

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:43
> +            this._cachedTargetsList = Array.from(this._targets.values()).filter((target) => !(target instanceof WI.MultiplexingBackendTarget));

If it is necessary to keep `allTargets`, you could use that here ` = this.allTargets.filter(`

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:47
> +    get allTargets()

This doesn't appear to be used.  Is it needed (e.g. a future patch)?

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:65
> +    targetCreated(targetInfo)

NIT: add `// Called from WI.TargetObserver.`

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:69
> +            this.createMultiplexingBackendTarget(targetInfo);

Should you return after this call?  Otherwise, the `_addTarget` call below will override this entry in `_targets`.

It also seems weird to use the same event for the "initial" target, and any subsequent targets thereafter.  If we're always expecting the first `targetCreated` to be a multiplexing target, I'd rather us have a separate event `multiplexerCreated` for that target, and have the rest of the "normal" targets go through `targetCreated`.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:79
> +    targetTerminated(targetId)

Ditto (65)

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:86
> +    dispatchMessageFromTarget(targetId, message)

Ditto (65)

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:121
> +    _addTarget(target)

Considering that this function is used elsewhere (e.g. `WI.WorkerManager`), it should remain public

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:132
> +    _removeTarget(target)

Ditto (121)

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:190
> +        clearTimeout(this._transitionTimeoutIdentifier);

This should also be cleared each time a new `WI.pageTarget` is set.  Instead of having the early return inside the callback of the `setTimeout`, we should clear the `setTimeout` itself and prevent it from firing.  It firing at all should be the "error".

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:324
> +        for (let domain in InspectorBackend._agents) {

NIT: use `Object.entries`

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:36
> +        const type = WI.Target.Type.Page;
> +        const displayName = WI.UIString("Page");

NIT: I've been starting to inline these types of constants if the value has something related to the parameter name (e.g. `WI.Target.Type.Page` has "type" already in it)

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:37
> +

Style: remove newline

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:40
> +        // Restrict to just TargetAgent.

NIT: this comment is unnecessary IMO since the code right below it is pretty self-explanatory

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:41
> +        const supportedMultiplexingDomains = ["Target"];

Are you planning on adding to this array in the future?  If not, I'd remove all the loop code and just write it once.

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:60
> +        console.error("Called name on a MultiplexingBackendTarget");

Should this `throw new WI.NotImplementedError` instead?

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:66
> +        console.error("Called executionContext on a MultiplexingBackendTarget");

Ditto (60)

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:72
> +        console.error("Called mainResource on a MultiplexingBackendTarget");

Ditto (60)

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

NIT: use `this.displayName`

> Source/WebInspectorUI/UserInterface/Protocol/PageTarget.js:39
> +        return WI.displayNameForURL(this._name);

`this._name` is constructed with `WI.UIString("Page")` on TargetManager.js:151, so it won't be a URL

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:215
> +            this._targetAdded({data: {target}});

Rather than do this "hack", I'd prefer you create a `_addTarget` function that is called by `_targetAdded` (you could also rename `_targetAdded` to `_handleTargetAdded`)

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:221
> +        for (let script of WI.debuggerManager.knownNonResourceScripts)

Considering how large this diff already is, I'd remove this and "fix" it later

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:743
> +            this._mainTargetTreeElement = treeElement;

When the `WI.mainTarget` changes, we should also remove the old `WI.TreeElement` (you might even want to replace the old one with the new one to ensure the right ordering)

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:116
> +        if (!WI.mainTarget || !WI.mainTarget.executionContext)

If the `WI.mainTarget` doesn't have an `executionContext`, shouldn't we clear the `_mainExecutionContextPathComponent`?

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:119
> +        this._mainExecutionContextPathComponent = this._createExecutionContextPathComponent(WI.mainTarget.executionContext);

Should we do anything to the old `_mainExecutionContextPathComponet` if it exists?  If the `WI.PathComponent` is displayed anywhere in the UI, we should also update that (e.g. the execution context picker).

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:321
> +        if (target.type !== WI.Target.Type.Worker)

What about `ServiceWorker`?

> Source/WebInspectorUI/UserInterface/Views/QuickConsole.js:335
> +        if (target.type !== WI.Target.Type.Worker)

Ditto (321)

> Source/WebKit/ChangeLog:20
> +        stay alive and reconnect to a page when the inspected page crashes!

\(0.0)/

> Source/WebKit/UIProcess/WebPageDebuggable.cpp:71
> +    WebPageInspectorController& inspectorController = m_page.inspectorController();

NIT: is it necessary to save this to a local variable?

> Source/WebKit/UIProcess/WebPageDebuggable.cpp:77
> +    WebPageInspectorController& inspectorController = m_page.inspectorController();

Ditto (71)

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:65
> +bool WebPageInspectorController::hasRemoteFrontend() const

You can use this object's `hasRemoteFrontend()` instead of re-calling `m_frontendRouter-> hasRemoteFrontend()` in the functions below

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:70
> +void WebPageInspectorController::connectFrontend(Inspector::FrontendChannel* frontendChannel, bool, bool)

It seems like this should be reworked so as to pass a reference, not a pointer

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:74
> +    bool connectedFirstFrontend = !m_frontendRouter->hasFrontends();

NIT: this should be present/future tense, as you haven't connected yet: `connectingFirstFrontend` or even just `hasFrontend` (negating it in the `if` below)

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:89
> +void WebPageInspectorController::disconnectFrontend(FrontendChannel* frontendChannel)

Ditto (70)

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:100
> +    if (!m_frontendRouter->hasFrontends())

Should this be `hasRemoteFrontend`?

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:155
> +    m_targets.append(makeRefPtr(target.get()));

Correct me if I'm wrong, but wouldn't `m_targets.append(target.copyRef())` also work?

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:162
> +    auto position = m_targets.findMatching([&](const auto& item) { return item->identifier() == targetId; });

Use a `HashMap<String, RefPtr<InspectorTargetProxy>>` to avoid this iterative lookup

> Source/WebKit/UIProcess/WebPageInspectorController.h:74
> +    Inspector::InspectorTargetAgent* m_targetAgent;

This can be a value/reference, since it's always guaranteed to exist

> Source/WebKit/UIProcess/WebPageInspectorTargetAgent.cpp:32
> +    ConnectionType connectionType() const final { return Inspector::FrontendChannel::ConnectionType::Remote; }

Should this always be `Remote`, or is this only for this patch (since you're only adding remote support)?

> Source/WebKit/UIProcess/WebPageProxy.h:396
> +    void showInspectorIndication();
> +    void hideInspectorIndication();

Is there a reason to move these outside `PLATFORM(IOS_FAMILY)`?

[Joseph Pecoraro]

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

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

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:122
>> +    m_frontendDispatcher->targetCreated(buildTargetInfoObject(target));
> 
> Should this only be dispatched when `m_isConnected`?

I suppose yes none of will do anything if we are not connected, I'll bail above.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:127
>> +    m_frontendDispatcher->targetTerminated(target.identifier());
> 
> Ditto (122)
> 
> NIT: is there a reason this isn't the last call in this function?

Sure I'll make these changes.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.h:52
>> +    void exists(ErrorString&) final;
> 
> This could use an explanation somewhere as to why it's really needed.

The explanation was in the imp.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.h:58
>> +    void targetTerminated(InspectorTarget&);
> 
> NIT: is there a reason to use "terminated"?  You could call this `targetDestroyed` to match the lifecycle of other agent objects.

Done. It was "Terminated" because this came from the Worker pattern, and Workers can be terminated.

>> Source/JavaScriptCore/inspector/protocol/Target.json:3
>> +    "availability": ["web"],
> 
> IIUC, this may eventually become available everywhere, but for now it's limited to actual pages?

I don't plan to do this everywhere... In fact it doesn't need to live in JavaScriptCore, but in case we do add a new such target in the future I put it here.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:154
>> +    WI.pageTarget = null;
> 
> While I do understand the "need" for something like this, I feel like our usage of `WI.pageTarget` is such that it doesn't need to be a different object from `WI.backendTarget`.  Furthermore, it seems like `WI.backendTarget` isn't actually directly needed, so could we move `WI.pageTarget` into `WI.mainTarget`.  Is it ever necessary that `WI.mainTarget` be the `WI.backendTarget` (e.g. some caller of `WI.mainTarget` expects that case to be true)?

WI.mainTarget should be the WI.backendTarget whenever there is a direct connection (JSContext inspection, Service Worker Inspection).

In cases where the frontend is using `WI.mainTarget.FooAgent.method` it should work regardless of being connected to a Page / JSContext / ServiceWorker.
In cases where the frontend is using `WI.pageTarget.FooAgent.method` it expected that we are connected to a Page and not a JSContext / ServiceWorker.

Assuming we can get rid of all unprefixed agent uses, then we can likely get rid of the pageTarget concept.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:47
>> +    get allTargets()
> 
> This doesn't appear to be used.  Is it needed (e.g. a future patch)?

I left this in only for debugging purposes.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:69
>> +            this.createMultiplexingBackendTarget(targetInfo);
> 
> Should you return after this call?  Otherwise, the `_addTarget` call below will override this entry in `_targets`.
> 
> It also seems weird to use the same event for the "initial" target, and any subsequent targets thereafter.  If we're always expecting the first `targetCreated` to be a multiplexing target, I'd rather us have a separate event `multiplexerCreated` for that target, and have the rest of the "normal" targets go through `targetCreated`.

Currently the frontend waits to create the multiplexing backend target when we get told about the first target. This should be eliminated when both the local inspector and remote inspector support the multiplexing path. At that point:

    - We can remote Target.exists
    - Main.js can just check the existence of window.TargetAgent to decide whether to create a MultiplexingBackendTarget or DirectBackendTarget

So this is a temporary measure for now. I'll add a FIXME like the one by Target.exists.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:190
>> +        clearTimeout(this._transitionTimeoutIdentifier);
> 
> This should also be cleared each time a new `WI.pageTarget` is set.  Instead of having the early return inside the callback of the `setTimeout`, we should clear the `setTimeout` itself and prevent it from firing.  It firing at all should be the "error".

I think ultimately we should remove this code in a month or so if it never triggers. I just left this hack to help detect any cases where a transition didn't happen properly. In practice I never once hit this.

>> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:41
>> +        const supportedMultiplexingDomains = ["Target"];
> 
> Are you planning on adding to this array in the future?  If not, I'd remove all the loop code and just write it once.

We may ultimately move Network and Storage up here, but I agree it is unnecessary now. I reduced this to:

        let agents = this._agents;
        this._agents = {
            Target: agents.Target,
        };

>> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:60
>> +        console.error("Called name on a MultiplexingBackendTarget");
> 
> Should this `throw new WI.NotImplementedError` instead?

Hmm, no. NotImplemented expects a subclass to implement. This won't be subclasses. We don't want these to be called at all. If they are we should figure out who called these on the backend target and eliminate the use cases. I haven't seen these called at all, but I wanted to catch any cases that did.

>> Source/WebInspectorUI/UserInterface/Protocol/PageTarget.js:32
>> +        this._executionContext = new WI.ExecutionContext(this, WI.RuntimeManager.TopLevelContextExecutionIdentifier, this.name, true, null);
> 
> NIT: use `this.displayName`

Unlike other targets (which are likely sub-targets) we want the ExecutionContext to be the name "Page" and not the url. At least that was old behavior and makes sense in the execution context picker. Maybe we can improve this to be "Page – <domain>".

>> Source/WebInspectorUI/UserInterface/Protocol/PageTarget.js:39
>> +        return WI.displayNameForURL(this._name);
> 
> `this._name` is constructed with `WI.UIString("Page")` on TargetManager.js:151, so it won't be a URL

You're right, I'll just drop displayName in this class.

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:743
>> +            this._mainTargetTreeElement = treeElement;
> 
> When the `WI.mainTarget` changes, we should also remove the old `WI.TreeElement` (you might even want to replace the old one with the new one to ensure the right ordering)

Good point, this might need some work, but I think this works as is. When the new target is the new mainTarget (page transition) it'll go through here and update the mainTargetTreeElement.

>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:65
>> +bool WebPageInspectorController::hasRemoteFrontend() const
> 
> You can use this object's `hasRemoteFrontend()` instead of re-calling `m_frontendRouter-> hasRemoteFrontend()` in the functions below

Removed hasRemoteFrontend.

>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:70
>> +void WebPageInspectorController::connectFrontend(Inspector::FrontendChannel* frontendChannel, bool, bool)
> 
> It seems like this should be reworked so as to pass a reference, not a pointer

Yep, lets do that in a follow-up. Since that exists in many places.

>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:100
>> +    if (!m_frontendRouter->hasFrontends())
> 
> Should this be `hasRemoteFrontend`?

Changed this to just be `if (hasLocalFrontend())`. The Remote Inspector listing state has a bit for "has local debugger" or not. If that is changing here, we update the state.

>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:155
>> +    m_targets.append(makeRefPtr(target.get()));
> 
> Correct me if I'm wrong, but wouldn't `m_targets.append(target.copyRef())` also work?

Works!

>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:162
>> +    auto position = m_targets.findMatching([&](const auto& item) { return item->identifier() == targetId; });
> 
> Use a `HashMap<String, RefPtr<InspectorTargetProxy>>` to avoid this iterative lookup

The expectation is that the list of targets will be very few. In fact right now it is just one. If that expectation changes we should convert this to a map.

>> Source/WebKit/UIProcess/WebPageInspectorTargetAgent.cpp:32
>> +    ConnectionType connectionType() const final { return Inspector::FrontendChannel::ConnectionType::Remote; }
> 
> Should this always be `Remote`, or is this only for this patch (since you're only adding remote support)?

Correct. When adding a local inspector this stub will need to know which type is connected from the UIProcess side.

>> Source/WebKit/UIProcess/WebPageProxy.h:396
>> +    void hideInspectorIndication();
> 
> Is there a reason to move these outside `PLATFORM(IOS_FAMILY)`?

Oops, good catch, I'll add that back. I was moving this to be public.

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 354420 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354420&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:103
> > +        this._addTarget(target);
> 
> It is debatable whether or not we should trigger a TargetAdded event for the
> MultiplexingBackendTarget. Since it doesn't appear in `WI.targets`.

I changed this to not do this for Multi Backend. This simplifies the code elsewhere that was ignoring instanceof WI.MultiplexingBackendTarget. Since this is not in `WI.targets` I think it shouldn't receive an event.

[Joseph Pecoraro]

Created attachment 354608 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

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


ERROR: Source/WebKit/UIProcess/WebPageInspectorController.cpp:157:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 71 files


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

[Joseph Pecoraro]

Created attachment 354690 [details]
[PATCH] Follow-up for Local Inspector

This is an expected follow-up to disable PSON for the local inspector until we add support for that.

[Devin Rousso]

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

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

r=me, do we need to retry the win bot?  Also, is there any way for you to write some tests for this?

> Source/JavaScriptCore/ChangeLog:16
> +        New protocol domain, modelled after Worker.json, to allow for

Typo: "domain, modeled after"

> Source/JavaScriptCore/ChangeLog:22
> +        provide an identifier, type, and means of connecting/disconnecting
> +        a frontend channel.

Grammar: "disconnecting to a"

> Source/JavaScriptCore/ChangeLog:39
> +        Implementation of TargetAgent holds a list of targets, and

You could just drop the "Implementation of TargetAgent", as the rest of the sentence reads just fine without it (clearer actually).

> Source/JavaScriptCore/inspector/protocol/Target.json:10
> +                { "name": "targetId", "type": "string", "description": "Unique identifier for the target." },

NIT: should this be a separate type, like `Network.RequestId`?  That way we could use it in other places as well (e.g. `Network.event.requestWillBeSent`).

> Source/WebCore/ChangeLog:28
> +        extras were not already enabled (iOS). This simplifies the logic, and toggling

NIT: the "(iOS)" should be moved to be right before the comma

> Source/WebCore/inspector/InspectorController.cpp:-354
> -    ASSERT(!m_frontendRouter->hasRemoteFrontend());

If you are changing this, theres another `m_frontendRouter->hasRemoteFrontend()` call in `InspectorController::connectFrontend` :)

> Source/WebInspectorUI/ChangeLog:116
> +        Workers are still special target like things that multiplex through

NIT: "special target-like things"

> Source/WebInspectorUI/ChangeLog:118
> +        be full targets in the future.

link/FIXME?

> Source/WebInspectorUI/ChangeLog:140
> +        therefore execution context is available. Just return null, when

NIT: "context, is"

> Source/WebInspectorUI/UserInterface/Models/DefaultDashboard.js:53
> +        WI.notifications.addEventListener(WI.Notification.TransitionPageTarget, this._transitionPageTarget, this);

NIT: I try to have every event handler begin with `_handle*` so it's easier to search for event related code

> Source/WebInspectorUI/UserInterface/Protocol/DirectBackendTarget.js:65
> +                type: WI.Target.Type.Page,
> +                displayName: WI.UIString("Main"),

Should we assume `WI.Target.Type.JSContext`, similar to `Inspector::targetTypeToProtocolType`?

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:59
> +        return WI.UIString("Page");

Ditto (DirectBackendTarget.js:64)

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

NIT: add `const isPageContext = true;` and drop the last argument

[Devin Rousso]

Comment on attachment 354690 [details]
[PATCH] Follow-up for Local Inspector

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

r=me

> Source/WebKit/ChangeLog:3
> +        Web Inspector: Keep Web Inspector window alive across process swaps (PSON) (Remote Inspector)

Do you want to make a new bug for this, or just have it as part of the same one?

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:105
> +    m_page->setHasLocalInspectorFrontend(true);

NIT: I'd move this before the message send, as you'd want to set the state before notifying listeners that the state has changed

[Joseph Pecoraro]

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

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

>> Source/JavaScriptCore/inspector/protocol/Target.json:10
>> +                { "name": "targetId", "type": "string", "description": "Unique identifier for the target." },
> 
> NIT: should this be a separate type, like `Network.RequestId`?  That way we could use it in other places as well (e.g. `Network.event.requestWillBeSent`).

Good idea, I'll do that in a follow-up.

>> Source/WebCore/inspector/InspectorController.cpp:-354
>> -    ASSERT(!m_frontendRouter->hasRemoteFrontend());
> 
> If you are changing this, theres another `m_frontendRouter->hasRemoteFrontend()` call in `InspectorController::connectFrontend` :)

I changed that one above as well.

>> Source/WebInspectorUI/UserInterface/Protocol/DirectBackendTarget.js:65
>> +                displayName: WI.UIString("Main"),
> 
> Should we assume `WI.Target.Type.JSContext`, similar to `Inspector::targetTypeToProtocolType`?

Hmm, okay I'll do that. And then I can get rid of the localized string "Main".

[Joseph Pecoraro]

https://trac.webkit.org/changeset/238192/webkit
https://trac.webkit.org/changeset/238193/webkit

[Joseph Pecoraro]

https://trac.webkit.org/r238195 - wincairo build fix