Bug 132875 – Web Inspector: support storing multiple timeline recordings in the manager

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

[Brian Burg]

The inspector currently drops old recordings on the floor. We should instead have a set of them, and an "active" recording.

This allows us to import/export recordings, add UI to switch between them, etc.

[Radar WebKit Bug Importer]

<rdar://problem/16900269>

[Brian Burg]

I've started prototyping this. We can add UI for switching between recordings later.

Some issues:

* If the TimelineContentView now represents TimelineRecording, then we need to create a new view for each recording. I am trying out creating a new recording whenever we would have done TimelineRecording.reset() before.

* We should save the selected timeline across resets (i.e., creating and displaying a new recording), but not across switching between views. Right?

* We shouldn't hold onto all timeline data forever. I think we could use a LRU cache with n=10 to support re-navigating in a drop-down select menu. I would probably want to land this separately with the UI that depends on it.

[Brian Burg]

Another thing I'm unsure about is how to implement the path components so that viewing the overview shows:

Timelines > www.example.com

and each timeline shows:

Timelines > www.example.com > Network Requests

and you can switch between the last 'n' recordings by clicking on the www.example.com component.

If the TimelineContentView is supplying the path components but we switch out the TimelineContentView when another recording is selected via path components, will this work? Or, do we need some sort of persistent "cluster" view that provides path components for all recordings?

[Timothy Hatcher]

The Timelines sidebar can supply a base path. The view supplies a selection path (which is the selected timeline).

So the sidebar should be able to provide any path needed.

[Timothy Hatcher]

Actually right now the ContentBrowser builds the base path based on the TreeElement ancestors that matches the represented object. So to do this today, it would require a matching tree with the desired hierarchy. More delegate hooks could be added to change that requirement.

We do artificially create a "Timelines" tree element for the base of the path in TimelineSidebarPanel (_timelineOverviewTreeElement).

[Brian Burg]

(In reply to comment #5)
> Actually right now the ContentBrowser builds the base path based on the TreeElement ancestors that matches the represented object. So to do this today, it would require a matching tree with the desired hierarchy. More delegate hooks could be added to change that requirement.
> 
> We do artificially create a "Timelines" tree element for the base of the path in TimelineSidebarPanel (_timelineOverviewTreeElement).

So, what if we create a separate content tree outline for recordings that is never shown? We can proxy out attempts to select recording tree elements to the alternate drop-down-based UI.

[Timothy Hatcher]

(In reply to comment #6)
> (In reply to comment #5)
> > Actually right now the ContentBrowser builds the base path based on the TreeElement ancestors that matches the represented object. So to do this today, it would require a matching tree with the desired hierarchy. More delegate hooks could be added to change that requirement.
> > 
> > We do artificially create a "Timelines" tree element for the base of the path in TimelineSidebarPanel (_timelineOverviewTreeElement).
> 
> So, what if we create a separate content tree outline for recordings that is never shown? We can proxy out attempts to select recording tree elements to the alternate drop-down-based UI.

That is an option. It could also be a tree outline that is shown by sliding in and out like we talked about before.

[Brian Burg]

Created attachment 236029 [details]
Patch

[Timothy Hatcher]

Comment on attachment 236029 [details]
Patch

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

Screenshot?

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:40
> +    function delayedWork() {

Newline before {.

> Source/WebInspectorUI/UserInterface/Controllers/TimelineManager.js:102
> +        var result = TimelineAgent.stop();

Why store result if we don't use it? Merge cruft when it was used to stop async?

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:31
> +        // When instantiated directly, potentially return an instance of a concrete subclass.
> +        if (type === WebInspector.TimelineRecord.Type.Network)
> +            return new WebInspector.NetworkTimeline(type);

Nice!

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.js:310
> -        console.assert(!this._recording.sourceCodeTimelinesForSourceCode(resourceTimelineRecord.resource).length);
> +        console.assert(!this.representedObject.sourceCodeTimelinesForSourceCode(resourceTimelineRecord.resource).length);

Even if recording is the representedObject, I still like having a var with a more descriptive name (like it was) to hint at the type of object. Just use recording.sourceCodeTimelinesForSourceCode here?

> Source/WebInspectorUI/UserInterface/Views/TimelineContentView.js:36
> +    for (var [identifier, timeline] of recording.timelines)
> +        this._discreteTimelineOverviewGraphMap.set(timeline, new WebInspector.TimelineOverviewGraph(timeline));

Woot!

> Source/WebInspectorUI/UserInterface/Views/TimelineContentView.js:53
> +    for (var [identifier, timeline] of recording.timelines)
> +        this._discreteTimelineViewMap.set(timeline, new WebInspector.TimelineView(timeline));

Yay!

> Source/WebInspectorUI/UserInterface/Views/TimelineContentView.js:65
> +    this._pathComponentMap.set(recording.timelines.get(WebInspector.TimelineRecord.Type.Network), createPathComponent.call(this, WebInspector.UIString("Network Requests"), WebInspector.TimelineSidebarPanel.NetworkIconStyleClass, recording.timelines.get(WebInspector.TimelineRecord.Type.Network)));
> +    this._pathComponentMap.set(recording.timelines.get(WebInspector.TimelineRecord.Type.Layout), createPathComponent.call(this, WebInspector.UIString("Layout & Rendering"), WebInspector.TimelineSidebarPanel.ColorsIconStyleClass, recording.timelines.get(WebInspector.TimelineRecord.Type.Layout)));
> +    this._pathComponentMap.set(recording.timelines.get(WebInspector.TimelineRecord.Type.Script), createPathComponent.call(this, WebInspector.UIString("JavaScript & Events"), WebInspector.TimelineSidebarPanel.ScriptIconStyleClass, recording.timelines.get(WebInspector.TimelineRecord.Type.Script)));

These hit the Map two times each. Maybe store them in locals?

> Source/WebInspectorUI/UserInterface/Views/TimelineContentView.js:110
> +    showTimelineViewForTimeline: function(representedObject)

Why representedObject and not timeline?

> Source/WebInspectorUI/UserInterface/Views/TimelineContentView.js:177
> +    saveToCookie: function(cookie)

Yay!

> Source/WebInspectorUI/UserInterface/Views/TimelineOverviewGraph.js:29
> +    if (this.constructor === WebInspector.TimelineOverviewGraph) {
> +        // When instantiated directly return an instance of a type-based concrete subclass.

This works well here.

[Brian Burg]

Committed r172094: <http://trac.webkit.org/changeset/172094>

[Joseph Pecoraro]

This patch is causing a number of issues with Timelines.

1. Uncaught exception
showTimelineViewForType called with an undefined type from restoreFromCookie. You mentioned this should fix that problem:

-        if (selectedTimelineViewIdentifier === WebInspector.TimelineSidebarPanel.OverviewTimelineIdentifierCookieValue)
+        if (!selectedTimelineViewIdentifier || selectedTimelineViewIdentifier === WebInspector.TimelineSidebarPanel.OverviewTimelineIdentifierCookieValue)

2. Timeline views showing up completely blank

- reload a page with the inspector open => overview/specific views are blank
- make new selections in the timeline => views are still blank
- switch recordings => causes things to work, but you have to make a new time selection

I don't quite understand the issues for #2. I wonder if we should roll this out while it is investigated.

[Brian Burg]

(In reply to comment #11)
> This patch is causing a number of issues with Timelines.
> 
> 1. Uncaught exception
> showTimelineViewForType called with an undefined type from restoreFromCookie. You mentioned this should fix that problem:
> 
> -        if (selectedTimelineViewIdentifier === WebInspector.TimelineSidebarPanel.OverviewTimelineIdentifierCookieValue)
> +        if (!selectedTimelineViewIdentifier || selectedTimelineViewIdentifier === WebInspector.TimelineSidebarPanel.OverviewTimelineIdentifierCookieValue)
> 
> 2. Timeline views showing up completely blank
> 
> - reload a page with the inspector open => overview/specific views are blank
> - make new selections in the timeline => views are still blank
> - switch recordings => causes things to work, but you have to make a new time selection
> 
> I don't quite understand the issues for #2. I wonder if we should roll this out while it is investigated.

Opened https://bugs.webkit.org/show_bug.cgi?id=135742 to track a fix.