1499906 - Sundry aboutMemory.js code improvements

Status: RESOLVED FIXED

(Toolkit :: about:memory, enhancement)

Description

[Nicholas Nethercote [inactive]]

I've been looking at the code and there is room for improvement. Some of this will be due to features that have been added to JS since aboutMemory.js was first written.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Use non-ASCII chars directly in the code

They're easier to read than \uXXXX escapes, and there's no reason to stick to
ASCII in the source code.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Improve treeline handling

This commit reduces the appendTreeElements2's treeline arguments from 3 to 2,
and makes the code easier to understand. It's also more efficient, because a
concatenation that used to be repeated for every child of a node is now done
once for all children.

This commit also shortens treeline variable names, because there are a lot of
them and they're easier to read when short.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Use for..of loops where possible

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Change `var` to `let`

For consistency; `let` is used in most places.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Use toLocaleString()

This replaces a bunch of code that inserted separators by hand.

For now I've kept the output mostly the same by forcing the locale to en-US.
But at least now we could consider localizing the output.

The places where the output is different, it's more consistent with the new
code. E.g. printing "-05.55%" (which matches "05.55%") instead of "-5.55%".

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Use String.prototype.padStart()

This lets us remove pad().

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Put some parameters in `aFoo` form. erahm

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Use template literals where suitable

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 9018166 [details] [diff] [review]
Use non-ASCII chars directly in the code

Review of attachment 9018166 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup! It might be worth checking that this doesn't get weird on Windows (bom markers and whatnot).

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 9018167 [details] [diff] [review]
Improve treeline handling

Review of attachment 9018167 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +1884,5 @@
>        // The 'kids' class is just used for sanity checking in toggle().
>        d = appendElement(aP, "span", showSubtrees ? "kids" : "kids hidden");
>  
> +      let tlThisForMost, tlKidsForMost;
> +      if (aT._kids.length > 1) {

It would be a little clearer to just set these unconditionally instead of leaving them undef'd and hoping that the code below doesn't change, but I get that's a little less efficient so I'll leave it up to you.

@@ +1889,5 @@
> +        tlThisForMost = aTlKids + "├──";
> +        tlKidsForMost = aTlKids + "│  ";
> +      }
> +      let tlThisForLast = aTlKids + "└──";
> +      let tlKidsForLast = aTlKids + "   ";

nit: It would be a little clearer with these two as `const`.

Comment 11

[Eric Rahm [:erahm]]

Comment on attachment 9018169 [details] [diff] [review]
Use for..of loops where possible

Review of attachment 9018169 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup, thanks.

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 9018171 [details] [diff] [review]
Use toLocaleString()

Review of attachment 9018171 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few comments.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +1505,5 @@
>    appendLink("end", "start", "↑");
>  }
>  
> +// Used for UNITS_BYTES values that are printed as MiB.
> +const gMBStyle = {

Should these be `kConst`? I don't have a strong preference either way, just curious if we have a set style now that `const` is a thing.

@@ +1543,3 @@
>   */
> +function formatInt(aN) {
> +  return aN.toLocaleString('en-US');

Whoa, we hand-rolled this? Nice cleanup :)

@@ +1554,5 @@
>   */
>  function formatBytes(aBytes) {
> +  return gVerbose.checked
> +       ? `${formatInt(aBytes)} B`
> +       : `${(aBytes / (1024 * 1024)).toLocaleString('en-US', gMBStyle)} MB`;

Would it make sense to have an optional arg to `formatInt` for the options dict and then we could just use that?

@@ +1586,5 @@
> +  // - We want 4 digits. For positive numbers, 00.00%--99.99% works
> +  //   normally, but 100.0% needs special handling.
> +  let num = aDenom === 0 ? 1 : (aNum / aDenom);
> +  let numText = (num * 100).toFixed(2);
> +  return numText === "100.00" // XXX: en-US specific

Is `toFixed` actually localized? Breaking on non en-US seems like a bad thing. Maybe we should just do:

> return Number.parseInt(num * 10000) == 10000

::: toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
@@ +367,4 @@
>     └──-0.00 MB (50.00%) -- g\n\
>        ├──-0.00 MB (31.82%) ── i [-]\n\
>        ├──-0.00 MB (27.27%) ── h [-]\n\
> +      └───0.00 MB (-09.09%) ── (fake child) [!]\n\

Stuff like this is a little unfortunate (the values are no longer aligned). Would it be too much trouble to special case (-10%,0%) given we also special case 100%? I'm not sure it's worth the effort.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 9018174 [details] [diff] [review]
Use template literals where suitable

Review of attachment 9018174 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for splitting all these up.

Comment 14

[Nicholas Nethercote [inactive]]

> > +const gMBStyle = {
> 
> Should these be `kConst`?

Yep, good catch.

> Whoa, we hand-rolled this? Nice cleanup :)

Yes. Before toLocaleString() was added there was no proper way to insert separators.

> Would it make sense to have an optional arg to `formatInt` for the options
> dict and then we could just use that?

Ok. I've done that for all the places that call toLocaleString(), which means that 'en-US' is specified in a single function now, which is nice. I also renamed `formatInt` as `formatNum`, because it does deal with non-ints.

> Is `toFixed` actually localized? Breaking on non en-US seems like a bad
> thing.

I ended up with this, which is much better:

>  return (0.99995 <= num && num <= 1)
>         ? formatNum(1, kFrac1Style)
>         : formatNum(num, kFracStyle);


> > +      └───0.00 MB (-09.09%) ── (fake child) [!]\n\
> 
> Stuff like this is a little unfortunate (the values are no longer aligned).
> Would it be too much trouble to special case (-10%,0%) given we also special
> case 100%? I'm not sure it's worth the effort.

With diffs you can get negative numbers *and* percentages larger than 100, so you can't guarantee alignment in general. And diffing is relatively rare, so I don't think it's worth any additional special-casing.

Comment 15

[Nicholas Nethercote [inactive]]

The whole numbers separators thing is quite interesting. Look at these examples:

> > (123456789.123).toLocaleString('en-US')
> '123,456,789.123'
> 
> > (123456789.123).toLocaleString('fr-FR')
> '123 456 789,123'
> 
> > (123456789.123).toLocaleString('de-DE')
> '123.456.789,123'
> 
> > (123456789.123).toLocaleString('ar-EG')
> '١٢٣٬٤٥٦٬٧٨٩٫١٢٣'
> 
> > (123456789.123).toLocaleString('en-IN')
> '12,34,56,789.123'

If you want to understand what's going on with the separators in 'en-IN', see https://en.wikipedia.org/wiki/Indian_numbering_system.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2fa6237585
Use non-ASCII chars directly in the code. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/14cef5ef3dc4
Improve treeline handling. r=erahm.
https://hg.mozilla.org/integration/mozilla-inbound/rev/075198910378
Use for..of loops where possible. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/97348d7aa09e
Change `var` to `let`. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/9819dbed2d88
Use toLocaleString(). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeea7ba794c3
Use String.prototype.padStart(). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e7eb7be03c
Put some parameters in `aFoo` form. erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/81ed2232fb09
Use template literals where suitable. r=erahm

Comment 17

[Noemi Erli[:noemi_erli]]

Backed out 8 changesets (Bug 1499906) for ES Lint failure in builds/worker/checkouts/gecko/toolkit/components/aboutmemory/content/aboutMemory.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=206527590&revision=81ed2232fb09403667483fad07a9ac4dc1d6f2f4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=206527590&repo=mozilla-inbound&lineNumber=276

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/be3ea7eb7ff484be2e8e840a64f8d86c6fdd0d65

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c53f4bdf597
(attempt 2) - Use non-ASCII chars directly in the code. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/d091653eec10
(attempt 2) - Improve treeline handling. r=erahm.
https://hg.mozilla.org/integration/mozilla-inbound/rev/51a3a633315c
(attempt 2) - Use for..of loops where possible. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/838f16e6dcf2
(attempt 2) - Change `var` to `let`. r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/edee090da6bd
(attempt 2) - Use toLocaleString(). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c9c172c245
(attempt 2) - Use String.prototype.padStart(). r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1752794d80ab
(attempt 2) - Put some parameters in `aFoo` form. erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/f35ac5e55bb4
(attempt 2) - Use template literals where suitable. r=erahm

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/2c53f4bdf597
https://hg.mozilla.org/mozilla-central/rev/d091653eec10
https://hg.mozilla.org/mozilla-central/rev/51a3a633315c
https://hg.mozilla.org/mozilla-central/rev/838f16e6dcf2
https://hg.mozilla.org/mozilla-central/rev/edee090da6bd
https://hg.mozilla.org/mozilla-central/rev/d8c9c172c245
https://hg.mozilla.org/mozilla-central/rev/1752794d80ab
https://hg.mozilla.org/mozilla-central/rev/f35ac5e55bb4