Bug 195321 – Web Inspector: CPU Usage Timeline - Allow clicking a bar in the overview to select a tight time range around it

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

[Joseph Pecoraro]

CPU Usage Timeline - Allow clicking a bar in the overview to select a tight time range around it

Steps to reproduce:
1. Record a CPU usage timeline on a website
2. Click a CPU bar in the overview
  => Nothing happens, expected it to be selected

[Joseph Pecoraro]

Created attachment 363619 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 363619 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:113
> +        if (before < after)
> +            return recordBefore;
> +        return recordAfter;

NIT: I'd make this into a ternary.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:85
> +    fill: var(--selected-background-color) !important;
> +    stroke: var(--selected-background-color-active) !important;

Is there any way to avoid the `!important`? :(

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:86
> +    fill-opacity: 0.5;

NIT: I'd put this alongside the `fill` property above.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:49
> +        this._chart.element.addEventListener("click", this._handleGraphMouseClick.bind(this));

NIT: I'd use `_handleChartClick` instead, as it more closely matches the variable name ("Chart" vs "Graph") and event name ("Click vs "MouseClick").
NIT: I'd move this above to be immediately below the initialization of `this._chart`.  I prefer adding event listeners before adding the element to the DOM.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:54
> +        this._lastSelectedRecordLayout = null;

Why is "Layout" part of the name?  It seems unnecessary.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:119
> +        let visibleRecords = this._cpuTimeline.recordsInTimeRange(graphStartTime, visibleEndTime + (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / 2), includeRecordBeforeStart);

I thought you preferred using the non-`WI` name when referenced from within the class?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:127
> +        let intervalWidth = (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / secondsPerPixel);

Ditto (>119).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:134
> +            let x = xScale(record.startTime - (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / 2));

Ditto (>119).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:150
> +        if (this._lastSelectedRecordLayout !== this.selectedRecord)
> +            this.needsLayout();

It might be worth mentioning that this is necessary since we don't actually have any of the actual DOM nodes for the records.  As such, the only way to add a "selected" class to a matching record is by redrawing the entire graph.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:180
> +            // Try again lower, since the scroll bar prevents low clicks.
> +            const tryLowerOffset = 5;
> +            elements = document.elementsFromPoint(event.pageX, event.pageY + tryLowerOffset);
> +            rectElement = elements.find((x) => x.localName === "rect");

Do we still need this now that <https://webkit.org/b/195318> has landed?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:185
> +        let chartElement = rectElement.closest(".stacked-column-chart");

It's cases like these that I see the utility in having a static variable for the CSS class of the element (e.g. `WI.StackedColumnChart.className`).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:194
> +        let rect = chartElement.getBoundingClientRect();
> +        let position = event.pageX - rect.left;
> +
> +        if (WI.resolvedLayoutDirection() === WI.LayoutDirection.RTL)
> +            return rect.width - position;
> +        return position;

We should make this into a utility function on `Element.prototype` given how often we seem to use it.  Something like `positionWithinElement`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:199
> +        let clickPosition = this._graphPositionForMouseEvent(event);

Shouldn't this be called `graphPosition` to match the function name?  If not, we should rename it to `_clickPositionForMouseEvent` or something like that.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:203
> +        let secondsPerPixel = this.timelineOverview.secondsPerPixel;

NIT: I'd inline this.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:207
> +        let clickTime = graphStartTime + graphClickTime;

Ditto (>203).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:209
> +        console.log("click", record);

Oops.

> Source/WebInspectorUI/UserInterface/Views/StackedColumnChart.js:87
> +    addColumnSet(x, totalHeight, width, heights, additionalClass)

It's moments like these that I wish we used `optional = {}` in more places :(

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:770
> +                let selectionPadding = 2.25 * WI.CPUTimelineOverviewGraph.samplingRatePerSecond;

NIT: I'd flip the order of the multiplication so that the "base" is first and the "multiplier" is second (e.g. "multiply <known value> by <scale>" rather than "multiply <scale> by <known value>").

[Joseph Pecoraro]

Comment on attachment 363619 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:85
>> +    stroke: var(--selected-background-color-active) !important;
> 
> Is there any way to avoid the `!important`? :(

I was matching what we did for timeline record bars:

    .timeline-record-bar.selected > .segment {
        background-color: var(--selected-background-color) !important;
        border-color: var(--selected-background-color-active) !important;
    }

Normally "selected" appearances are the rare cases we use important to ensure regardless of specificity we get selected styles.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:54
>> +        this._lastSelectedRecordLayout = null;
> 
> Why is "Layout" part of the name?  It seems unnecessary.

It is the last selected record we performed a layout with. Selected record may change many times, but we only need to update our layout if it doesn't match the one when we last did a layout.

I'll rename this to:

    this._lastSelectedRecordInLayout

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:119
>> +        let visibleRecords = this._cpuTimeline.recordsInTimeRange(graphStartTime, visibleEndTime + (WI.CPUTimelineOverviewGraph.samplingRatePerSecond / 2), includeRecordBeforeStart);
> 
> I thought you preferred using the non-`WI` name when referenced from within the class?

Oops, yes I meant to go back and fix this after the global replace I did.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:150
>> +            this.needsLayout();
> 
> It might be worth mentioning that this is necessary since we don't actually have any of the actual DOM nodes for the records.  As such, the only way to add a "selected" class to a matching record is by redrawing the entire graph.

Added a comment.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:180
>> +            rectElement = elements.find((x) => x.localName === "rect");
> 
> Do we still need this now that <https://webkit.org/b/195318> has landed?

Hmm, yes and no. When there is no scrollbar visible, clicks will work everywhere now. But if there is a scroll bar visible then there might be a bar underneath the scrollbar (an overlay) which cannot be clicked directly. That said, I don't think scrollbars are common here, and normally I think people will want to click large columns, not near zero columns (which are already hard to click at 4px).

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.js:199
>> +        let clickPosition = this._graphPositionForMouseEvent(event);
> 
> Shouldn't this be called `graphPosition` to match the function name?  If not, we should rename it to `_clickPositionForMouseEvent` or something like that.

I'll just name this `position`. It is the click position within the graph.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/242567/webkit

[Radar WebKit Bug Importer]

<rdar://problem/48653647>