1121879 - Refactor ViewHelpers.L10N.numberWithDecimals to use toLocaleString

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

This comment in ViewHelpers.L10N.numberWithDecimals says that nb.toLocaleString can't be used because not implemented on OSX:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#378

After testing locally, that doesn't seem to be the case anymore, and so this function can be simplified a lot.

I think just returning:
aNumber.toLocaleString(undefined, {
  maximumFractionDigits: aDecimals
});
after the check for isNaN and null should be enough. No need to check if there are decimals and pad with 0s anymore.

I'm also going to propose that we take this opportunity to add a new parameter to this function to force the minimum number of decimals. I've come across a case lately where I wanted my numbers to always have 2 decimals, even if 0s, for better alignment.

So maybe the signature could be:
numberWithDecimals: function(aNumber, aDecimals = 0, aMinDecimals = 0)

and then:

aNumber.toLocaleString(undefined, {
  maximumFractionDigits: aDecimals,
  minimumFractionDigits: aMinDecimals
});

Comment 1

[Patrick Brosset <:pbro>]

Victor, I think you worked on this. Does this make sense?

Comment 2

[Victor Porof [:vporof][:vp]]

Of course, I'd love to see that. Could file a bug for adding this in the network panel too, to keep numbers consistent.

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Victor Porof [:vporof][:vp] from comment #2)
> Of course, I'd love to see that. Could file a bug for adding this in the
> network panel too, to keep numbers consistent.
Cool, thanks. Can you give pointers to the places in the netmonitor code that would need to be changed. I can file a bug with these then.

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> (In reply to Victor Porof [:vporof][:vp] from comment #2)
> > Of course, I'd love to see that. Could file a bug for adding this in the
> > network panel too, to keep numbers consistent.
> Cool, thanks. Can you give pointers to the places in the netmonitor code
> that would need to be changed. I can file a bug with these then.

I'd say everywhere. Especially in the requests table.
Just grep for L10N.numberWithDecimals or getFormatStrWithNumbers

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - using toLocaleString

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8554267 [details] [diff] [review]
rev 1 - using toLocaleString

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

These changes look good to me, thanks!
Are you able to push to TRY to see if tests still pass or do you want me to do this?

Comment 7

[Patrick Brosset <:pbro>]

Pending try push with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48c6b490de56

Comment 8

[Patrick Brosset <:pbro>]

Note that the commit message in the patch is not formatted correctly:
"Refactor ViewHelpers.L10N.numberWithDecimals to use toLocaleString"

It should be:
"Bug 1121879 - Refactor ViewHelpers.L10N.numberWithDecimals to use toLocaleString; r=pbrosset"

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

Firstly, I'm sorry for being late.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> These changes look good to me, thanks!
> Are you able to push to TRY to see if tests still pass or do you want me to
> do this?
Actually, I have requested access.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> Note that the commit message in the patch is not formatted correctly:
> "Refactor ViewHelpers.L10N.numberWithDecimals to use toLocaleString"
> 
> It should be:
> "Bug 1121879 - Refactor ViewHelpers.L10N.numberWithDecimals to use
> toLocaleString; r=pbrosset"
Sorry, I missed that. Do I need to attach updated patch with this commit message?

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Abdelrhman Ahmed from comment #9)
> Firstly, I'm sorry for being late.
No problem at all! :)

> Actually, I have requested access.
Great, so you'll be able to push to TRY for the next patches you write.

> > It should be:
> > "Bug 1121879 - Refactor ViewHelpers.L10N.numberWithDecimals to use
> > toLocaleString; r=pbrosset"
> Sorry, I missed that. Do I need to attach updated patch with this commit
> message?
Yes please do attach a new patch with the updated commit message, and just set it as R+ yourself since I already reviewed the code changes.

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - using toLocaleString

Comment 12

[Patrick Brosset <:pbro>]

Thanks for the update.

There are test failures on TRY however: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48c6b490de56
I have canceled all remaining test jobs to avoid wasting execution time.

Can you take a look at the test failure messages and see what is the right course of action to fix them?
When you think you've found the fix, can you try and run the tests locally? Here's how to do it: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests (see the devtools mochitest section).

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

Hello,

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> Can you take a look at the test failure messages and see what is the right
> course of action to fix them?

These cases caused the fail of test
1.
toLocalString rounds numbers
eg. 12.12537 assume maximumFractionDigits: 4
result 12.1254 and expected 12.1253

2.
eg. 36.602 assume maximumFractionDigits: 2
result 36.6 and expected 36.60

I think if we try to edit numberWithDecimals to use maximumFractionDigits
and minimumFractionDigits, it won't be better than current code there.
So if we want to use it like this patch, I think we need to edit test cases
to be compatible with new edits and uses minimumFractionDigits.

Is that right?

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8555285 [details] [diff] [review]
rev 2 - using toLocaleString

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

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +369,4 @@
>     * @return string
>     *         The localized number as a string.
>     */
> +  numberWithDecimals: function(aNumber, aDecimals = 0, aMinDecimals = 0) {

In fact, re-reading the patch, I realized adding this new parameter was a mistake because its default value changes the behavior of the function.
So this means callers of numberWithDecimals that don't provide a value for aMinDecimals (which is to say, *all* callers right now) won't have the expected number of decimals in some cases.

I think the right course of actions is removing the aMinDecimals parameter, and just using aDecimals for both maximumFractionDigits *and* minimumFractionDigits.

This is what the function was doing before anyway, padding with 0s if needed, and always cutting after aDecimals, not before.

Comment 15

[Victor Porof [:vporof][:vp]]

Why not numberWithDecimals: function(aNumber, aDecimals = 0, aMinDecimals = aDecimals) ?

Comment 16

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> This is what the function was doing before anyway, padding with 0s if
> needed, and always cutting after aDecimals, not before.

I don't think so, let's describe this function behaviour after integer and isNaN checks
> let localized = aNumber.toLocaleString(); // localize

this calling to toLocaleString handles 3 cases and converts it to string:

case 1:
eg. aNumber is 12.124563
result is "12.125"
it gets first 3 digits after dot and rounds last digit depending on next digit.

case 2:
eg. aNumber is 12.1
result is "12.1"

case 3:
eg. aNumber is 12.0001
result is "12"

next, returns the result if localized has no dot.
next, padding and get result to return.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> I think the right course of actions is removing the aMinDecimals parameter,
> and just using aDecimals for both maximumFractionDigits *and*
> minimumFractionDigits.

You are right in removing aMinDecimals parameter because it changes behaviour of the function but if we used maximumFractionDigits and minimumFractionDigits we will miss these cases:

case 1:
aNumber is 0
maximumFractionDigits = minimumFractionDigits = 2
result will be "0.00" and the expected result is "0"

case 2:
aNumber is 12.0001
maximumFractionDigits = minimumFractionDigits = 2
result will be "12.00" and the expected result is "12"

case 3:
aNumber is 12.12543
maximumFractionDigits = minimumFractionDigits = 2
result will be "12.13" and the expected result is "12.12"

the main problems here are the fixed minimumFractionDigits in cases 1, 2 and the rounding in case of aDecimals is less than 3.

I think if we tried to add some code to fix previous cases after using aDecimals for both maximumFractionDigits and minimumFractionDigits won't be better than the existing code.

(In reply to Victor Porof [:vporof][:vp] from comment #15)
> Why not numberWithDecimals: function(aNumber, aDecimals = 0, aMinDecimals =
> aDecimals) ?

The behaviour will not be as expected when using aDecimals for both maximumFractionDigits and minimumFractionDigits but after resloving previous cases we need to know if caller of this function uses aMinDecimals parameter or not to avoid the problem of previous cases or next cases:

case 1:
aMinDecimals parameter is not used from the caller
numberWithDecimals(12.0001, 2)
here the expected result is "12"

case 2:
aMinDecimals parameter is used from the caller
numberWithDecimals(12.0001, 2, 2)
here the expected result is "12.00"

So if there is no need for aMinDecimals as a parameter we don't need to add it :).

Comment 17

[Abdelrahman Ahmed [:abdelrahman]]

Is there any update?

Comment 18

[Patrick Brosset <:pbro>]

Sorry for the delay, I just read your analysis in comment 16 and it sounds very correct indeed.
So, this bug is simply about refactoring the implementation of numberWithDecimals without changing its behavior, so let's drop the aMinDecimals parameter altogether for now.

So let's see if we can simplify the internal implementation of the function using toLocaleString while respecting what the function outputs.
Because the function is a little weird in the sense that it only pads with 0s if there are fractions, we can't only use toLocaleString, we have to retain some of the special cases in the function.
But I do believe we can use toLocaleString to get rid of the, somewhat complex, final 0 padding logic.
What about this:

function numberWithDecimals(aNumber, aDecimals = 0) {
  // If this is an integer, don't do anything special.
  if (aNumber == (aNumber | 0)) {
    return aNumber;
  }
  if (isNaN(aNumber) || aNumber == null) {
    return "0";
  }

  let localized = aNumber.toLocaleString(); // localize

  // If no grouping or decimal separators are available, bail out, because
  // padding with zeros at the end of the string won't make sense anymore.
  if (!localized.match(/[^\d]/)) {
    return localized;
  }

  return aNumber.toLocaleString(undefined, {
    minimumFractionDigits: aDecimals,
    maximumFractionDigits: aDecimals
  })
}

We have to keep the first call to toLocaleString() so that we can test for fractions in the result, but in the end, instead of dealing with arrays, we just call again toLocaleString with both minimumFractionDigits and maximumFractionDigits.

Would the test pass with this?

Comment 19

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18)
> Sorry for the delay, I just read your analysis in comment 16 and it sounds

No problem :)
 
> function numberWithDecimals(aNumber, aDecimals = 0) {
>   // If this is an integer, don't do anything special.
>   if (aNumber == (aNumber | 0)) {
>     return aNumber;
>   }
>   if (isNaN(aNumber) || aNumber == null) {
>     return "0";
>   }
> 
>   let localized = aNumber.toLocaleString(); // localize
> 
>   // If no grouping or decimal separators are available, bail out, because
>   // padding with zeros at the end of the string won't make sense anymore.
>   if (!localized.match(/[^\d]/)) {
>     return localized;
>   }
> 
>   return aNumber.toLocaleString(undefined, {
>     minimumFractionDigits: aDecimals,
>     maximumFractionDigits: aDecimals
>   })
> }
> Would the test pass with this?

This is better but misses one case (the case of rounding) in the last return
of previous code and also using of |maximumFractionDigits: aDecimals| has no
meaning because it doesn't affect results because of using |minimumFractionDigits|
so I edited it to handle this case
>  localized = aNumber.toLocaleString(undefined, {
>    minimumFractionDigits: (aDecimals + 1)
>  });
>  return localized.substring(0, localized.length-1);

I also tested it locally and worked well.
Is this acceptable?

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Abdelrhman Ahmed from comment #19)
> >  localized = aNumber.toLocaleString(undefined, {
> >    minimumFractionDigits: (aDecimals + 1)
> >  });
> >  return localized.substring(0, localized.length-1);
> I also tested it locally and worked well.
> Is this acceptable?
It's sad that this bug is about simplifying a function by making use of toLocaleString and that we end up with a code that's just as complex as before.
With my proposal (with maximumFractionDigits removed), the remaining failing tests are just due to rounding errors, right? Since we'd normally get more precise results with this new implementation than with the current one, I'd say we could just change the tests that fail.

Comment 21

[Abdelrahman Ahmed [:abdelrahman]]

I have pushed patch for try to determine cases need to be changed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=372a5bfaa336

Comment 22

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - using toLocaleString

> With my proposal (with maximumFractionDigits removed), the remaining failing
> tests are just due to rounding errors, right?

Here in this we need maximumFractionDigits. When I said that (removing it)
, it was about the previous code because of this line
>  return localized.substring(0, localized.length-1);

I pushed this patch to try server to check everything is ok
(https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f293bb0dc2b)

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8568971 [details] [diff] [review]
rev 3 - using toLocaleString

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

LGTM and try is green. Good to go.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/dabd88a2065d

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dabd88a2065d