Closed Bug 874040 Opened 7 years ago Closed 7 years ago

Various netmonitor tests fail because of decimal separator issues

Categories

(DevTools :: Netmonitor, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: Gijs, Assigned: Gijs)

Details

Attachments

(1 file)

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))
Attached patch PatchSplinter Review
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
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.