Bug 195151 - Web Inspector: CPU Usage Timeline - Energy Impact Section
Summary: Web Inspector: CPU Usage Timeline - Energy Impact Section
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 194455
  Show dependency treegraph
 
Reported: 2019-02-28 00:28 PST by Joseph Pecoraro
Modified: 2019-03-03 16:12 PST (History)
5 users (show)

See Also:


Attachments
[IMAGE] Dark Mode (945.44 KB, image/png)
2019-02-28 00:28 PST, Joseph Pecoraro
no flags Details
[IMAGE] Dark Mode - Empty (826.71 KB, image/png)
2019-02-28 00:29 PST, Joseph Pecoraro
no flags Details
[IMAGE] Dark Mode - Popover (979.12 KB, image/png)
2019-02-28 00:29 PST, Joseph Pecoraro
no flags Details
[IMAGE] Dark Mode - RTL (905.82 KB, image/png)
2019-02-28 00:29 PST, Joseph Pecoraro
no flags Details
[IMAGE] Light Mode (928.46 KB, image/png)
2019-02-28 00:29 PST, Joseph Pecoraro
no flags Details
[IMAGE] Light Mode - Empty (814.21 KB, image/png)
2019-02-28 00:29 PST, Joseph Pecoraro
no flags Details
[IMAGE] Light Mode - Popover (963.90 KB, image/png)
2019-02-28 00:29 PST, Joseph Pecoraro
no flags Details
[IMAGE] Light Mode - RTL (888.42 KB, image/png)
2019-02-28 00:30 PST, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (37.71 KB, patch)
2019-02-28 00:41 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (37.72 KB, patch)
2019-02-28 16:49 PST, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff
[Image] Proposed empty state colors (14.52 KB, image/png)
2019-03-01 11:56 PST, Nikita Vasilyev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-02-28 00:28:11 PST
CPU Usage - Energy Impact Section

- Provide a score for the CPU utilization of a page
Comment 1 Joseph Pecoraro 2019-02-28 00:28:53 PST
Created attachment 363200 [details]
[IMAGE] Dark Mode
Comment 2 Joseph Pecoraro 2019-02-28 00:29:04 PST
Created attachment 363201 [details]
[IMAGE] Dark Mode - Empty
Comment 3 Joseph Pecoraro 2019-02-28 00:29:13 PST
Created attachment 363202 [details]
[IMAGE] Dark Mode - Popover
Comment 4 Joseph Pecoraro 2019-02-28 00:29:26 PST
Created attachment 363203 [details]
[IMAGE] Dark Mode - RTL
Comment 5 Joseph Pecoraro 2019-02-28 00:29:41 PST
Created attachment 363204 [details]
[IMAGE] Light Mode
Comment 6 Joseph Pecoraro 2019-02-28 00:29:50 PST
Created attachment 363205 [details]
[IMAGE] Light Mode - Empty
Comment 7 Joseph Pecoraro 2019-02-28 00:29:59 PST
Created attachment 363206 [details]
[IMAGE] Light Mode - Popover
Comment 8 Joseph Pecoraro 2019-02-28 00:30:09 PST
Created attachment 363207 [details]
[IMAGE] Light Mode - RTL
Comment 9 Joseph Pecoraro 2019-02-28 00:41:27 PST
Created attachment 363210 [details]
[PATCH] Proposed Fix

We may want to discuss the low/medium/high range heuristics, but the patch is quite reviewable.
Comment 10 Joseph Pecoraro 2019-02-28 00:47:20 PST Comment hidden (obsolete)
Comment 11 Nikita Vasilyev 2019-02-28 12:58:40 PST
The patch doesn't apply to ToT. Could you rebaseline or let me know what other patch(-es) needs to be applied first?
Comment 12 Joseph Pecoraro 2019-02-28 16:49:39 PST
Created attachment 363277 [details]
[PATCH] Proposed Fix
Comment 13 Joseph Pecoraro 2019-02-28 16:54:33 PST
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.
Comment 14 Nikita Vasilyev 2019-03-01 11:28:54 PST
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`.
Comment 15 Nikita Vasilyev 2019-03-01 11:55:13 PST
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.
Comment 16 Nikita Vasilyev 2019-03-01 11:56:06 PST
Created attachment 363350 [details]
[Image] Proposed empty state colors
Comment 17 Joseph Pecoraro 2019-03-01 12:32:48 PST
> > 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!
Comment 18 Nikita Vasilyev 2019-03-01 12:38:42 PST
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.
Comment 19 Devin Rousso 2019-03-01 15:50:49 PST
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`.
Comment 20 Joseph Pecoraro 2019-03-01 17:11:09 PST
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.
Comment 21 Joseph Pecoraro 2019-03-01 17:19:26 PST
<https://trac.webkit.org/r242300>
Comment 22 Radar WebKit Bug Importer 2019-03-01 17:21:21 PST
<rdar://problem/48529361>
Comment 23 Nikita Vasilyev 2019-03-03 16:12:49 PST
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.