Bug 153034

Summary: Web Inspector: Timelines UI redesign: add the timelines tree outline to the TimelineOverview
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 153146, 154000    
Bug Blocks: 153036, 153904    
Description Flags
[Patch] Proposed Fix
[Patch] Proposed Fix
[Image] New UI
[Patch] Proposed Fix
[Image] New overview UI (sidebar collapsed)
Updated for Variables.css changes in https://webkit.org/b/154302
[Patch] Proposed Fix none

Description Matt Baker 2016-01-12 12:50:24 PST
Add the timelines tree outline to the TimeilneOverview. The new design eliminates the navigation sidebar, so this needs a new home. Some things to consider:

- Whether to add a resizer to the overview, or automatically adjust the tree outline's width to fit the widest timeline title.
- Timelines could be selected by clicking directly on the graph, in addition to the timeline's tree element.
- A tree outline won't always be shown. For example, I expect the Rendering Frames graph will fill the entire width of the overview.
Comment 1 Radar WebKit Bug Importer 2016-01-14 14:35:59 PST
Comment 2 Matt Baker 2016-02-01 22:12:09 PST
Created attachment 270473 [details]
[Patch] Proposed Fix
Comment 3 Matt Baker 2016-02-04 12:38:55 PST
Comment on attachment 270473 [details]
[Patch] Proposed Fix

Waiting on other UI patches. See blocking issues for https://bugs.webkit.org/show_bug.cgi?id=153036.
Comment 4 Joseph Pecoraro 2016-02-04 13:54:29 PST
Comment on attachment 270473 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.css:52
> -    height: 29px;
> +    height: 23px;

Can you see what this does to the MemoryTimelineView? I think this will break positioning of things.
Comment 5 Matt Baker 2016-02-04 17:40:45 PST
Created attachment 270707 [details]
[Patch] Proposed Fix
Comment 6 Matt Baker 2016-02-04 18:00:28 PST
Created attachment 270712 [details]
[Image] New UI

Shown with navigation sidebar collapsed.
Comment 7 Joseph Pecoraro 2016-02-04 19:18:17 PST
Comment on attachment 270707 [details]
[Patch] Proposed Fix

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

r- for the questions raised below.

> Source/WebInspectorUI/ChangeLog:14
> +        Web Inspector: Timelines UI redesign: add the timelines tree outline to the TimelineOverview
> +        https://bugs.webkit.org/show_bug.cgi?id=153034
> +        <rdar://problem/24195628>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Move the Timelines tree outline to the TimelineRecordingContentView, in preparation
> +        for removing the navigation sidebar in <https://webkit.org/b/153036>.
> +
> +        TimelineOverview dispatches two new events, GraphAdded and GraphRemoved. This was
> +        added so the TimelineRecordingContent can set the height of timeline tree elements
> +        to match the height of the corresponding TimelineOverGraph.

The title says "add timelines tree outline to TimelineOverview" but it is instead adding it to TimelineRecordingContentView.

TimelineOverview, already knows about the graphs. And TimelineOverviewGraph already knows its height. So putting it in TimelineOverview sounds like it would have avoided these GraphAdded / GraphRemoved events.

Why did you end up putting the TreeOutline in the RecordingContentView instead of the Overview?

> Source/WebInspectorUI/ChangeLog:24
> +        * UserInterface/Views/LayoutTimelineOverviewGraph.js:
> +        (WebInspector.LayoutTimelineOverviewGraph):
> +        * UserInterface/Views/MemoryTimelineOverviewGraph.js:
> +        (WebInspector.MemoryTimelineOverviewGraph):
> +        (WebInspector.MemoryTimelineOverviewGraph.prototype.layout):
> +        (WebInspector.MemoryTimelineOverviewGraph.prototype._visibleRecords):        
> +        * UserInterface/Views/NetworkTimelineOverviewGraph.js:
> +        (WebInspector.NetworkTimelineOverviewGraph):
> +        Refactoring for new TimelineOverviewGraph timeline property.

All of this could be in a standalone patch separate from this one. It would have made reviewing this one simpler.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverviewGraph.js:28
> -    constructor(timelineOverview)
> +    constructor(timeline, timelineOverview)

Hmm, this implies that a TimelineOverviewGraph will only have a single timeline. In practice, that is currently the case. Maybe we should strive to keep it that way. There is certainly a lot of code right now assuming it, so this seems fine.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:54
> +        WebInspector.TimelineOverview.addEventListener(WebInspector.TimelineOverview.Event.GraphAdded, this._timelineOverviewGraphAdded, this);

This is listening for ANY timeline overview adding a graph. This includes a different RecordingContentView's TimelineOverview getting a graph.

Likewise, it doesn't listen for graphs getting removed. It seems like it should.

Ideally this listener would be added to the TimelineOverview instances.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:626
> +        let height = this._timelineOverviewGraphHeightMap.get(timeline);
> +        if (height) {
> +            timelineTreeElement.listItemElement.style.height = height + "px";
> +            this._timelineOverviewGraphHeightMap.delete(timeline);
> +        }

This seems gross. It looks like the RecordingContentView is juggling the height value between GraphAdded and InstrumentAdded because it doesn't know both at the same time. Is that right?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:647
> +        this._timelineOverviewGrapHeightMap.delete(timeline);

Typo in this variable name would throw exceptions.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:677
> +        console.assert(timelineOverviewGraph.timelineOverview === this._currentTimelineOverview);

Seems this assert would fire frequently when this TimelineRecordingContentView is hidden and a different TimelineRecordingContentView is active.
Comment 8 Matt Baker 2016-02-16 11:16:37 PST
Created attachment 271447 [details]
[Patch] Proposed Fix
Comment 9 Matt Baker 2016-02-16 11:17:58 PST
Created attachment 271450 [details]
[Image] New overview UI (sidebar collapsed)
Comment 10 Timothy Hatcher 2016-02-16 11:27:08 PST
Comment on attachment 271447 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/Variables.css:37
> +    --border-color: hsl(0, 0%, 70%);
> +    --border-inactive-color: hsl(0, 0%, 85%);
> +    --panel-background-color: hsl(0, 0%, 94%);

Comment 11 Matt Baker 2016-02-17 12:56:12 PST
Created attachment 271580 [details]
Updated for Variables.css changes in https://webkit.org/b/154302
Comment 12 Matt Baker 2016-03-02 15:28:04 PST
Created attachment 272694 [details]
[Patch] Proposed Fix
Comment 13 WebKit Commit Bot 2016-03-02 16:20:52 PST
Comment on attachment 272694 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 272694

Committed r197473: <http://trac.webkit.org/changeset/197473>
Comment 14 WebKit Commit Bot 2016-03-02 16:20:58 PST
All reviewed patches have been landed.  Closing bug.