Various netmonitor tests fail because of decimal separator issues

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Developer Tools: Netmonitor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

unspecified
Firefox 24
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Particularly:

browser_net_content-type.js
browser_net_jsonp.js
browser_net_post-data.js
browser_net_simple-request-data.js
browser_net_sort-01.js
browser_net_sort-02.js
browser_net_sort-03.js
browser_net_status-codes.js

All report errors along the line of:

The tooltip size is incorrect. - Got 0,75 KB, expected 0.75 KB
The displayed size is incorrect. - Got 0,02 KB, expected 0.02 KB

It looks like the verification code and the rendering code use different L10N paths.

In "Region and Language" in my copy of Windows 7, the active setting is: Dutch (Nederlands). The decimal separator here is ',', not '.'. However, I'm running an English copy of Windows 7 (possibly en-GB, although I'm not sure - "Color Management" in the control panel seems to be missing a u...). I don't know enough about the L10N framework to be able to tell if it is an error in there or something that Net Monitor is doing wrong.
Yeah, it's pretty obvious. For example check out browser_net_content-type.js, see
> L10N.getFormatStr("networkMenu.sizeKB", 0.04)
..it should be
> L10N.getFormatStr("networkMenu.sizeKB", L10N.numberWithDecimals(0.04, 2))
Created attachment 751682 [details] [diff] [review]
Patch

Something like this works. The bind magic had me scratching my head for a bit, if you see a better/shorter way to write this, do let me know. :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #751682 - Flags: review?(vporof)
Comment on attachment 751682 [details] [diff] [review]
Patch

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

r+ with two nits. If you don't have time to address them that's fine, I'll make the changes myself when landing.

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +203,5 @@
>      return this.stringBundle.formatStringFromName(aName, aArgs, aArgs.length);
>    },
>  
>    /**
> +   * L10N shortcut function for numeric arguments that need to be formatted

Nit: fullstop at the end of the sentence.
You might want to also add the fact that all numbers will be formatted to contain only two decimals.

@@ +209,5 @@
> +   * @param array aArgs
> +   * @return string
> +   */
> +  getFormatStrWithNumbers: function(aName, ...aArgs) {
> +    let numberFmt = this.numberWithDecimals.bind(this);

You could simply inline this since => functions are lexically bound, so |this| will be correct.
Attachment #751682 - Flags: review?(vporof) → review+
Whiteboard: [land-in-fx-team]
Landed in fx-team with nits addressed: https://hg.mozilla.org/integration/fx-team/rev/8ab9e35fd41f

Thanks!
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8ab9e35fd41f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.