[webkit-reviews] review granted: [Bug 192248] Web Inspector: REGRESSION(r238330): Timeline auto-capture does not work after page transition : [Attachment 356434] [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 192248: Web Inspector: REGRESSION(r238330): Timeline auto-capture does not
work after page transition
https://bugs.webkit.org/show_bug.cgi?id=192248

Attachment 356434: [PATCH] Proposed Fix

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




--- Comment #7 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 356434
  --> https://bugs.webkit.org/attachment.cgi?id=356434
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:69
> +	   if (target.TimelineAgent) {

Should we assert `!this._transitioningPageTarget` as a sanity check, since we'd
expect to call `transitionPageTarget` shortly thereafter?  Is it possible for
targets to switch quickly enough that `_mainResourceDidChange` wouldn't get
called to reset `_transitioningPageTarget`?

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:849
> +	   if (this._transitioningPageTarget) {

Just curious, is there a case where this wouldn't be true during some sort of
navigation?  My guess is that this isn't true for refreshes, but will be true
for all other forms of navigation (e.g. where the URL changes).