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

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted 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 354608: [PATCH] Proposed Fix

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




--- Comment #14 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 354608
  --> https://bugs.webkit.org/attachment.cgi?id=354608
[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