1406375 - DevTools l10n helper returns inconsistent results for number formatting

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

Attachment: netmonitor-ar.png

See the netmonitor screenshot in attachement.

The finish time is displayed in locale numbers, while the DOMContentLoaded and load timings are not.

The reason for this is that our method to format decimal numbers is not always using a formatter. See http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/devtools/shared/l10n.js#137-160

If the number is an integer:

    if (number === (number|0)) {
      return number;
    }

We return the number directly, which will not be formatted when stringified later.
If the number is invalid    

    if (isNaN(number) || number === null) {
      return "0";
    }

We return "0" instead of the locale string representing the number 0.
We should also format numbers in this case.

Going to block that on Bug 1406311 as I am changing a bit the way the method works in this other bug and it should make it easier to implement this bug afterwards.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1406375 - always localize numbers in l10n::numbersWithDecimals;

Review commit: https://reviewboard.mozilla.org/r/187256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187256/

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1406375 - test integers in browser_num-l10n.js;

Review commit: https://reviewboard.mozilla.org/r/190578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190578/

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8915971 [details]
Bug 1406375 - always localize numbers in l10n::numbersWithDecimals;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187256/diff/1-2/

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8915971 [details]
Bug 1406375 - always localize numbers in l10n::numbersWithDecimals;

https://reviewboard.mozilla.org/r/187256/#review196306

LGTM

Honza

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8919667 [details]
Bug 1406375 - test integers in browser_num-l10n.js;

https://reviewboard.mozilla.org/r/190578/#review196310

Looks good, thanks for working on this!

R+ (assuming try is green)

Honza

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1406375 - update animation inspector test to support formatted numbers;

Integers used to skip localization & formatting.
We used to have (for 2 decimals formatting)
1000   -> 1000
1000.1 -> 1,000.10

With the other patches attached here, the behavior is now consistent
1000   -> 1,000
1000.1 -> 1,000.10

This changeset updates some regexp in an animation inspector test to be
compatible with the new format.

Review commit: https://reviewboard.mozilla.org/r/191250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191250/

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8919667 [details]
Bug 1406375 - test integers in browser_num-l10n.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190578/diff/1-2/

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8915971 [details]
Bug 1406375 - always localize numbers in l10n::numbersWithDecimals;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187256/diff/2-3/

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> Comment on attachment 8919667 [details]
> Bug 1406375 - test integers in browser_num-l10n.js;
> 
> https://reviewboard.mozilla.org/r/190578/#review196310
> 
> Looks good, thanks for working on this!
> 
> R+ (assuming try is green)
> 
> Honza

Thanks for the review! Ran through try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=648f9b2953823fbfca74da265511c18912730294

Thanks to this spotted a small regression so I updated our implementation and added a new unit test to cover this case. 
However a small test update is needed in the animation inspector.

New try at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7020e794a2c9e59b8dd74066c2bdba197c158bb

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8920230 [details]
Bug 1406375 - update animation inspector test to support formatted numbers;

https://reviewboard.mozilla.org/r/191250/#review196650

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ff5094d1ff0
test integers in browser_num-l10n.js;r=Honza
https://hg.mozilla.org/integration/autoland/rev/6e24be390f51
always localize numbers in l10n::numbersWithDecimals;r=Honza
https://hg.mozilla.org/integration/autoland/rev/48219c8a825d
update animation inspector test to support formatted numbers;r=pbro

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3ff5094d1ff0
https://hg.mozilla.org/mozilla-central/rev/6e24be390f51
https://hg.mozilla.org/mozilla-central/rev/48219c8a825d