Bug 172467 – Web Inspector: Don't load the active recording until a Timeline view needs to be shown

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

[Devin Rousso]

The active recording `WebInspector.timelineManager.activeRecording` is displayed when the Timeline tab is created, causing a ContentView to be created even though it isn't being shown.  Instead of displaying the recording then, we can do it later when a timeline view needs to be shown, assuming a different recording hasn't already been loaded.

[Devin Rousso]

Created attachment 310906 [details]
Patch

[Devin Rousso]

Created attachment 310907 [details]
[Image] Timeline Before

[Devin Rousso]

Created attachment 310908 [details]
[Image] Timeline After

[Matt Baker]

Nice, a 50 ms improvement is pretty good! Did you take more than one sample?

[Devin Rousso]

(In reply to Matt Baker from comment #4)
> Nice, a 50 ms improvement is pretty good! Did you take more than one sample?

I re-ran it about 5 times, each on a different tab, and saw around the same improvement.

Additionally, I tested the few approaches I know of to load the inspector on the Timelines tab (Develop menu, reloading when Timelines was last selected, clicking the dashboard, etc.) and saw no issues.

[Joseph Pecoraro]

Comment on attachment 310906 [details]
Patch

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

r- for question below.

> Source/WebInspectorUI/ChangeLog:12
> +        `_displayedRecording` isn't already set.  This delays the creation of the content view for

Style: One space after a period.

> Source/WebInspectorUI/ChangeLog:19
> +
> +

Style: Too many blank lines

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:537
> +        if (!this._displayedRecording && WebInspector.timelineManager.activeRecording)
> +            this._recordingLoaded();

I think this deserves a comment, because it is really not the right place for this.

Perhaps it should just be inside a function named `_lazyInitialization`. It is also unclear to me why this is the correct place and not something like `shown` or `restoreFromCookie`. Can you describe why this is the most appropriate?

[Devin Rousso]

Comment on attachment 310906 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:537
>> +            this._recordingLoaded();
> 
> I think this deserves a comment, because it is really not the right place for this.
> 
> Perhaps it should just be inside a function named `_lazyInitialization`. It is also unclear to me why this is the correct place and not something like `shown` or `restoreFromCookie`. Can you describe why this is the most appropriate?

I am investigating moving it to `restoreFromCookie`.  I agree that it was badly placed, but I put it here because it was the one function that every "entry" to the Timelines tab seemed to call.

[Devin Rousso]

Created attachment 311043 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311043 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:326
>  

Does that me we should be able to assert down here that we have a _displayedRecording? Before this change, and the previous patch, would have loaded the active recording in this path as well but this patch does not. Why does it not need to be done here?

[Devin Rousso]

Created attachment 311080 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311080 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 311080 [details]
Patch

Clearing flags on attachment: 311080

Committed r217379: <http://trac.webkit.org/changeset/217379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32479815>