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

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted 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 384771: Patch

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




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