[webkit-reviews] review granted: [Bug 132875] Web Inspector: support storing multiple timeline recordings in the manager : [Attachment 236029] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Brian Burg
<burg at cs.washington.edu>'s request for review:
Bug 132875: Web Inspector: support storing multiple timeline recordings in the
manager
https://bugs.webkit.org/show_bug.cgi?id=132875

Attachment 236029: Patch
https://bugs.webkit.org/attachment.cgi?id=236029&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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(resourceTimeli
neRecord.resource).length);
> +	  
console.assert(!this.representedObject.sourceCodeTimelinesForSourceCode(resourc
eTimelineRecord.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.