[webkit-reviews] review granted: [Bug 204086] Web Inspector: REGRESSION(r250618): main resource view is empty when pausing on inline 'debugger' statement : [Attachment 383368] 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 204086: Web Inspector: REGRESSION(r250618): main resource view is empty
when pausing on inline 'debugger' statement
https://bugs.webkit.org/show_bug.cgi?id=204086

Attachment 383368: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:72
> +    get editableRevision()

Please move this after `set currentRevision` so we don't split up a get-set
pair.

> Source/WebInspectorUI/UserInterface/Models/SourceCode.js:190
> +	   if (revision !== this._currentRevision)

Let's add a `console.assert(revision === this._currentRevision);` before this. 
I don't think we ever want to update non-current revisions.

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:215
> +	   // The view maybe populated with inline scripts content by the time
resource
> +	   // content arrives. SourceCodeTextEditor will handle that.
> +	   if (this._hasContent())
> +	       return;

Why is this needed?  I tried doing some testing myself and it seems to work
fine.  Do you have a specific test case where this is needed?  If so, what
exactly is "wrong" when this isn't included?

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:297
> +	       let revision =
localResourceOverride.localResource.editableRevision;

Aside: we should probably move this inside the callback, in case the revision
changes in between.