874040 - Various netmonitor tests fail because of decimal separator issues

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[:Gijs (he/him)]

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.

Comment 1

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

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))

Comment 2

[:Gijs (he/him)]

Attachment: 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. :-)

Comment 3

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

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.

Comment 4

[:Gijs (he/him)]

Landed in fx-team with nits addressed: https://hg.mozilla.org/integration/fx-team/rev/8ab9e35fd41f

Thanks!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ab9e35fd41f