Bug 195151 – Web Inspector: CPU Usage Timeline - Energy Impact Section

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

CPU Usage - Energy Impact Section

- Provide a score for the CPU utilization of a page

[Joseph Pecoraro]

Created attachment 363200 [details]
[IMAGE] Dark Mode

[Joseph Pecoraro]

Created attachment 363201 [details]
[IMAGE] Dark Mode - Empty

[Joseph Pecoraro]

Created attachment 363202 [details]
[IMAGE] Dark Mode - Popover

[Joseph Pecoraro]

Created attachment 363203 [details]
[IMAGE] Dark Mode - RTL

[Joseph Pecoraro]

Created attachment 363204 [details]
[IMAGE] Light Mode

[Joseph Pecoraro]

Created attachment 363205 [details]
[IMAGE] Light Mode - Empty

[Joseph Pecoraro]

Created attachment 363206 [details]
[IMAGE] Light Mode - Popover

[Joseph Pecoraro]

Created attachment 363207 [details]
[IMAGE] Light Mode - RTL

[Joseph Pecoraro]

Created attachment 363210 [details]
[PATCH] Proposed Fix

We may want to discuss the low/medium/high range heuristics, but the patch is quite reviewable.

[Joseph Pecoraro]

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

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

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:239
> +                {className: "high", limit: 100},            

Oops some trailing whitespace. Fixed locally.

[Nikita Vasilyev]

The patch doesn't apply to ToT. Could you rebaseline or let me know what other patch(-es) needs to be applied first?

[Joseph Pecoraro]

Created attachment 363277 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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

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

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:28
> +// Initialize the chart with a semi-circle height,  stroke width, and segments.

Nit: Double space has been fixed locally.

[Nikita Vasilyev]

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

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

I'll add more comment in 45-60 minutes.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:46
> +    color: white;
> +    background-color: darkgray;

I looked at the CSS variables we could use, but --text-color-* and --background-color-* didn't quite fit here. I think it's fine to leave what you have now.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:54
> +@media (prefers-color-scheme: dark) {

Everywhere else, media queries (such as prefers-color-scheme) are at the bottom of the CSS files.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:282
> +    stroke: var(--cpu-low-stroke-color);

`stroke` is unnecessary here. --cpu-low-stroke-color is the same as --cpu-low-fill-color.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:287
> +    stroke: var(--cpu-medium-stroke-color);

I don't think we need to use `stroke` here. It looks clear without the stroke.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:292
> +    stroke: var(--cpu-high-stroke-color);

`stroke` is unnecessary here. --cpu-high-stroke-color is the same as --cpu-high-fill-color.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:297
> +    stroke: hsla(0, 0%, 36%, 0.85);

`stroke` matches `fill`? Why have stroke at all?

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:45
> +    stroke-width: 1;

1. `stroke-width: 1` is already defined for `.gauge-chart > svg > polygon`
2. I think it should be `stroke-widht: 0`.

[Nikita Vasilyev]

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

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

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:37
> +    stroke: hsla(0, 0%, var(--foreground-lightness), 0.1);

I see; this is `stroke` for the empty state.

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:39
> +    transition: all 0.2s;

This looks nice!

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:50
> +    fill: lightgray;
> +    stroke: hsla(0, 0%, var(--foreground-lightness), 0.1);

The needle is overlaying segment polygons. For the empty state, it would be better to use the same solid fill color for both needle and segment polygons. See the attached image.

[Nikita Vasilyev]

Created attachment 363350 [details]
[Image] Proposed empty state colors

[Joseph Pecoraro]

> > Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:50
> > +    fill: lightgray;
> > +    stroke: hsla(0, 0%, var(--foreground-lightness), 0.1);
> 
> The needle is overlaying segment polygons. For the empty state, it would be
> better to use the same solid fill color for both needle and segment
> polygons. See the attached image.

Good idea!

[Nikita Vasilyev]

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

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

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:674
> +            // Zero. (0% CPU, mapped to 0)
> +            if (value === 0)
> +                return {label: WI.UIString("Low"), className: undefined, value: 0};
> +
> +            // Very High. (>150% CPU, mapped to 100)
> +            if (value >= 150)
> +                return {label: WI.UIString("Very High"), className: "high", value: 100};
> +
> +            // Low. (<=3% CPU, mapped to 0-10)
> +            if (value <= CPUTimelineView.lowEnergyValue)
> +                return {label: WI.UIString("Low"), className: "low", value: mapWithBias(value, 0, 4, 0, 10, 0.85)};
> +
> +            // High. (50-150% CPU, mapped to 80-100)
> +            if (value >= CPUTimelineView.highEnergyValue)
> +                return {label: WI.UIString("High"), className: "high", value: mapWithBias(value, 50, 150, 80, 100, 0.9)};
> +
> +            // Medium (3%-90% CPU, mapped to 10-80)
> +            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

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:26
> +// GuageChart creates a semi-circle gauge chart with colored segments.

Spelling: Gauge

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:118
> +        const pathStartIndicatorUnderhang = 4;

Unused.

> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:141
> +        this._needleElement.setAttribute("points", `0,${midY + needlePointExtraDraw}, 0,${midY - needlePointExtraDraw} ${midX + needleUnderhangDraw},${midY - needleBaseExtraDraw} ${midX + needleUnderhangDraw},${midY + needleBaseExtraDraw}`);

This is fine for now, but it would be nice to wrap this into an API that takes numbers, not a string.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:150
> +    --cpu-low-fill-color: rgb(105, 201, 86);
> +    --cpu-low-stroke-color: rgb(105, 201, 86);
> +    --cpu-medium-fill-color: rgb(246, 205, 70);
> +    --cpu-medium-stroke-color: rgb(246, 205, 70);
> +    --cpu-high-fill-color: rgb(232, 77, 61);
> +    --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.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:299
> +        --cpu-low-fill-color: rgb(113, 210, 94);
> +        --cpu-low-stroke-color: rgb(113, 210, 94);
> +        --cpu-medium-fill-color: rgb(248, 214, 74);
> +        --cpu-medium-stroke-color: rgb(248, 214, 74);
> +        --cpu-high-fill-color: rgb(233, 85, 69);
> +        --cpu-high-stroke-color: rgb(233, 85, 69);

Ditto.

[Devin Rousso]

Comment on attachment 363277 [details]
[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`.

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:54
>> +@media (prefers-color-scheme: dark) {
> 
> Everywhere else, media queries (such as prefers-color-scheme) are at the bottom of the CSS files.

Yeah that seems curiously unnecessary to me, but I'll follow that style for now.

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:297
>> +    stroke: hsla(0, 0%, 36%, 0.85);
> 
> `stroke` matches `fill`? Why have stroke at all?

The empty case has them be different so I believe I need to override default empty style.

>>> 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?

We transition the fill and stroke color as well.

>> Source/WebInspectorUI/UserInterface/Views/GaugeChart.css:50
>> +    stroke: hsla(0, 0%, var(--foreground-lightness), 0.1);
> 
> The needle is overlaying segment polygons. For the empty state, it would be better to use the same solid fill color for both needle and segment polygons. See the attached image.

I still want the needle to stand out even in the empty case but I did tweak this to be lighter than lightgray.

>> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:91
>> +    }
> 
> ???

Hah, was supposed to be removed, I moved this to construction since it should always be known ahead of time in this chart type.

>> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:141
>> +        this._needleElement.setAttribute("points", `0,${midY + needlePointExtraDraw}, 0,${midY - needlePointExtraDraw} ${midX + needleUnderhangDraw},${midY - needleBaseExtraDraw} ${midX + needleUnderhangDraw},${midY + needleBaseExtraDraw}`);
> 
> This is fine for now, but it would be nice to wrap this into an API that takes numbers, not a string.

The ever illusive CSSOM!

>> Source/WebInspectorUI/UserInterface/Views/GaugeChart.js:158
>> +        this._needleElement.style.transform = "none";
> 
> Why is this needed?

Oops, yes this was leftover from when I was trying to get animations to work properly! This is not needed.

>>> 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`.

Oops. I had experimented with a different stroke than the file, but ultimately avoided that. I'll reduce the duplication. I'll still keep a fill and stroke in the UI for sizing, since empty treats them differently.

[Joseph Pecoraro]

<https://trac.webkit.org/r242300>

[Radar WebKit Bug Importer]

<rdar://problem/48529361>

[Nikita Vasilyev]

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

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

>>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:54
>>> +@media (prefers-color-scheme: dark) {
>> 
>> Everywhere else, media queries (such as prefers-color-scheme) are at the bottom of the CSS files.
> 
> Yeah that seems curiously unnecessary to me, but I'll follow that style for now.

I don't have a preference for one style over another but I do want consistency.