Bug 195390 – Web Inspector: CPU Usage Timeline - Add legend and graph hover effects

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

CPU Usage Timeline - Add legend and graph hover effects

These make it easier to understand the data.

[Joseph Pecoraro]

Created attachment 363822 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

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

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1707
> +        this._overlayRecord = record;

An alternative to this approach is to not individually add point markers into the graphs, but instead do a needsLayout() and add them in layout using `this._overlayRecord`. This approach does a smaller layout but I could try switching to the other. Most of this code comes from an era before we used the SVGs to handle graph overlay stuff.

[Joseph Pecoraro]

Created attachment 363824 [details]
[IMAGE] Dark Mode

[Joseph Pecoraro]

Created attachment 363825 [details]
[IMAGE] Light Mode

[Devin Rousso]

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

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

> Source/WebInspectorUI/UserInterface/Views/AreaChart.js:57
> +        this._pointMarkers = [];

Considering that the markers can be any arbitrary point, I don't think it makes sense to prefix this with `point`.  `_pointMarkers` makes me think that the marker will only "apply" to an item present in `_points`, which is not the case.  I think `_markers` is fine.

> Source/WebInspectorUI/UserInterface/Views/AreaChart.js:132
> +                circle.remove();

Rather than remove and then newly create a bunch of <circle>, is it worth trying to reuse some existing elements (and remove the extra ones or create the missing ones)?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:64
> +.timeline-overview-graph.cpu > .stacked-column-chart > svg > rect.total-usage {

I'm assuming that this means that there isn't a <rect> that has a class other than `.total-usage`, `.main-thread-usage`, or `.worker-thread-usage`?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:66
> +    fill: var(--cpu-other-thread-fill-color);
> +    stroke: var(--cpu-other-thread-stroke-color);

NIT: it is odd that we use the class name `.total-usage` but use the variables named `--cpu-other-thread-*-color`.  I'd expect them to match (as they do in every other case).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:129
> -.timeline-view.cpu .legend {
> +.timeline-view.cpu > .content > .overview .legend {

Was this necessary, or was this to make sure we don't footgun?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:149
>  .timeline-view.cpu .legend > .row > .swatch.sample-type-script {

Shouldn't the rest of these be changed to add  `.content > .overview` as well?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:196
> +    fill: var(--cpu-other-thread-fill-color);
> +    stroke: var(--cpu-other-thread-stroke-color);

Ditto (>65).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:-214
> -.timeline-view.cpu .cpu-usage-view.empty {

Was this never used?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:211
> +.timeline-view.cpu .cpu-usage-combined-view .legend-container .swatch.total {

Is this rule actually necessary?  It seems like it wouldn't be overriding anything.  Is this a footgun-preventer?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:212
> +    border: none;

NIT: I normally put `border` after `background-color`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:213
> +    background: none;

NIT: `background-color`?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:216
> +.timeline-view.cpu .cpu-usage-combined-view .legend-container .swatch.other-threads {

Can we move all the `.cpu-usage-combined-view` styles into CPUUsageCombinedView.css?  It is weird that they aren't.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:218
> +    background-color: var(--cpu-other-thread-fill-color);
> +    border: 1px solid var(--cpu-other-thread-stroke-color);

It makes me very happy that they line up so nicely 😊

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:269
> +    pointer-events: none;

NIT: I normally put properties like these last, as they have nothing to do the the style and are instead about interaction.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:275
> +    border: none;

Considering how high a spike in the CPU timeline can get to the header of the overview, I actually find this to be pretty helpful.  I think we should keep it.
If anything, using `border: none;` relies heavily on the current implementation of the `.timestamp`, so I think we should just use `display: none;` to avoid any future changes breaking this.

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

NIT: adding "Mouse" is unnecessary in my mind, as "click" already implies that.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:447
> +        this._secondsPerPixelInLayout = secondsPerPixel;

Is it necessary that we save this value rather than just get it from `this._timelineRuler.secondsPerPixel`?  Are you worried about it getting out of sync?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:626
>          let layoutMax = max;

Could we replace this property with `this._layoutMax` inside this function?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1615
> +        if (isNaN(mousePosition)) {

Is this case ever actually possible?  Wouldn't we just want to clear the overlay, not the scanner, when you click and nothing is found?  If you click on the graph, you still have to be inside the graph area, which means you should still have a scanner (clicking on this "null" point should clear the overlay though).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1616
> +            this.dispatchEventToListeners(WI.TimelineView.Event.ScannerDidClear);

This event doesn't exist :(

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1626
>      _handleGraphMouseMove(event)

We should still show the overlay if you move your mouse from the combined "CPU Usage" area to the "Threads" section header to the "Main Thread" section.  When the cursor is over the "Threads" area, the overlay disappears (because it can't find a chart element using `closest`).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1656
> +                bestTime = record.timestamp;

Can't this be calculated at the end, once you have a `nearestRecord` that you know isn't falsy (e.g. after >1664)?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1679
> +        let secondsPerPixel = this._timelineRuler.secondsPerPixel;

Should this be `this._secondsPerPixelInLayout`?  (assuming you don't change it as per my comment >447).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1682
> +        let visibleEndTime = Math.min(this.endTime, this.currentTime);

NIT: use `graphEndTime` to avoid a second call.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1690
> +        let adjustedTime = bestTime;
> +        if (adjustedTime < graphStartTime)
> +            adjustedTime = graphStartTime;
> +        else if (adjustedTime > visibleEndTime)
> +            adjustedTime = visibleEndTime;
> +
> +        this._showGraphOverlay(nearestRecord, adjustedTime);

You could inline this quite nicely.
```
    this._showGraphOverlay(nearestRecord, Number.constrain(bestTime, graphStartTime, visibleEndTime));
```

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1698
> +        const force = true;

I typically only use `const` for arguments when I'd have to go to another file to see the definition of the function.  Since it's defined in the same file (and as literally the next thing in the file), I'd just inline this (or make it an optional object `{force: true}`).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1699
> +        this._showGraphOverlay(this._overlayRecord, this._overlayTime, force);

Rather than having another parameter for this, you could save and reassign to `this._overlayRecord` to achieve the same effect.
```
    let record = this._overlayRecord;
    this._overlayRecord = null;
    this._showGraphOverlay(record, this._overlayTime);
```

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1710
> +        let layoutMax = this._layoutMax;

Considering this isn't used anywhere else, I'd just line `this._layoutMax`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1754
> +            let unseenWorkersViews = new Set(this._workerViews.values());

Please use `unseenWorkerViews`, which matches the below `workerView` (and sounds/reads nicer).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1768
> +            for (let workerView of unseenWorkersViews)
> +                workerView.updateLegend(NaN);

I feel like this is potentially more work than just iterating over `this._workerViews.values()` and calling `workerView.updateLegend(NaN)` on each.  Using a `Set` means that you have to do all the unique-ness calculation when initializing it, as well as when you potentially re-delete a view while iterating `workersData`.  I'd rather you just iterate them all before you update the ones that actually do have data.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1785
> +        for (let workerView of this._workerViews)

Should this be `this._workerViews.values()`?

> Source/WebInspectorUI/UserInterface/Views/CPUUsageCombinedView.css:36
> +.cpu-usage-combined-view > .graph > .stacked-area-chart > svg,
> +.cpu-usage-combined-view > .graph > .range-chart > svg {

`:matches(.stacked-area-chart, .range-chart) > svg`

> Source/WebInspectorUI/UserInterface/Views/CPUUsageCombinedView.css:48
> +    z-index: calc(var(--timeline-marker-z-index) + 1);

NIT: I normally put `z-index` alongside `position`, as it is related/controlled by that property.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageCombinedView.css:80
> +body[dir=rtl] .cpu-usage-combined-view > .graph {

NIT: I'd put this below the next rule, as it has a greater specificity.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageCombinedView.js:52
> +        this._chart = new WI.StackedAreaChart;

This could have a better name, since you have a  `_chart` and `_rangeChart`.  How about `_threadChart` or `_usageChart`, or even just `_stackedAreaChart` (to match the "bluntness" of `_rangeChart`)?

> Source/WebInspectorUI/UserInterface/Views/CPUUsageCombinedView.js:112
> +        console.assert(min <= max);

This assertion is subsumed by the transitivity of the next assertion.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageCombinedView.js:213
> +        this._legendMainThreadElement.textContent = WI.UIString("Main Thread");
> +        this._legendWorkerThreadsElement.textContent = WI.UIString("Worker Threads");
> +        this._legendOtherThreadsElement.textContent = WI.UIString("Other Threads");
> +        this._legendTotalThreadsElement.textContent = "";

I really don't like the idea of replacing text with a number.  If anything, I think the idea of "threads" is easily deduced when looking at this view, so I'd rather have an emdash (as we do with most other legend-esque values) that get's replaced with a number.  I'd prefer if we always kept the "Total:" text visible, but I don't feel as strongly about that (mainly because it serves no purpose when all the values that it's supposed to be the sum of are emdashes), although it is a little jarring to have "Total:" flash in/out of existence as I move my cursor between graphs.

> Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.js:-47
> -        this._updateDetails(NaN, NaN);

Do we need this anymore?

> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:118
> +        this._detailsUsageElement.hidden = true;

Ditto (>CPUUsageCombinedView.js:210).

> Source/WebInspectorUI/UserInterface/Views/StackedAreaChart.js:64
> +        this._pointMarkers = [];

Ditto (>AreaChart.js:57).

> Source/WebInspectorUI/UserInterface/Views/StackedAreaChart.js:162
> +                circle.remove();

Ditto (>AreaChart.js:132).

> Source/WebInspectorUI/UserInterface/Views/Variables.css:145
> +    --cpu-overlay-color: var(--cpu-main-thread-stroke-color);

I think we should match the color of the timeline scanner.  This color blends in with `--cpu-fill-color` way too easily.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:307
> +        --cpu-overlay-color: hsl(36, 98%, 50%);

This color is just painful to look at :(

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/AreaChart.js:132
>> +                circle.remove();
> 
> Rather than remove and then newly create a bunch of <circle>, is it worth trying to reuse some existing elements (and remove the extra ones or create the missing ones)?

In nearly all of my SVG tests, just deleting and adding new elements was pretty much always faster. Given in this case there are only a handful at most it seems fine.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:64
>> +.timeline-overview-graph.cpu > .stacked-column-chart > svg > rect.total-usage {
> 
> I'm assuming that this means that there isn't a <rect> that has a class other than `.total-usage`, `.main-thread-usage`, or `.worker-thread-usage`?

In the stacked-column-chart those are the only sections:

    this._chart = new WI.StackedColumnChart(size);
    this._chart.initializeSections(["main-thread-usage", "worker-thread-usage", "total-usage"]);

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:129
>> +.timeline-view.cpu > .content > .overview .legend {
> 
> Was this necessary, or was this to make sure we don't footgun?

It was necessary when I was testing something named just .legend in CPUUsageViews. In this patch it ended up being .legend-container.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:149
>>  .timeline-view.cpu .legend > .row > .swatch.sample-type-script {
> 
> Shouldn't the rest of these be changed to add  `.content > .overview` as well?

Sure, I rolled this down the line.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:-214
>> -.timeline-view.cpu .cpu-usage-view.empty {
> 
> Was this never used?

Correct it was not used. I may bring it back, for Linux ports I believe we will want to do something about improving / hiding sections when they only send a single thread's data.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:211
>> +.timeline-view.cpu .cpu-usage-combined-view .legend-container .swatch.total {
> 
> Is this rule actually necessary?  It seems like it wouldn't be overriding anything.  Is this a footgun-preventer?

This is just for clarity. It is not necessary.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:216
>> +.timeline-view.cpu .cpu-usage-combined-view .legend-container .swatch.other-threads {
> 
> Can we move all the `.cpu-usage-combined-view` styles into CPUUsageCombinedView.css?  It is weird that they aren't.

Done. We've done this elsewhere where colors are all put in a single place to avoid spreading them around, but I agree here it doesn't matter much.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:447
>> +        this._secondsPerPixelInLayout = secondsPerPixel;
> 
> Is it necessary that we save this value rather than just get it from `this._timelineRuler.secondsPerPixel`?  Are you worried about it getting out of sync?

It is necessary. The TimelineRuler updates its layout whenever the window resizes, the Charts do not. So when placing markers into the charts we need to use the same secondsPerPixel that the charts laid out with, instead the charts stretch. If we used the new secondsPerPixel from the ruler the positions would be incorrect.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:626
>>          let layoutMax = max;
> 
> Could we replace this property with `this._layoutMax` inside this function?

This code gets eliminated in the patch following this, which sets a different layoutMax in different sections. So I'll remove it in that.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1615
>> +        if (isNaN(mousePosition)) {
> 
> Is this case ever actually possible?  Wouldn't we just want to clear the overlay, not the scanner, when you click and nothing is found?  If you click on the graph, you still have to be inside the graph area, which means you should still have a scanner (clicking on this "null" point should clear the overlay though).

Yes, this is code that probably didn't merge well since ScannerDidClear doesn't exist anymore anyways. I've removed it.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1626
>>      _handleGraphMouseMove(event)
> 
> We should still show the overlay if you move your mouse from the combined "CPU Usage" area to the "Threads" section header to the "Main Thread" section.  When the cursor is over the "Threads" area, the overlay disappears (because it can't find a chart element using `closest`).

That can be a follow-up. I imagine most people will collapse the Threads section (that is the default) and I haven't found it annoying in real use.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1656
>> +                bestTime = record.timestamp;
> 
> Can't this be calculated at the end, once you have a `nearestRecord` that you know isn't falsy (e.g. after >1664)?

Sure

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1679
>> +        let secondsPerPixel = this._timelineRuler.secondsPerPixel;
> 
> Should this be `this._secondsPerPixelInLayout`?  (assuming you don't change it as per my comment >447).

`secondsPerPixel` and `graphEndTime` are not actually used here. I factored out the code below and forgot to remove it!

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1710
>> +        let layoutMax = this._layoutMax;
> 
> Considering this isn't used anywhere else, I'd just line `this._layoutMax`.

All the layoutMax code changes in the next patch.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1768
>> +                workerView.updateLegend(NaN);
> 
> I feel like this is potentially more work than just iterating over `this._workerViews.values()` and calling `workerView.updateLegend(NaN)` on each.  Using a `Set` means that you have to do all the unique-ness calculation when initializing it, as well as when you potentially re-delete a view while iterating `workersData`.  I'd rather you just iterate them all before you update the ones that actually do have data.

Sounds good.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1785
>> +        for (let workerView of this._workerViews)
> 
> Should this be `this._workerViews.values()`?

It is just an array, the .values() above was unnecessary.

>> Source/WebInspectorUI/UserInterface/Views/CPUUsageStackedView.js:-47
>> -        this._updateDetails(NaN, NaN);
> 
> Do we need this anymore?

I added it back to be safe, but in practice the first layout calls either updateChart or clear which would set the values.

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:145
>> +    --cpu-overlay-color: var(--cpu-main-thread-stroke-color);
> 
> I think we should match the color of the timeline scanner.  This color blends in with `--cpu-fill-color` way too easily.

The purpose of this is to exactly match the graph's color. It enhances the snap to point experience because it matches the stroke on the graph.  In dark mode the color was just too dark and had to be different.

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:307
>> +        --cpu-overlay-color: hsl(36, 98%, 50%);
> 
> This color is just painful to look at :(

Hmm, lets work on replacing it. I don't know what you mean by painful.

[Joseph Pecoraro]

Created attachment 364238 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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

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

r=me, awesome work!  Thanks for iterating :)

> Source/WebInspectorUI/UserInterface/Views/AreaChart.js:90
> +    addPointMarker(x, y)

NIT: I don't know why I didn't mention this earlier, but I feel like this could be shortened to just `addMarker`.

> Source/WebInspectorUI/UserInterface/Views/AreaChart.js:95
> +    clearPointMarkers()

Ditto (>90), but with `clearMarkers`.

[Joseph Pecoraro]

> > Source/WebInspectorUI/UserInterface/Views/AreaChart.js:90
> > +    addPointMarker(x, y)
> 
> NIT: I don't know why I didn't mention this earlier, but I feel like this
> could be shortened to just `addMarker`.

I'm going to keep it "Point Markers" for now since this draws a <circle> at the given (x, y) point. Whereas other places in Web Inspector use "marker" to mean a line at a specific time / x.

[Joseph Pecoraro]

https://trac.webkit.org/r242739

[Radar WebKit Bug Importer]

<rdar://problem/48782854>