Sundry aboutMemory.js code improvements

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(8 attachments)

Assignee

Description

8 months ago
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.
Assignee

Comment 1

8 months ago
They're easier to read than \uXXXX escapes, and there's no reason to stick to
ASCII in the source code.
Attachment #9018166 - Flags: review?(erahm)
Assignee

Updated

8 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee

Comment 2

8 months ago
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.
Attachment #9018167 - Flags: review?(erahm)
Assignee

Comment 3

8 months ago
Attachment #9018169 - Flags: review?(erahm)
Assignee

Comment 4

8 months ago
For consistency; `let` is used in most places.
Attachment #9018170 - Flags: review?(erahm)
Assignee

Comment 5

8 months ago
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%".
Attachment #9018171 - Flags: review?(erahm)
Assignee

Comment 6

8 months ago
This lets us remove pad().
Attachment #9018172 - Flags: review?(erahm)

Comment 9

8 months ago
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).
Attachment #9018166 - Flags: review?(erahm) → review+
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`.
Attachment #9018167 - Flags: review?(erahm) → review+
Comment on attachment 9018169 [details] [diff] [review]
Use for..of loops where possible

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

Nice cleanup, thanks.
Attachment #9018169 - Flags: review?(erahm) → review+

Updated

8 months ago
Attachment #9018170 - Flags: review?(erahm) → review+
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.
Attachment #9018171 - Flags: review?(erahm) → review+

Updated

8 months ago
Attachment #9018172 - Flags: review?(erahm) → review+

Updated

8 months ago
Attachment #9018173 - Flags: review?(erahm) → review+
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.
Attachment #9018174 - Flags: review?(erahm) → review+
Assignee

Comment 14

8 months ago
> > +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.
Assignee

Comment 15

8 months ago
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 18

8 months ago
Pushed by nnethercote@mozilla.com:
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
Assignee

Updated

8 months ago
Flags: needinfo?(n.nethercote)
You need to log in before you can comment on or make changes to this bug.