Bug 204170 – Web Inspector: allow inspector to pause provisional page load and restore its state

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

[Yury Semikhatsky]

Target agent should allow to automatically pause new targets on start to let inspector front-end restore its state (breakpoints, user agent overrides, network recording etc) before main resource's loading event starts.

[Yury Semikhatsky]

Created attachment 383481 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Yury Semikhatsky]

Created attachment 383491 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 383491 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:214
> +        if (this._hasContent())

Note to reviewers: this patch also includes changes from https://bugs.webkit.org/show_bug.cgi?id=204086, I'm going to drop those changes once that bug is fixed.

[Yury Semikhatsky]

Chris, please have a look at the modifications in WebPageProxy and ProvisionalPageProxy. I spoke with you briefly about the changes at the WebKit conference.

[Devin Rousso]

Comment on attachment 383491 [details]
Patch

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

> Source/JavaScriptCore/inspector/InspectorTarget.h:54
> +        if (!paused)

NIT: you can use `m_pausedOnStart` instead, since that's the actual state of this object

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

Shouldn't this be `protected`?

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:65
> +    InspectorTarget* target = m_targets.get(targetId);

`auto`?

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:69
> +    }

Style: missing newline after

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:71
> +        error = "Target is not paused on start"_s;

NIT: I'd rephrase this as "Target for given targetId is not paused"

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:73
> +    }

Style: missing newline after

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:74
> +    target->setPausedOnStart(false);

Having this be called `setPausedOnStart` seems weird.  How about we just call it `isPaused` and `setPaused`.

> Source/JavaScriptCore/inspector/protocol/Target.json:20
> +            "name": "setPauseOnStart",

Why do we need this command, especially since we always pass `true`?

> Source/JavaScriptCore/inspector/protocol/Target.json:28
> +            "description": "Will resume target it was paused on start.",

s/it/if

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:-38
> -        this._provisionalTargetInfos = new Map;
> -        this._swappedTargetIds = new Set;

Why were these removed?  Were they replaced by some other logic, or are they no longer needed?

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:94
> +        target.TargetAgent.setPauseOnStart(true);

This needs a compatibility statement (assuming we keep it).
```
    // COMPATIBILITY (iOS 13): Target.setPauseOnStart did not exist yet.
    if (target.hasCommand("Target.setPauseOnStart"))
        target.TargetAgent.setPauseOnStart(true);
```

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:123
> +        this.dispatchEventToListeners(WI.TargetManager.Event.TargetCreated, {target, isPausedOnStart: targetInfo.isPausedOnStart});

Can we remove this event altogether?  We could add an `_isPaused` member to `WI.Target` and use `WI.TargetManager.Event.TargetAdded` instead.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:133
> +            return;
> +        target.isProvisional = false;

Style: missing newline

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:171
> +            parentTarget.TargetAgent.resume(targetInfo.targetId).catch((error) => {

Could we have this be inside `WI.Target.prototype.initialize`?  Do we want to put this inside the `Promise.all` that's in that function?

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:172
> +                if (subTarget.isDestroyed)

Please add a comment explaining why we have this early return.

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:187
> +            return;
>          this._checkAndHandlePageTargetTermination(target);

Style: missing newline

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:213
> +        // COMPATIBILITY (iOS 13.0): `Target.TargetInfo.isProvisional` did not exist yet.

This type of compatibility statement isn't necessary, since you're using logic defined in the frontend (e.g. `WI.Target.prototype.get isProvisional`).  We only use compatibility statements for things that directly communicate with the backend (e.g. command invocation or event handling).

> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:232
>              return;
> +        if (target.isProvisional)

Style: missing newline

> 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.

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

Why not pass this in as part of the constructor?

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

Style: we normally put a get-set pair in it's own section.  Take a look at `WI.Recording.prototype.get source` and `WI.Recording.prototype.set source` for an example of what I mean.

>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:214
>> +        if (this._hasContent())
> 
> Note to reviewers: this patch also includes changes from https://bugs.webkit.org/show_bug.cgi?id=204086, I'm going to drop those changes once that bug is fixed.

I don't see how that change is related to this bug.  Could you explain?

Also, for future reference, it makes it more difficult to review as I don't know what's necessary for this patch vs what isn't.  I'd appreciate it if you removed those changes (unless it is required) for the next patch you upload.

> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:110
> +    if (!m_provisionalPage)
> +        return;
> +    m_provisionalPage->resumeLoading();

NIT: I'd just have this be a regular `if`, not an early return.
```
    if (m_provisionalPage)
        m_provisionalPage->resumeLoading();
```

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:95
> +    void resumeLoading();
> +    bool shouldPauseLoading() const { return m_loadingPaused; }
> +    void runWhenLoadingResumed(WTF::Function<void(bool)>&&);

Please rename these to have "ForInspector" (or something like it) at the end of the name so it's clear when this is expected to be used.

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:170
> +WebPageInspectorController::PauseLoading WebPageInspectorController::didCreateProvisionalPage(ProvisionalPageProxy& provisionalPage)

Having this function return something is very weird, and not at all what I'd expect from reading the name.  Please either create another function `shouldPauseProvisionalPage` (which would be called immediately after `didCreateProvisionalPage`) or rename `didCreateProvisionalPage` to indicate that it returns something which should be used to change some functionality.

> Source/WebKit/UIProcess/WebPageInspectorController.cpp:173
> +    InspectorTarget* targetPtr = target.get();

`auto`?

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

What is the point of `isCancelled`?  It seems like we always pass `false` when calling this function.

Also, why is this `mutable`?

> LayoutTests/http/tests/inspector/target/provisional-load-cancels-previous-load.html:69
> +provisional targets being created, first of which is later destroyed and the second
> +is committed.

Style: please keep the `<p>` on one line.

> LayoutTests/http/tests/inspector/target/resources/inline-debugger-statement.html:18
> +</p>

Ditto

[Yury Semikhatsky]

Comment on attachment 383491 [details]
Patch

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

>> Source/JavaScriptCore/inspector/InspectorTarget.h:54
>> +        if (!paused)
> 
> NIT: you can use `m_pausedOnStart` instead, since that's the actual state of this object

Done.

>> Source/JavaScriptCore/inspector/InspectorTarget.h:64
>> +    virtual void resumeTarget() { }
> 
> Shouldn't this be `protected`?

No, I don't want to let any subclasses call this method, only override.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:65
>> +    InspectorTarget* target = m_targets.get(targetId);
> 
> `auto`?

Done.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:69
>> +    }
> 
> Style: missing newline after

Done.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:71
>> +        error = "Target is not paused on start"_s;
> 
> NIT: I'd rephrase this as "Target for given targetId is not paused"

Done.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:73
>> +    }
> 
> Style: missing newline after

Done.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:74
>> +    target->setPausedOnStart(false);
> 
> Having this be called `setPausedOnStart` seems weird.  How about we just call it `isPaused` and `setPaused`.

Done.

>> Source/JavaScriptCore/inspector/protocol/Target.json:20
>> +            "name": "setPauseOnStart",
> 
> Why do we need this command, especially since we always pass `true`?

This command get the backend to the state where normal control flow is interrupted and requires an explicit reaction from the frontend before it can continue. It's up to the front-end to decide if it needs to pause the targets at all. E.g. consider a performance profiling mode where you want to minimize impact of the inspector on the browser and may not want to introduce extra delays to the navigation.

>> Source/JavaScriptCore/inspector/protocol/Target.json:28
>> +            "description": "Will resume target it was paused on start.",
> 
> s/it/if

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:-38
>> -        this._swappedTargetIds = new Set;
> 
> Why were these removed?  Were they replaced by some other logic, or are they no longer needed?

We now create PageTarget for provisional targets as well and isProvisional bit is stored inside the target. Similarly _swappedTargetIds is not needed anymore, it was a temporary measure.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:94
>> +        target.TargetAgent.setPauseOnStart(true);
> 
> This needs a compatibility statement (assuming we keep it).
> ```
>     // COMPATIBILITY (iOS 13): Target.setPauseOnStart did not exist yet.
>     if (target.hasCommand("Target.setPauseOnStart"))
>         target.TargetAgent.setPauseOnStart(true);
> ```

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:123
>> +        this.dispatchEventToListeners(WI.TargetManager.Event.TargetCreated, {target, isPausedOnStart: targetInfo.isPausedOnStart});
> 
> Can we remove this event altogether?  We could add an `_isPaused` member to `WI.Target` and use `WI.TargetManager.Event.TargetAdded` instead.

Done. Removed both TargetCreated and TargetDestroyed events as now that we eagerly create Target for provisional pages both of them are redundant.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:133
>> +        target.isProvisional = false;
> 
> Style: missing newline

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:171
>> +            parentTarget.TargetAgent.resume(targetInfo.targetId).catch((error) => {
> 
> Could we have this be inside `WI.Target.prototype.initialize`?  Do we want to put this inside the `Promise.all` that's in that function?

Moved to Target.initialize. As long as commands from Promise.all are _sent_ before Target.resume I don't see a reason why Target.resume should be in the same Promise.all and be a blocker for Inspector.initialized (note that the target will be dispatching inspector messages even if it's paused).

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:172
>> +                if (subTarget.isDestroyed)
> 
> Please add a comment explaining why we have this early return.

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:187
>>          this._checkAndHandlePageTargetTermination(target);
> 
> Style: missing newline

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:213
>> +        // COMPATIBILITY (iOS 13.0): `Target.TargetInfo.isProvisional` did not exist yet.
> 
> This type of compatibility statement isn't necessary, since you're using logic defined in the frontend (e.g. `WI.Target.prototype.get isProvisional`).  We only use compatibility statements for things that directly communicate with the backend (e.g. command invocation or event handling).

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:232
>> +        if (target.isProvisional)
> 
> Style: missing newline

Done.

>> 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.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:42
>> +        this._isProvisional = false;
> 
> Why not pass this in as part of the constructor?

It has a setter and the only ancestor that may set it to true is PageTarget, so I simply call the setter from PageTarget constructor instead of adding one more argument to the already long list. Let me know if you prefer updating all subclasses to pass isProvisional value explicitly.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:176
>> +    set isProvisional(value) { this._isProvisional = value; }
> 
> Style: we normally put a get-set pair in it's own section.  Take a look at `WI.Recording.prototype.get source` and `WI.Recording.prototype.set source` for an example of what I mean.

Done.

>>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:214
>>> +        if (this._hasContent())
>> 
>> Note to reviewers: this patch also includes changes from https://bugs.webkit.org/show_bug.cgi?id=204086, I'm going to drop those changes once that bug is fixed.
> 
> I don't see how that change is related to this bug.  Could you explain?
> 
> Also, for future reference, it makes it more difficult to review as I don't know what's necessary for this patch vs what isn't.  I'd appreciate it if you removed those changes (unless it is required) for the next patch you upload.

Without these changes debugger is broken for inline breakpoints because of the bug mentioned above and it'd be difficult to test this patch if you decided to apply it locally. The fix is landed so this modifications are going to be dropped. How do you suggest to make the patch testable locally without squashing other bug fix changes into it? One way would be to upload the bug fix separately and provide instructions what are the prerequisite patches which is less convenient for reviewers IMO.

>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:110
>> +    m_provisionalPage->resumeLoading();
> 
> NIT: I'd just have this be a regular `if`, not an early return.
> ```
>     if (m_provisionalPage)
>         m_provisionalPage->resumeLoading();
> ```

Done.

>> Source/WebKit/UIProcess/ProvisionalPageProxy.h:95
>> +    void runWhenLoadingResumed(WTF::Function<void(bool)>&&);
> 
> Please rename these to have "ForInspector" (or something like it) at the end of the name so it's clear when this is expected to be used.

Done.

>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:170
>> +WebPageInspectorController::PauseLoading WebPageInspectorController::didCreateProvisionalPage(ProvisionalPageProxy& provisionalPage)
> 
> Having this function return something is very weird, and not at all what I'd expect from reading the name.  Please either create another function `shouldPauseProvisionalPage` (which would be called immediately after `didCreateProvisionalPage`) or rename `didCreateProvisionalPage` to indicate that it returns something which should be used to change some functionality.

Renamed to interceptProvisionalPage. Let me know if you have a better name in your mind.

>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:173
>> +    InspectorTarget* targetPtr = target.get();
> 
> `auto`?

Done.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3085
>> +    auto continuation = [this, navigation = makeRef(navigation), newProcess = WTFMove(newProcess), websitePolicies = WTFMove(websitePolicies)](bool isCancelled) mutable {
> 
> What is the point of `isCancelled`?  It seems like we always pass `false` when calling this function.
> 
> Also, why is this `mutable`?

Removed isCancelled.

It has to be `mutable` as the closure moves captured values (by default all of them are const in c++ lambdas).

>> LayoutTests/http/tests/inspector/target/provisional-load-cancels-previous-load.html:69
>> +is committed.
> 
> Style: please keep the `<p>` on one line.

Done.

>> LayoutTests/http/tests/inspector/target/resources/inline-debugger-statement.html:18
>> +</p>
> 
> Ditto

Done.

[Yury Semikhatsky]

Created attachment 384109 [details]
Patch

[Devin Rousso]

Comment on attachment 384109 [details]
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`.

[Yury Semikhatsky]

Comment on attachment 384109 [details]
Patch

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

>> Source/JavaScriptCore/inspector/InspectorTarget.h:66
>> +    bool m_pausedOnStart { false };
> 
> Please change this to be just `m_paused` to match the getter/setter naming.

Done.

>> Source/WebInspectorUI/UserInterface/Controllers/TargetManager.js:96
>> +            target.TargetAgent.setPauseOnStart(true);
> 
> Let's move this to `WI.MultiplexingBackendTarget.prototype.initialize` (please keep the existing comments).

Done.

>> 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;
> ```

Done. Unlike others it's used only once.

>> Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:50
>> +        let workerTarget = new WI.WorkerTarget(target, workerId, url, connection, isPaused);
> 
> Style: we normally omit trailing arguments if they're falsy.

Done.

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

Done.

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

Done.

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

It can be truthy here.

>> 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;`.

Done.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:43
>> +        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.

Done.

>> 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`.

Done.

>> 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;
>     });
> ```

Done.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:124
>> +    get isPaused() { return this._isPaused; }
> 
> Style: I'd put this right above `get isDestroyed()`.

Done.

>> 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();
>     }
> ```

We have to transition main target (_checkAndHandlePageTargetTransition) before dispatching the messages, so the sequence is the following:
1. targets is marked as committed
2. main target updated
3. deferred messaages are dispatched

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

Done.

>> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:708
>> +                return !!typeIdentifier;
> 
> Why was this changed?

To avoid resource:///org/webkit/inspector/UserInterface/Views/ContentView.js:193:132: CONSOLE ERROR Error: Can't make a ContentView for an unknown representedObject of type: CallFrame

>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:81
>> +        setPaused(false);
> 
> Shouldn't this be before the early-return above?

It should, nice catch!

>> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:126
>> +    m_loadingPaused = false;
> 
> We should `ASSERT(m_loadingPausedForInspector);` at the beginning of this function.

Done.

>> 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();
> ```

Done.

>> Source/WebKit/UIProcess/ProvisionalPageProxy.h:146
>> +    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 };
> ```

Done.

>> 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`.

should* (as well as is*) usually implies no side effects whereas inside this method we also create target proxy and notify the frontend. I'd prefer a method name that would make the enum redundant but couldn't come up with a good one.

>> 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`.

Done. This is not strictly necessary though as WebPageProxy owns provisional page where we pass ownership of the callback.

[Yury Semikhatsky]

Created attachment 384741 [details]
Patch

[Yury Semikhatsky]

Created attachment 384757 [details]
Patch

[Devin Rousso]

Comment on attachment 384741 [details]
Patch

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

It looks like 'http/tests/inspector/target/provisional-load-cancels-previous-load.html' is still failing :(

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:63
> +void InspectorTargetAgent::resume(ErrorString& error, const String& targetId)

NIT: we normally use `errorString` instead of `error`.

> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:-121
> -    target.disconnect();

Why was this removed?

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

Please use the actual supported options `{isProvisional, isPaused} = {}` (or `options = {}` at the very least) instead.

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

Since `isPaused` is effectively optional, please add a fallback.
```
    this._isPaused = !!isPaused;
```

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:39
> +        console.assert(!isPaused || parentTarget.hasCommand("Target.setPauseOnStart"));

Please put this assertion next to the one above.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:40
> +        this._isProvisional = isProvisional;

Ditto (38).
```
    this._isProvisional = !!isProvisional;
```

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:197
> +

Style: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Protocol/Target.js:199
> +

Ditto (197)

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

Ditto (PageTarget.js:28)

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

>> Why was this changed?
> To avoid resource:///org/webkit/inspector/UserInterface/Views/ContentView.js:193:132: CONSOLE ERROR Error: Can't make a ContentView for an unknown representedObject of type: CallFrame
Do you have steps to reproduce this?  A backtrace?  This should be a separate bug.

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:145
> +    WTF::Function<void()> m_continueLoadingWhenResumedByInspectorCallback;

NIT: I'd add a newline before this so there's an inspector "section".

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

I think we should move these to a map on `WebPageInspectorController` or `InspectorTargetAgent` instead, so that `ProvisionalPageProxy` doesn't have these extra values, especially since `ProvisionalPageProxy` itself doesn't actually really _need_ any of these values (they're just stored here for somewhat convenience).  I realize that there are probably very few instances of `ProvisionalPageProxy`, but we should try to avoid as much as we reasonably can such as if Web Inspector isn't open.  This way, we could also go back to `void didCreateProvisionalPage` and have the pausing/callback logic wholly contained, organized, and invoked within Web Inspector code.  We already do something very similar for network interception (e.g. local resource overrides) in `InspectorNetworkAgent`.

[Yury Semikhatsky]

Comment on attachment 384741 [details]
Patch

> It looks like 'http/tests/inspector/target/provisional-load-cancels-previous-load.html' is still failing :(
Patch from #c12 should fixed the test by avoiding racy navigation.



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

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:63
>> +void InspectorTargetAgent::resume(ErrorString& error, const String& targetId)
> 
> NIT: we normally use `errorString` instead of `error`.

Changed.

>> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:-121
>> -    target.disconnect();
> 
> Why was this removed?

That's because the cancellation test was crashing with the trace below. In any case if the tarter is destroyed, corresponding InspectorTarget is going to be destroyed as well and there is no value in calling disconnect. As for the front-end we still report that the target has been destroyed.



Thread 1 (Thread 0x7f6a5995fa00 (LWP 48946)):
#0  0x00007f6a69ea5f65 in WebKit::ProvisionalPageProxy::loadRequest (this=0x0, navigation=..., request=..., shouldOpenExternalURLsPolicy=WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, userData=0x0, websitePolicies=...) at ../../Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:177
#1  0x00007f6a69ed4d15 in WebKit::WebPageProxy::continueNavigationInNewProcess(API::Navigation&, std::unique_ptr<WebKit::SuspendedPageProxy, std::default_delete<WebKit::SuspendedPageProxy> >&&, WTF::Ref<WebKit::WebProcessProxy, WTF::DumbPtrTraits<WebKit::WebProcessProxy> >&&, WebKit::ProcessSwapRequestedByClient, WTF::Optional<WebKit::WebsitePoliciesData>&&)::$_0::operator()() (this=<optimized out>) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:3122
#2  0x00007f6a69ea5b76 in WTF::Function<void ()>::operator()() const (this=0x7f6a5907dc08) at DerivedSources/ForwardingHeaders/wtf/Function.h:79
#3  WebKit::ProvisionalPageProxy::continueLoadingAfterPausingForInspector (this=0x7f6a5907dac0) at ../../Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:124
#4  0x00007f6a69ea067b in Inspector::InspectorTarget::setPaused (this=0x7f6a5900c120, paused=false) at DerivedSources/ForwardingHeaders/JavaScriptCore/InspectorTarget.h:55
#5  WebKit::InspectorTargetProxy::disconnect (this=0x7f6a5900c120) at ../../Source/WebKit/UIProcess/InspectorTargetProxy.cpp:73
#6  0x00007f6a66440254 in Inspector::InspectorTargetAgent::targetDestroyed (this=<optimized out>, target=...) at ../../Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:146
#7  0x00007f6a69ec6999 in WebKit::WebPageInspectorController::destroyInspectorTarget (this=0x7f6a590f2120, targetId=...) at ../../Source/WebKit/UIProcess/WebPageInspectorController.cpp:161
#8  0x00007f6a69ec6b31 in WebKit::WebPageInspectorController::willDestroyProvisionalPage (this=0x0, provisionalPage=...) at ../../Source/WebKit/UIProcess/WebPageInspectorController.cpp:180
#9  0x00007f6a69ea5946 in WebKit::ProvisionalPageProxy::~ProvisionalPageProxy (this=0x7f6a5907dac0) at ../../Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:96
#10 0x00007f6a69ea5b09 in WebKit::ProvisionalPageProxy::~ProvisionalPageProxy (this=0x7f6a5907dac0) at ../../Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:94
#11 0x00007f6a69ed4538 in std::default_delete<WebKit::ProvisionalPageProxy>::operator() (__ptr=<optimized out>, this=<optimized out>) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/unique_ptr.h:78
#12 std::unique_ptr<WebKit::ProvisionalPageProxy, std::default_delete<WebKit::ProvisionalPageProxy> >::reset (this=<optimized out>, __p=<optimized out>) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/unique_ptr.h:376
#13 std::unique_ptr<WebKit::ProvisionalPageProxy, std::default_delete<WebKit::ProvisionalPageProxy> >::operator=(decltype(nullptr)) (this=<optimized out>) at /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/unique_ptr.h:312
#14 WebKit::WebPageProxy::continueNavigationInNewProcess (this=0x7f6a3d6fc000, navigation=..., suspendedPage=..., newProcess=..., processSwapRequestedByClient=WebKit::ProcessSwapRequestedByClient::No, websitePolicies=...) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:3095
#15 0x00007f6a69ee859d in WebKit::WebPageProxy::receivedNavigationPolicyDecision(WebCore::PolicyAction, API::Navigation*, WebKit::ProcessSwapRequestedByClient, WebKit::WebFrameProxy&, API::WebsitePolicies*, WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >&&)::$_7::operator()(WTF::Ref<WebKit::WebProcessProxy, WTF::DumbPtrTraits<WebKit::WebProcessProxy> >&&, WebKit::SuspendedPageProxy*, WTF::String const&) (this=<optimized out>, processForNavigation=..., destinationSuspendedPage=<optimized out>, reason=...) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:3006
#16

>> Source/WebInspectorUI/UserInterface/Protocol/PageTarget.js:28
>> +    constructor(parentTarget, targetId, name, connection, state)
> 
> Please use the actual supported options `{isProvisional, isPaused} = {}` (or `options = {}` at the very least) instead.

Done. Not sure if that's what you meant.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:38
>> +        this._isPaused = isPaused;
> 
> Since `isPaused` is effectively optional, please add a fallback.
> ```
>     this._isPaused = !!isPaused;
> ```

Done.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:39
>> +        console.assert(!isPaused || parentTarget.hasCommand("Target.setPauseOnStart"));
> 
> Please put this assertion next to the one above.

Done.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:40
>> +        this._isProvisional = isProvisional;
> 
> Ditto (38).
> ```
>     this._isProvisional = !!isProvisional;
> ```

Done.

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:197
>> +
> 
> Style: unnecessary newline.

done

>> Source/WebInspectorUI/UserInterface/Protocol/Target.js:199
>> +
> 
> Ditto (197)

done

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

Done.

>> Source/WebKit/UIProcess/ProvisionalPageProxy.h:145
>> +    WTF::Function<void()> m_continueLoadingWhenResumedByInspectorCallback;
> 
> NIT: I'd add a newline before this so there's an inspector "section".

done

>> Source/WebKit/UIProcess/ProvisionalPageProxy.h:146
>> +    bool m_loadingPausedForInspector { false };
> 
> I think we should move these to a map on `WebPageInspectorController` or `InspectorTargetAgent` instead, so that `ProvisionalPageProxy` doesn't have these extra values, especially since `ProvisionalPageProxy` itself doesn't actually really _need_ any of these values (they're just stored here for somewhat convenience).  I realize that there are probably very few instances of `ProvisionalPageProxy`, but we should try to avoid as much as we reasonably can such as if Web Inspector isn't open.  This way, we could also go back to `void didCreateProvisionalPage` and have the pausing/callback logic wholly contained, organized, and invoked within Web Inspector code.  We already do something very similar for network interception (e.g. local resource overrides) in `InspectorNetworkAgent`.

Done. Moved this logic to InspectorTargetProxy.

[Yury Semikhatsky]

Created attachment 384765 [details]
Patch

[Devin Rousso]

Comment on attachment 384765 [details]
Patch

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

I think we're pretty much done!  AWESOME work! =D

Please wait until you see a green mac-wk2 EWS after responding to the following:

> Source/WebInspectorUI/ChangeLog:19
> +        * UserInterface/Controllers/CSSManager.js:
> +        (WI.CSSManager.prototype._resourceContentDidChange.applyStyleSheetChanges.styleSheetFound):
> +        (WI.CSSManager.prototype._resourceContentDidChange.applyStyleSheetChanges):
> +        (WI.CSSManager.prototype._resourceContentDidChange):
> +        (WI.CSSManager.prototype._updateResourceContent.fetchedStyleSheetContent):

These changes are no longer in this patch.  Please regenerate the ChangeLog.

> Source/JavaScriptCore/inspector/InspectorTarget.h:51
> +    void setPaused(bool paused)

Rather than "overload" this with double functionality, I think we should have separate `pause` and `resume` functions.
```
    void pause()
    {
        ASSERT(!m_isPaused);
        m_isPaused = true;
    }

    void resume()
    {
        ASSERT(m_isPaused);
        m_isPaused = false;

        if (m_resumeCallback) {
            m_resumeCallback();
            m_resumeCallback = nullptr;
        }
    }
```

Ideally we'd also be able to `ASSERT` that we don't ever `pause()` more than once, but it's not strictly necessary.

At this point, I think we should also create a 'Source/JavaScriptCore/inspector/InspectorTarget.cpp'.

> Source/WebCore/inspector/InspectorFrontendHost.cpp:373
> +    fprintf(stderr, "sendMessageToBackend %s\n", message.left(200).ascii().data());

Oops.  Please remove.

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

Per my previous comment, please put this into a separate change.  It deserves its own explanation.

> Source/WebKit/WebProcess/WebPage/WebInspectorFrontendAPIDispatcher.cpp:95
> +    fprintf(stderr, "evaluateOrQueueExpression %s\n", expression.left(200).ascii().data());

Oops.  Please remove.

[Yury Semikhatsky]

Comment on attachment 384765 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:19
>> +        (WI.CSSManager.prototype._updateResourceContent.fetchedStyleSheetContent):
> 
> These changes are no longer in this patch.  Please regenerate the ChangeLog.

Done.

>> Source/JavaScriptCore/inspector/InspectorTarget.h:51
>> +    void setPaused(bool paused)
> 
> Rather than "overload" this with double functionality, I think we should have separate `pause` and `resume` functions.
> ```
>     void pause()
>     {
>         ASSERT(!m_isPaused);
>         m_isPaused = true;
>     }
> 
>     void resume()
>     {
>         ASSERT(m_isPaused);
>         m_isPaused = false;
> 
>         if (m_resumeCallback) {
>             m_resumeCallback();
>             m_resumeCallback = nullptr;
>         }
>     }
> ```
> 
> Ideally we'd also be able to `ASSERT` that we don't ever `pause()` more than once, but it's not strictly necessary.
> 
> At this point, I think we should also create a 'Source/JavaScriptCore/inspector/InspectorTarget.cpp'.

Done. I was trying to avoid the pain of adding a new file but I agree that it's too much logic for the header :-)

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:373
>> +    fprintf(stderr, "sendMessageToBackend %s\n", message.left(200).ascii().data());
> 
> Oops.  Please remove.

Done.

>> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:708
>> +                return !!typeIdentifier;
> 
> Per my previous comment, please put this into a separate change.  It deserves its own explanation.

Sorry missed that comment last time. PTAL https://bugs.webkit.org/show_bug.cgi?id=204823

>> Source/WebKit/WebProcess/WebPage/WebInspectorFrontendAPIDispatcher.cpp:95
>> +    fprintf(stderr, "evaluateOrQueueExpression %s\n", expression.left(200).ascii().data());
> 
> Oops.  Please remove.

Done.

[Yury Semikhatsky]

Created attachment 384771 [details]
Patch

[Devin Rousso]

Comment on attachment 384771 [details]
Patch

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

r=me, nice work!  Thanks for iterating :)

Please wait for EWS to be green on mac-wk2 before landing.

> Source/JavaScriptCore/inspector/InspectorTarget.cpp:46
> +    m_isPaused = false;
> +    if (m_resumeCallback) {

NIT: I'd add a newline

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:60
> +        this._provisionalMessages.push(message);

We should also assert `console.assert(this.target && this.target.isProvisional);`.

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:66
> +        console.assert(this.target);
> +        console.assert(!this.target.isProvisional);

NIT: you could combine these assertions.

> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:69
> +        this._provisionalMessages = null;

We should set this back to `[]` to match the `constructor`.

[Yury Semikhatsky]

Comment on attachment 384771 [details]
Patch

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

>> Source/JavaScriptCore/inspector/InspectorTarget.cpp:46
>> +    if (m_resumeCallback) {
> 
> NIT: I'd add a newline

Done.

>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:60
>> +        this._provisionalMessages.push(message);
> 
> We should also assert `console.assert(this.target && this.target.isProvisional);`.

Done.

>> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:69
>> +        this._provisionalMessages = null;
> 
> We should set this back to `[]` to match the `constructor`.

Done.

[Yury Semikhatsky]

Created attachment 384791 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384791 [details]
Patch for landing

Clearing flags on attachment: 384791

Committed r253097: <https://trac.webkit.org/changeset/253097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57617094>

[Truitt Savell]

The new test http/tests/inspector/target/pause-on-inline-debugger-statement.html

added in https://trac.webkit.org/changeset/253097/webkit

is crashing on Mac debug 

https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Finspector%2Ftarget%2Fpause-on-inline-debugger-statement.html

crash:
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r253149%20(6209)/http/tests/inspector/target/pause-on-inline-debugger-statement-crash-log.txt

Can this be looked at today?

[Yury Semikhatsky]

I'll have a look.

[Yury Semikhatsky]

Filed https://webkit.org/b/204901 to track this, the assertion is failing on all platforms.