Bug 157588 – Web Inspector: Improve snapshot selection in heap allocations overview graph

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Created attachment 278670 [details]
[Video] snapshot selection

* SUMMARY
Improve snapshot selection in heap allocations overview graph.

- Now that http://webkit.org/b/156963 has landed, selecting snapshot icons make use of TimelineOverGraph.selectedRecord.
- Viewing a snapshot (by clicking an icon in the overview graph, or making a navigation bar selection) should make the icon in the graph appear "selected" (see video).

[Radar WebKit Bug Importer]

<rdar://problem/26231245>

[Timothy Hatcher]

Neat!

[Matt Baker]

Created attachment 278678 [details]
[Video] better snapshot selection

Added styles for a selected marker in a deselected graph, and the ability to select a graph by clicking a marker.

[Matt Baker]

(In reply to comment #3)
> Created attachment 278678 [details]
> [Video] better snapshot selection
> 
> Added styles for a selected marker in a deselected graph, and the ability to
> select a graph by clicking a marker.

This requires https://bugs.webkit.org/show_bug.cgi?id=157593.

[Matt Baker]

Created attachment 278690 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

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

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

r=me, but feel free to put up another patch given my comments.

> Source/WebInspectorUI/ChangeLog:37
> +        * UserInterface/Views/HeapAllocationsTimelineView.js:
> +        (WebInspector.HeapAllocationsTimelineView.prototype.get selectionPathComponents):
> +        Current content view may be null if path components are requested
> +        after data grid has been removed but before the snapshot view is added.

I'm not sure I understand this. The three show functions showHeapSnapshotList, showHeapSnapshotTimelineRecord, showHeapSnapshotDiff tend to be very conservative and only issue path components changes at the end. And adding/removing a subview only happens in these.

This seems fine, but when is the currentContentView null during a selectionPathComponents call?

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineOverviewGraph.css:40
> +.timeline-overview-graph.heap-allocations > img.snapshot.selected {
> +    content: url(../Images/HeapSnapshotSelected.svg);
> +}

We should probably do something about non-Apple ports, but that is not new here.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineOverviewGraph.js:82
> +            imageElement.addEventListener("click", () => this.selectedRecord = record);

With arrow functions, I have been pushing the style of using braces if the return value is not significant. (No braces, actually returns the value of the body statement). So I'd keep the braces here, because I think it ends up being clearer. You can look at it and know the return value is not significant.

    imageElement.addEventListener("click", () => { this.selectedRecord = record });

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineOverviewGraph.js:84
> +            this._recordImageElements.set(record, imageElement);

Seems like it might be easier / more efficient to just put the imageElement as a Symbol / private property on the record:

    record[WebInspector.HeapAllocationsTimelineOverviewGraph.RecordElementAssociationSymbol] = imageElement;

Instead of clearing and populating a Map every layout.

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineOverviewGraph.js:103
> +        this._selectedImageElement = null;

This could be done in the bail. That way there is always only one assignment.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverviewGraph.js:251
>          this._scheduledSelectedRecordLayoutUpdateIdentifier = requestAnimationFrame(function() {
>              this._scheduledSelectedRecordLayoutUpdateIdentifier = undefined;
> +            console.assert(this instanceof WebInspector.TimelineOverviewGraph);
>              this.updateSelectedRecord();
> +
> +            this.dispatchEventToListeners(WebInspector.TimelineOverviewGraph.Event.RecordSelected, {record: this.selectedRecord});
>          }.bind(this));

Lets use an arrow function here and eliminate the bind! I'm not sure the assert is needed.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:634
> +        let record = event.data.record;
> +        let timeline = event.data.timeline;

Might read easier as:

    let {record, timeline} = event.data;

[Matt Baker]

Created attachment 278725 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 278725

Committed r200767: <http://trac.webkit.org/changeset/200767>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.