[webkit-reviews] review canceled: [Bug 195390] Web Inspector: CPU Usage Timeline - Add legend and graph hover effects : [Attachment 363822] [PATCH] Proposed Fix

Mon Mar 11 00:51:57 PDT 2019

Joseph Pecoraro <joepeck at webkit.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195390: Web Inspector: CPU Usage Timeline - Add legend and graph hover
effects
https://bugs.webkit.org/show_bug.cgi?id=195390

Attachment 363822: [PATCH] Proposed Fix

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

--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 363822
  --> https://bugs.webkit.org/attachment.cgi?id=363822
[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.