[webkit-reviews] review granted: [Bug 194788] Web Inspector: CPU Usage Timeline - Thread Breakdown : [Attachment 362958] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 194788: Web Inspector: CPU Usage Timeline - Thread Breakdown
https://bugs.webkit.org/show_bug.cgi?id=194788

Attachment 362958: [PATCH] Proposed Fix

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




--- Comment #29 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 362958
  --> https://bugs.webkit.org/attachment.cgi?id=362958
[PATCH] Proposed Fix

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:143
> +	   function appendLegendRow(legendElement, sampleType, label) {

NIT: `label` isn't used.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:159
> +

Style: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:190
> +	   // Cause the TimelineRuler to layout now so we will have some of its
> +	   // important properties initialized for our layout.
> +	   this._timelineRuler.updateLayout(WI.View.LayoutReason.Resize);

This is an interesting case, because `didLayoutSubtree` would be a solution to
this, but because we modify some of the subviews, we'd effectively cause two
layouts for each one.  I suppose it's better to have a single view do two
layouts initially rather than have most (if not all) subviews do two layouts
per layout.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:219
> +	   this._workerViews = [];

Should this be defined (maybe just as `null`) in the constructor?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:234
> +	   if (!secondsPerPixel)
> +	       return;

Given that you force a layout above, is it ever possible to have this happen? 
My comment in the previous patch was more about moving the assertion, not
removing it entirely.  I do agree that this shouldn't be reachable.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:253
> +	   if (!nonIdleSamplesCount) {

Style: since we already have an `else`, you should switch the order of these
(the "meaty" logic is inside the current `else` anyways, so bringing it closer
to the data source may be easier to read).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:266
> +	       this._breakdownLegendStyleElement.textContent =
`${Number.percentageString(percentStyle)} (${samplingData.samplesStyle})`;

Are parenthesis localizable? I've had issues with "%" in the past, so we should
probably use a `UIString` here just in case.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:393
> +	   for (let [workerId, workerData] of workersDataMap)

NIT: you can use `workersDataMap.values()` and avoid the array destructure.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:424
> +		   let even = (hundreds % 2) === 0;

NIT: you could inline this, as well as reversing the `if` so you can drop the
`=== 0`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:441
> +	   function layoutView(view, property, graphHeight, {dataPoints, min,
max, average}) {

Since `max` and `min` are already defined above, we should use a different name
or use an object accessor (e.g. not destructure) to avoid shadowing "issues".

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:449
> +	       let isAllThreadsGraph = property === null;

Style: `!property`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:476
> +		   const minimumMarkerTextHeight = 17;

Should this be kept in sync with some CSS?  How was this number achieved?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:495
> +		   labelElement.classList.add("label");
> +		   const precision = 0;

Style: missing newline.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:516
> +	       layoutView(workerView, "usage",
CPUTimelineView.threadCPUUsageViewHeight, {dataPoints: workerData.dataPoints,
min: workerData.min, max: workerData.max, average: workerData.average});

Can we just pass `workerData` rather than try to remove just a few pieces of
data, especially since `layoutView` is also defined in this same file/function,
or are you worried that the keys/names might change and break things?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:621
> +	   let samplesIdle = 0;

Rather than prefixing each of these with `samples`, how about flipping the name
and adding a `Count` suffix (e.g. `idleCount`)?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:626
> +	   for (let i = 0; i < samples.length; ++i) {

Style: `for (let sample of samples)`

> Source/WebInspectorUI/UserInterface/Views/View.js:134
> +    removeUnparentedSubview(view)

This should be removed.