1029540 - ViewHelpers.L10N.numberWithDecimals doesn't properly handle NaN and numbers that can't be localized

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

numberWithDecimals(0.0001, 2) // "00"

It should be "0" or "0.0" or "0,0" instead, but since we don't know the localization settings ("," or "."?), we should simply return "0".

Comment 1

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

Attachment: v1

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8445222 [details] [diff] [review]
v1

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

I think browser.ini needs to be rebased.  Looks good besides that, just a couple of questions about the early return.

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +375,5 @@
>      let localized = aNumber.toLocaleString(); // localize
> +
> +    // If no grouping or decimal characters are available, bail out, because
> +    // padding with zeros at the end of the string won't make sense anymore.
> +    if (!localized.contains(".") && !localized.contains(",")) {

I'm actually not sure what case this is catching.  Is this just an early return to prevent some computation or is this actually changing the output of the function?  For instance, if I run l10n.numberWithDecimals(123, 10); with or without the patch, I get the same result.

Also, do all locales use , for grouping?

Comment 3

[Brian Grinstead [:bgrins]]

> I'm actually not sure what case this is catching.  Is this just an early
> return to prevent some computation or is this actually changing the output
> of the function?  For instance, if I run l10n.numberWithDecimals(123, 10);
> with or without the patch, I get the same result.

Scratch that, I see the case in the first comment

Comment 4

[Brian Grinstead [:bgrins]]

With the patch applied:

l10n.numberWithDecimals(0.0001, 2); // "0"
l10n.numberWithDecimals(0.001, 2);  // "0.00"
l10n.numberWithDecimals(1.00001, 2) // "1"

I'm not really sure what the right result here is, but it is a bit odd that some decimals become NUM and some become NUM.00.  Luckily this also fixed what looks like a really bad bug without the patch applied:

l10n.numberWithDecimals(1.00001, 2) // "10" ? huh

Comment 5

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

This is Number.toLocaleString misbehaving. There's nothing I can do about that, like for 0.0001 vs. 0.001.

Comment 6

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

Attachment: v2

Addressed comments from the discussion on IRC.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8445262 [details] [diff] [review]
v2

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

Changes LGTM

Comment 8

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

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

Comment 9

[Carsten Book [:Tomcat]]

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