Bug 153034 – Web Inspector: Timelines UI redesign: add the timelines tree outline to the TimelineOverview

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
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.

[Radar WebKit Bug Importer]

<rdar://problem/24195628>

[Matt Baker]

Created attachment 270473 [details]
[Patch] Proposed Fix

[Matt Baker]

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.

[Joseph Pecoraro]

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.

[Matt Baker]

Created attachment 270707 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 270712 [details]
[Image] New UI

Shown with navigation sidebar collapsed.

[Joseph Pecoraro]

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.

[Matt Baker]

Created attachment 271447 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 271450 [details]
[Image] New overview UI (sidebar collapsed)

[Timothy Hatcher]

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%);

Yes!

[Matt Baker]

Created attachment 271580 [details]
Updated for Variables.css changes in https://webkit.org/b/154302

[Matt Baker]

Created attachment 272694 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 272694 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 272694

Committed r197473: <http://trac.webkit.org/changeset/197473>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.