Closed Bug 1406375 Opened 4 years ago Closed 4 years ago

DevTools l10n helper returns inconsistent results for number formatting

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(4 files)

Attached image 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 on attachment 8915971 [details]
Bug 1406375 - always localize numbers in l10n::numbersWithDecimals;

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

LGTM

Honza
Attachment #8915971 - Flags: review?(odvarko) → review+
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
Attachment #8919667 - Flags: review?(odvarko) → review+
(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 on attachment 8920230 [details]
Bug 1406375 - update animation inspector test to support formatted numbers;

https://reviewboard.mozilla.org/r/191250/#review196650
Attachment #8920230 - Flags: review?(pbrosset) → review+
Pushed by jdescottes@mozilla.com:
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.