[webkit-reviews] review denied: [Bug 204170] Web Inspector: allow inspector to pause provisional page load and restore its state : [Attachment 384109] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 204170: Web Inspector: allow inspector to pause provisional page load and
restore its state
https://bugs.webkit.org/show_bug.cgi?id=204170

Attachment 384109: Patch

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




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 384109
  --> https://bugs.webkit.org/attachment.cgi?id=384109
Patch

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

r-, as it looks like
'http/tests/inspector/target/provisional-load-cancels-previous-load.html' is
failing :(

Thanks for iterating on this :)  I think we're almost there!

> Source/JavaScriptCore/inspector/InspectorTarget.h:66
> +    bool m_pausedOnStart { false };

Please change this to be just `m_paused` to match the getter/setter naming.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:96
> +	   // COMPATIBILITY (iOS 13): Target.setPauseOnStart did not exist yet.
> +	   if (target.hasCommand("Target.setPauseOnStart"))
> +	       target.TargetAgent.setPauseOnStart(true);

Let's move this to `WI.MultiplexingBackendTarget.prototype.initialize` (please
keep the existing comments).

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:177
> +	       return new WI.PageTarget(parentTarget, targetId,
WI.UIString("Page"), connection, targetInfo.isProvisional, isPaused);

Why not also pull out `isProvisional` above?
```
    // COMPATIBILITY (iOS 13.0): `Target.TargetInfo.isProvisional` and
`Target.TargetInfo.isPaused` did not exist yet.
    let {targetId, type, isProvisional, isPaused} = targetInfo;
```

> Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:50
> +	   const isPaused = false;
> +	   let workerTarget = new WI.WorkerTarget(target, workerId, url,
connection, isPaused);

Style: we normally omit trailing arguments if they're falsy.

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:58
> +    addProvisionalMessage(message)

>> Rather than introduce an entire new function, couldn't we store the paused
state on `WI.Target` and have `InspectorBackend.Connection.prototype.dispatch`
have an early return if `this._target.isPaused`?  That way, all the logic can
be contained inside `WI.Target` and `WI.Connection` and doesn't need to be
"exposed" in other classes/files.
> I don't quite like it as today Target it not in the business of dispatching
messages. Buffering the messages on Target would anyway require adding similar
new methods on the Target class. Additionally we'd have to plumb all message
dispatching logic through Target (current implementation does it by calling
target.connection.dispatch). Having dispatch() and
dispatchinProvisionalMessages() seems more logical to me as it doesn't leak
notion of messages to Target.
I wasn't suggesting that we store provisional messages on `WI.Target`, but
rather to have `InspectorBackend.Connection.prototype.dispatch` check whether
`this._target.isProvisional` and do this logic there instead.  That way,
`WI.TargetManager` doesn't have to know about whether the `WI.Target` is
provisional or not, and instead can just let `WI.Target` and
`InspectorBackend.Connection` deal with it instead.

```
    dispatch(message)
    {
	if (this._target.isProvisional) {
	    this._provisionalMessages.push(message);
	    return;
	}
	...
    }
```

Then, inside `WI.Target.prototype.didCommitProvisionalTarget`, we could call
`InspectorBackend.Connection.prototype.dispatchProvisionalMessages`.

> Source/WebInspectorUI/UserInterface/Protocol/DirectBackendTarget.js:37
> +	   const isPaused = false;
> +	   super(parentTarget, targetId, displayName, type,
InspectorBackend.backendConnection, isPaused);

Ditto (WorkerManager.js:49)

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:38
> +	   const isPaused = false;
> +	   super(parentTarget, targetId, WI.UIString("Web Page"),
WI.TargetType.WebPage, InspectorBackend.backendConnection, isPaused);

Ditto (WorkerManager.js:49)

> Source/WebInspectorUI/UserInterface/Protocol/PageTarget.js:28
> +    constructor(parentTarget, targetId, name, connection, isProvisional,
isPaused)

Ditto (Target.js:28)

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:28
> +    constructor(parentTarget, identifier, name, type, connection, isPaused)

We could make `isPaused` into an optional object argument so that this
constructor can better support additional arguments.
```
    constructor(parentTarget, workerId, name, connection, {isProvisional,
isPaused} = {})
```

Also, since a `WI.Target` can't become provisional after construction, we
shouldn't have a `set isProvisional`.  I'd much prefer including it as part of
the optional object so it can only be set by the constructor and then have a
`didCommitProvisionalTarget` member function that sets `this._isProvisional =
false;`.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:43
> +	   this._isProvisional = false;
> +	   this._isPaused = isPaused;

Since these come from `constructor` parameters, please move them to be after
the previous parameter argument `connection` so they're nearby the other
constructor values.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:103
> +	       console.assert(this._parentTarget.hasCommand("Target.resume"));

Nice!  While we're at it, we should also `console.assert(!isPaused ||
parentTarget.hasCommand("setPauseOnStart"));` in the `constructor`.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:105
> +	       .then(() => {

We typically prefer using a callback instead of a `Promise` if we don't
actually need to use a `Promise`, that way we can avoid the microtask delay. 
It also makes the code cleaner in that all the handling logic is inside one
function instead of two.
```
    this._parentTarget.TargetAgent.resume(this._identifier, (error) => {
	if (error) {
	    // Ignore errors if the target was destroyed after the command was
sent.
	    if (!this._isDestroyed)
		WI.reportInternalError(error);
	    return;
	}

	this._isPaused = false;
    });
```

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:124
> +    get isPaused() { return this._isPaused; }

Style: I'd put this right above `get isDestroyed()`.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:191
> +    set isProvisional(value) { this._isProvisional = value; }

It's not possible (or at the very least it shouldn't be possible) for an
existing `WI.Target` to _become_ provisional (e.g. `value === true`), so I'd
rather have this be more specific.

```
    didCommitProvisionalTarget()
    {
	this._isProvisional = false;

	this._connection.dispatchProvisionalMessages();
    }
```

> Source/WebInspectorUI/UserInterface/Protocol/WorkerTarget.js:28
> +    constructor(parentTarget, workerId, name, connection, isPaused)

Ditto (Target.js:28)

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:708
> -		   return true;
> +		   return !!typeIdentifier;

Why was this changed?

> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:81
> +    if (isPaused())
> +	   setPaused(false);

Shouldn't this be before the early-return above?

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:126
> +    m_loadingPaused = false;

We should `ASSERT(m_loadingPausedForInspector);` at the beginning of this
function.

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:93
> +    void continueLoading();

This should have the word "ForInspector" in it's name so it's clear that it
assist with Web Inspector functionality, not something special for provisional
pages.
```
    void continueLoadingAfterPausingForInspector();
```

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:146
> +    WTF::Function<void()> m_loadingContinuation;
> +    bool m_loadingPaused { false };

Both of these should have "ForInspector" in the name so it's clear that this
callback is used by Web Inspector to support debugging, and not some WebKit
critical functionality.

```
    #include <wtf/Function.h>
    ...
    URL m_provisionalLoadURL;

    Function<void()> m_continueLoadingWhenResumedByInspectorFunction;
    bool m_loadingPausedForInspector { false };
```

> Source/WebKit/UIProcess/WebPageInspectorController.h:70
> +    PauseLoading interceptProvisionalPage(ProvisionalPageProxy&);

Rather than "intercept", how about "shouldPause" to match the naming in other
files?	We then could return a `bool`.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3099
> +    auto continuation = [this, navigation = makeRef(navigation), newProcess
= WTFMove(newProcess), websitePolicies = WTFMove(websitePolicies)]() mutable {

I think we should also capture `protectedThis = makeRef(*this)` instead of
`this`.