[webkit-reviews] review canceled: [Bug 191494] Web Inspector: Keep Web Inspector window alive across process swaps (PSON) (Remote Inspector) : [Attachment 354444] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 191494: Web Inspector: Keep Web Inspector window alive across process swaps
(PSON) (Remote Inspector)
https://bugs.webkit.org/show_bug.cgi?id=191494

Attachment 354444: [PATCH] Proposed Fix

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




--- Comment #9 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 354444
  --> https://bugs.webkit.org/attachment.cgi?id=354444
[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.