[webkit-reviews] review granted: [Bug 195151] Web Inspector: CPU Usage Timeline - Energy Impact Section : [Attachment 363277] [PATCH] Proposed Fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 1 15:50:49 PST 2019
Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195151: Web Inspector: CPU Usage Timeline - Energy Impact Section
https://bugs.webkit.org/show_bug.cgi?id=195151
Attachment 363277: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=363277&action=review
--- Comment #19 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 363277
--> https://bugs.webkit.org/attachment.cgi?id=363277
[PATCH] Proposed Fix
View in context: https://bugs.webkit.org/attachment.cgi?id=363277&action=review
r=me, looks awesome!
> Source/WebInspectorUI/UserInterface/Main.html:113
> + <link rel="stylesheet" href="Views/GaugeChart.css">
This should be placed at the top of the "G" section.
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:43
> +.timeline-view.cpu > .content .subtitle > .info {
NIT: I'd reorder much of this.
display: inline-block;
width: 15px;
height: 15px;
-webkit-margin-start: 7px;
color: white;
font-size: 12px;
background-color: darkgray;
border-radius: 50%;
This way, the styles follow from things affecting "visibility" to "size" to
"position" to "content" to "background" to "border" to "etc" :)
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:62
> + static get lowEnergyValue() { return 3; }
> + static get highEnergyValue() { return 50; }
For the sake of matching the various gauges, perhaps we should also have a
`mediumEnergyThreshold `.
static get lowEnergyThreshold() { return 3; }
static get mediumEnergyThreshold() { return 50; }
static get highEnergyThreshold() { return 150; }
That way, we have all the numbers in one place and can configure them all here.
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:649
> + console.assert(value >= rangeLow && value <= rangeHigh, "value
was not in range.");
NIT: I find that logging the value being tested in the assert significantly
helps in future debugging.
console.assert(value >= rangeLow && value <= rangeHigh, "value was not in
range.", value);
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:656
> + function valuesForGauge(value) {
It seems to me like this function doesn't really add much. It's not strictly
necessary that you return these objects vs having a bunch of `if else`s.
this._clearEnergyImpactText();
if (value === 0) {
// Zero. (0% CPU, mapped to 0)
this._energyImpactLabelElement.textContent = WI.UIString("Low");
this._energyImpactLabelElement.classList.add("low");
this._energyChart.value = 0;
} else if (value <= CPUTimelineView.lowEnergyThreshold) {
// Low. (<=3% CPU, mapped to 0-10)
this._energyImpactLabelElement.textContent = WI.UIString("Low");
this._energyImpactLabelElement.classList.add("low");
this._energyChart.value = mapWithBias(value, 0,
CPUTimelineView.lowEnergyThreshold, 0, 10, 0.85);
} else if (value <= CPUTimelineView. mediumEnergyThreshold) {
// Medium (3%-90% CPU, mapped to 10-80)
this._energyImpactLabelElement.textContent = WI.UIString("Medium");
this._energyImpactLabelElement.classList.add("medium");
this._energyChart.value = mapWithBias(value,
CPUTimelineView.lowEnergyThreshold, CPUTimelineView.mediumEnergyThreshold, 10,
80, 0.6);
} else if (value < CPUTimelineView. highEnergyThreshold) {
// High. (50-150% CPU, mapped to 80-100)
this._energyImpactLabelElement.textContent = WI.UIString("High");
this._energyImpactLabelElement.classList.add("high");
this._energyChart.value = mapWithBias(value,
CPUTimelineView.mediumEnergyThreshold, CPUTimelineView.highEnergyThreshold, 80,
100, 0.9);
} else {
// Very High. (>150% CPU, mapped to 100)
this._energyImpactLabelElement.textContent = WI.UIString("High");
this._energyImpactLabelElement.classList.add("high");
this._energyChart.value = 100;
}
this._energyChart.needsLayout();
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:659
> + return {label: WI.UIString("Low"), className: undefined,
value: 0};
NIT: remove the `className: undefined` since it's not doing anything anyways.
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:667
> + return {label: WI.UIString("Low"), className: "low", value:
mapWithBias(value, 0, 4, 0, 10, 0.85)};
Should this be `3` (not `4`)?
>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:674
>> + return {label: WI.UIString("Medium"), className: "medium",
value: mapWithBias(value, 3, 90, 10, 80, 0.6)};
>
> I'd prefer using ascending order here:
> Zero
> Low
> Medium
> High
> Very High
I agree. Rather than having the "common" case be the one that we have last, I
think this would be more efficient (and read nicer) if we ordered it in
increasing values.
value === 0
value <= CPUTimelineView.lowEnergyValue
value < CPUTimelineView.highEnergyValue
value < 150
<leftover return>
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:677
> + let values = valuesForGauge(average);
How about `gaugeData` or `gaugeConfig`?
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:682
> + this._energyImpactLabelElement.classList.remove("low", "medium",
"high");
You could just call `_clearEnergyImpactText` and keep the logic more
centralized.
> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:684
> +
this._energyImpactLabelElement.classList.toggle(values.className, true);
Why not just use `add`?
this._energyImpactLabelElement.classList.add(values.className);
>> Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:39
>> + transition: all 0.2s;
>
> This looks nice!
Is it necessary that you use `all`? Does `transition: transform 0.2s ease` not
work?
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:44
> + fill: gray;
> + stroke: gray;
Rather than having these styles be overridden, perhaps we can leverage fallback
CSS variable values instead.
.gauge-chart:not(.empty) > svg > polygon.needle {
fill: var(--gauge-chart-needle-fill, gray);
stroke: var(--gauge-chart-needle-stroke, gray);
}
This way, all of the styles in CPUTimelineView.css don't need to add
`:not(.empty)` :)
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:91
> + set segments(segments)
> + {
> + }
???
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:100
> + console.assert(value >= 0 && value <= 100, "value should be between
0 and 100.");
NIT: I find that logging the value being tested in the assert significantly
helps in future debugging.
console.assert(value >= 0 && value <= 100, "value should be between 0 and
100.", value);
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:119
> + const onePercentAngle = ((1 / 100) * Math.PI);
`Math.PI / 100`?
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:122
> + let offset = limit === 100 ? 0 : 1;
Alternatively, you could just subtract `onePercentAngle` from `endAngle` if
`limit !== 100`.
let endAngle = Math.PI + Math.percentOfPI(limit);
if (limit !== 100)
endAngle -= onePercentAngle;
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:123
> + let endAngle = Math.PI + (((limit - offset) / 100) * Math.PI);
You could create some sort of private/Utilties.js function like `percentOfPI`.
Object.defineProperty(Math, "percentOfPI",
{
value(percent)
{
return Math.PI * (percent / 100);
}
});
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:156
> + let percent = (value / 100);
NIT: I'd just inline this, as that would be clearer enough where I don't think
you'd need a comment.
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:158
> + this._needleElement.style.transform = "none";
Why is this needed?
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:170
> + console.assert(limit <= 100, "limit should be between 1 and
100", limit);
NIT: you could combine these as a single `limit >= 1 && limit <= 100`, since
you're logging the value with the assertion anyways (e.g. I can therefore see
which "side" it's falling off of).
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:183
> + const onePercentArc = ((1 / 100) * Math.PI);
Ditto (>119).
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:184
> + const r3 = (r2 - startIndicatorUnderhang);
> +
> + const onePercentArc = ((1 / 100) * Math.PI);
> + const largeArcFlag = ((a2 - a1) % (Math.PI * 2)) > Math.PI ? 1 : 0;
Style: these should all be `let` as they can change depending on what's passed
in.
> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:187
> + y1 = c + Math.sin(a1) * r1,
Style: there should be a `let` for each of these (and an ending `;`).
> Source/WebInspectorUI/UserInterface/Views/Variables.css:145
> + --cpu-low-fill-color: rgb(105, 201, 86);
These should all be `hsl` colors.
>> Source/WebInspectorUI/UserInterface/Views/Variables.css:150
>> + --cpu-high-stroke-color: rgb(232, 77, 61);
>
> Even if you end up using stroke color that matches `fill`, I'd rather define
only one variable instead of two.
I disagree. I think the verbosity is better both for readability and future
changes/customization. With that having been said, if it turns out that he
gets rid of the stroke entirely (e.g. just uses a fill, which should be similar
(if not the same) to what's already there), then I think we should rename it to
something a bit less specific, like `--cpu-high-color`.
More information about the webkit-reviews
mailing list