1009795 - A Persian digit (۵) is replaced with decimal separator on Firefox download manager and popup with system Persian locale

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[ebrahim]

Attachment: Firefox.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428193813

Steps to reproduce:

I installed Ubuntu 14.04 and while installation I set Tehran timezone and Persian keyboard but English for UI.

I opened Firefox with Persian locale then downloaded a file

My related environment variables is:
$ env | sort
LANG=en_US.UTF-8
LANGUAGE=en_US
LC_ADDRESS=fa_IR
LC_IDENTIFICATION=fa_IR
LC_MEASUREMENT=fa_IR
LC_MONETARY=fa_IR
LC_NAME=fa_IR
LC_NUMERIC=fa_IR
LC_PAPER=fa_IR
LC_TELEPHONE=fa_IR
LC_TIME=fa_IR



Actual results:

I see 2۵7 instead 2.7 (۵ is not even a decimal separator, it is our equivalant to 5)


Expected results:

It should be either ۲٫۷ or 2.7

Comment 1

[(no longer active)]

Wow, that is super weird!  Ebrahim, can you please attach a larger screenshot so that I can see the surrounding text on the dialog?  Thanks!

Comment 2

[ebrahim]

Attachment: 1.png

Comment 3

[ebrahim]

Attachment: 2.png

Comment 4

[Simon Montagu :smontagu]

This is mentioned in bug 999003, but perhaps it deserves its own bug rather than being duped.

See bug 999003 comment 10 for my analysis of why this happens: the following line at http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#49
  
  return this.gDecimalSymbol = Number(5.4).toLocaleString().match(/\D/);
 
assumes that the first character in the output of |Number(5.4).toLocaleString()| which is not an ASCII numeral will be the decimal separator in the appropriate locale. Because of bug 999003 toLocaleString() returns "۵٫۴", so the regex returns ۵!

Comment 5

[:Gijs (out for now; he/him)]

This is pretty weird, as bugs go. Simon, is there no API we can use to just get the decimal separator?

Comment 6

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #5)
> This is pretty weird, as bugs go. Simon, is there no API we can use to just
> get the decimal separator?

In the worst case, we could just get the second character in the returned string, assuming the digit "5" is made of only one UTF-16 character.

But what we're really trying to do is to format a number with a given number of decimal digits:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#443

I don't know whether we can modify the code to use toLocaleString with the minimumFractionDigits and maximumFractionDigits options. This depends on the assumptions of the callers, and whether we want to localize the whole number using locale-specific digits or not.

Comment 7

[Simon Montagu :smontagu]

(In reply to :Gijs Kruitbosch from comment #5)
> This is pretty weird, as bugs go. Simon, is there no API we can use to just
> get the decimal separator?

Not that I'm aware of.

Comment 11

[Masatoshi Kimura [:emk]]

As bug 999003 comment #31 is saying, it is not so obvious that the new toLocaleString() behavior is a bug.

http://ecma-international.org/ecma-262/5.1/#sec-15.7.4.3
> 15.7.4.3 Number.prototype.toLocaleString()
>
> Produces a String value that represents this Number value formatted
> according to the conventions of the host environment’s current locale. This
> function is implementation-dependent, and it is permissible, but not
> encouraged, for it to return the same thing as toString.

But the download manager issue is definitely a bug. The download manager should not depend on the implementation-defined behavior in the first place. This bug should not have been marked as a dupe of bug 999003.

Comment 12

[Jeff Walden [:Waldo]]

The new toLocaleString behavior is desired behavior, as the Internationalization API reformulated the old methods in terms of Intl.* operations.  DownloadUtils.jsm should be changed to use a NumberFormat object, perhaps passed some specific locale, and it shouldn't be trying to sniff the value of a decimal separator (which it's entirely possible isn't even a concept in some localizations -- Intl is supposed to abstract away all the locale-specific quirks, you tell it what data you want formatted and how, and it gives you something that matches as best as it can -- which may not be any particular expected way).

Comment 14

[Simon Montagu :smontagu]

Attachment: Patch

Comment 15

[Simon Montagu :smontagu]

This patch is an attempt to get the best of both worlds by using toLocaleString to pick up the locale's decimal separator, with maximumDigits/minimumDigits to get the fixed number of decimals, but adding "-u-nu-latn" to the default locale so as not to get localized digit forms.

An initial try run at https://tbpl.mozilla.org/?tree=Try&rev=6c1a0de47591 had failures because "Infinity" also got localized to "∞", so I added a condition for that too, which passed https://tbpl.mozilla.org/?tree=Try&rev=40864ec9af45

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8450476 [details] [diff] [review]
Patch

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

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +107,5 @@
>  
>      let status;
> +    // Number.toLocaleString can return U+221E INFINITY for Infinity
> +    const unicodeInfinitySymbol = '\u221e';
> +    if (rate === "Infinity" || rate === unicodeInfinitySymbol) {

I think we should rather skip toLocaleString and directly return Infinity in convertByteUnits, should be a simple if condition, skipping the toLocaleString conversion code. I also honestly don't know if \u221e would be returned for any locale in the world, so that solution may be safer.

@@ +458,5 @@
>  
>      // Get rid of insignificant bits by truncating to 1 or 0 decimal points
>      // 0 -> 0; 1.2 -> 1.2; 12.3 -> 12.3; 123.4 -> 123; 234.5 -> 235
> +    // added in bug 462064: (unitIndex != 0) makes sure that no decimal digit for bytes appears when aBytes < 100
> +    let decimalDigits = (aBytes > 0) && (aBytes < 100) && (unitIndex != 0) ? 1 : 0;

let's call this fractionDigits to mimic the intl api.

@@ +467,5 @@
> +    defaultLocale = defaultLocale + "-u-nu-latn";
> +
> +    aBytes = aBytes.toLocaleString(defaultLocale,
> +                                   {minimumFractionDigits: decimalDigits,
> +                                    maximumFractionDigits: decimalDigits});

At this point I think it's more coherent to just stick to the intl API:

aBytes = Intl.NumberFormat(defaultLocale + "-u-nu-latn",
                           { maximumFractionDigits: fractionDigits })
             .format(aBytes);

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8450476 [details] [diff] [review]
Patch

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

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +107,5 @@
>  
>      let status;
> +    // Number.toLocaleString can return U+221E INFINITY for Infinity
> +    const unicodeInfinitySymbol = '\u221e';
> +    if (rate === "Infinity" || rate === unicodeInfinitySymbol) {

Without looking beyond Splinter context that may be misleading me, doesn't this only happen if |normalizedSpeed === Infinity|?  I'd skip conversion entirely in that case and jump straight to the code below -- it's probably a category error to ask for things that will just get ignored entirely.

@@ +463,5 @@
>  
> +    let defaultLocale = Intl.NumberFormat().resolvedOptions().locale;
> +
> +    // Backward compatibility: don't use localized digits
> +    defaultLocale = defaultLocale + "-u-nu-latn";

Possibly worth a followup to fix this?  Naively I'd think people would generally want to read localized digits.  But I guess this better preserves the DownloadUtils API for all the umpteen callers of it, that I remember seeing when I looked at this last.

@@ +467,5 @@
> +    defaultLocale = defaultLocale + "-u-nu-latn";
> +
> +    aBytes = aBytes.toLocaleString(defaultLocale,
> +                                   {minimumFractionDigits: decimalDigits,
> +                                    maximumFractionDigits: decimalDigits});

You really should just create and cache a NumberFormat object, I think.  I'm not sure you have any way to cache |defaultLocale|, but at least this formatter can be cached.

Comment 18

[Marco Bonardo [:mak]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> Comment on attachment 8450476 [details] [diff] [review]
> Patch
> 
> Review of attachment 8450476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/downloads/DownloadUtils.jsm
> @@ +107,5 @@
> >  
> >      let status;
> > +    // Number.toLocaleString can return U+221E INFINITY for Infinity
> > +    const unicodeInfinitySymbol = '\u221e';
> > +    if (rate === "Infinity" || rate === unicodeInfinitySymbol) {
> 
> Without looking beyond Splinter context that may be misleading me, doesn't
> this only happen if |normalizedSpeed === Infinity|?  I'd skip conversion
> entirely in that case and jump straight to the code below -- it's probably a
> category error to ask for things that will just get ignored entirely.

yes, good catch, we might also check normalizedSpeed and skip all of the conversion with small refactoring.

> Possibly worth a followup to fix this?  Naively I'd think people would
> generally want to read localized digits.  But I guess this better preserves
> the DownloadUtils API for all the umpteen callers of it, that I remember
> seeing when I looked at this last.

Maybe, I'd not change this here, it might introduce unwanted regressions and off-hand I don't know how common is to have localized digits in download managers.

> @@ +467,5 @@
> > +    defaultLocale = defaultLocale + "-u-nu-latn";
> > +
> > +    aBytes = aBytes.toLocaleString(defaultLocale,
> > +                                   {minimumFractionDigits: decimalDigits,
> > +                                    maximumFractionDigits: decimalDigits});
> 
> You really should just create and cache a NumberFormat object, I think.  I'm
> not sure you have any way to cache |defaultLocale|, but at least this
> formatter can be cached.

yep, defaultLocale might be cached in a lazy getter in the module scope, is it expensive?

Comment 19

[Jeff Walden [:Waldo]]

(In reply to Marco Bonardo [:mak] from comment #18)
> > You really should just create and cache a NumberFormat object, I think.  I'm
> > not sure you have any way to cache |defaultLocale|, but at least this
> > formatter can be cached.
> 
> yep, defaultLocale might be cached in a lazy getter in the module scope, is
> it expensive?

Probably not too much.  I'm just not aware of a standards-based way to get the locale other than via this resolved-options trick, on a necessarily fresh Intl object.  (Caching the Intl object or the options would just get the originally-correct locale, not the instantaneously-correct one.)  Maybe the non-standard mozL10N thing exposes something like this, that would be adequate.  (Although that may not consult the same thing the default locale thing here does, so might not be an adequate replacement.)

Comment 23

[ebrahim]

still exists and widely affects Firefox users on everyday use

Comment 24

[Marco Bonardo [:mak]]

There is a wip patch and comments 16 on to address, maybe we can leave this as a mentored bug for the community?

Comment 25

[Hamid Reza]

FF40 and it still exists.
How long it takes to fix such a bug? Is it too hard or the code is so unmaintainable, nobody can fix some character replacement issue? Or I'm too naive to thinks it must be so simple?

Comment 26

[:Gijs (out for now; he/him)]

(In reply to Hamid Reza from comment #25)
> FF40 and it still exists.
> How long it takes to fix such a bug? Is it too hard or the code is so
> unmaintainable, nobody can fix some character replacement issue? Or I'm too
> naive to thinks it must be so simple?

It's not entirely trivial to figure out the best fix, considering add-ons and the number of callsites. Nobody has volunteered to fix up the patch, and I *think* Simon is a lot less active these days than he was. Marco is also mostly away until August 24. Ehsan, is this something you can pick up next week?

Comment 27

[(no longer active)]

I'll take this.  Sorry for the delay.

Comment 28

[(no longer active)]

Attachment: Use toLocalString to format download size instead of the decimalSymbol hook

I had to modify the test expectations according to the changes in the number formatter output.  As a bonus, this test now passes on systems with non-en_US locales!

Comment 29

[:Paolo Amadini]

Comment on attachment 8651592 [details] [diff] [review]
Use toLocalString to format download size instead of the decimalSymbol hook

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

::: toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js
@@ +181,5 @@
>  
>    // Now test without rates, via getDownloadStatusNoRate.
>    statusFunc = DownloadUtils.getDownloadStatusNoRate.bind(DownloadUtils);
>  
> +  testStatus(statusFunc, 2, 1, 7, ["A few seconds remaining -- 2 of 2 KB", 1.724]);

Hm, changing the test expectation from "2.3 of 2.4 KB" to "2 of 2 KB" doesn't seem right, seems we're losing precision. Why is this happening?

Comment 30

[(no longer active)]

(In reply to :Paolo Amadini from comment #29)
> Comment on attachment 8651592 [details] [diff] [review]
> Use toLocalString to format download size instead of the decimalSymbol hook
> 
> Review of attachment 8651592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js
> @@ +181,5 @@
> >  
> >    // Now test without rates, via getDownloadStatusNoRate.
> >    statusFunc = DownloadUtils.getDownloadStatusNoRate.bind(DownloadUtils);
> >  
> > +  testStatus(statusFunc, 2, 1, 7, ["A few seconds remaining -- 2 of 2 KB", 1.724]);
> 
> Hm, changing the test expectation from "2.3 of 2.4 KB" to "2 of 2 KB"
> doesn't seem right, seems we're losing precision. Why is this happening?

Doh, good catch.  All of these test changes were necessary due to a bug in getLocaleNumberFormat ignoring the fractionDigits argument in computing the cache key, so we'd end up using the same cached NumberFormat object for all fractionDigits arguments for the same locale.

Comment 31

[(no longer active)]

Attachment: Use toLocalString to format download size instead of the decimalSymbol hook

Original Patch by Simon Montagu <[e-mail]>

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 8652359 [details] [diff] [review]
Use toLocalString to format download size instead of the decimalSymbol hook

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

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +45,5 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
>                                    "resource://gre/modules/PluralForm.jsm");
>  
> +let localeNumberFormatCache = {};

what about using a Set? it has a nicer API.

@@ +474,5 @@
> +    // added in bug 462064: (unitIndex != 0) makes sure that no decimal digit for bytes appears when aBytes < 100
> +    let fractionDigits = (aBytes > 0) && (aBytes < 100) && (unitIndex != 0) ? 1 : 0;
> +
> +    // toLocaleString can return U+221E or Infinity for Infinity values,
> +    // so normalize that to Infinity here.

I think this patch removed the toLocaleString call that was in the previous version and now this comment is outdated?

Comment 33

[(no longer active)]

(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #32)
> Comment on attachment 8652359 [details] [diff] [review]
> Use toLocalString to format download size instead of the decimalSymbol hook
> 
> Review of attachment 8652359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/downloads/DownloadUtils.jsm
> @@ +45,5 @@
> >  
> >  XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
> >                                    "resource://gre/modules/PluralForm.jsm");
> >  
> > +let localeNumberFormatCache = {};
> 
> what about using a Set? it has a nicer API.

I think you mean Map, since this needs to be an associative array.  Will do.

Comment 34

[(no longer active)]

Attachment: Use toLocalString to format download size instead of the decimalSymbol hook

Original Patch by Simon Montagu <[e-mail]>

Comment 35

[Marco Bonardo [:mak]]

Comment on attachment 8652439 [details] [diff] [review]
Use toLocalString to format download size instead of the decimalSymbol hook

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

LGTM, thank you for finalizing this!

Comment 36

[(no longer active)]

Comment on attachment 8652439 [details] [diff] [review]
Use toLocalString to format download size instead of the decimalSymbol hook

Approval Request Comment
[Feature/regressing bug #]: Unclear, this has probably been broken for a while, possibly after we switched to the new download manager.
[User impact if declined]: See comment 0, and the screenshots.  The bug is pretty embarrassing!
[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: I think this patch is fairly low risk, it replaces a well understood hack with a proper implementation.
[String/UUID change made/needed]: None.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7008d63383

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb7008d63383

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652439 [details] [diff] [review]
Use toLocalString to format download size instead of the decimalSymbol hook

Taking it for aurora. It is indeed not super user friendly display for our Persian users.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd468f69c6a2

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ebrahim, could you please verify this works as expected on the latest Nightly 8/28 or later?

Comment 42

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Pike, are there any l10n automated tests in m-c that would catch any unexpected fall outs from this fix? I am ok with uplifting to Beta at this point but wanted to check with you if any additional testing is needed before we do that. I am waiting for Ebrahim to confirm that this issue is fixed before approving Beta uplift.

Comment 43

[Axel Hecht [:Pike]]

Pretty sure there are no tests.

Does this patch affect fennec? As, AFAIK, we don't have the Intl API on on fennec.

Comment 44

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ehsan might be able to tell us whether this affects Fennec or not.

Comment 45

[ebrahim]

Attachment: NaN.png

Hey guys, thanks for resolving this but I see NaN all over instead on a Windows system with Nightly :(

Steps to repro:
1. Open a Windows system, from "Change date, time, or number formats", change the locale to Persian
2. Download a file, a big sized file that you could see its progress on Fx download manager and open fx download manager.

The issue was available on a Windows system.

Comment 46

[ebrahim]

Attachment: 41.0b4.png

It is interesting that the 41.0b4 on the same machine that just the mentioned setting is changed, is working differently than the time I posted the bug (the original issue was same on Ubuntu and Windows on that time AFAIK and now I should check Ubuntu if something is changed there)

Comment 47

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Pike, can we get somebody in our l10n team to manually verify this fix before I approve uplift to Beta? Based on comment 45, my confidence in uplifting this fix without verification is low. Appreciate your help.

Comment 48

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8652439 [details] [diff] [review]
Use toLocalString to format download size instead of the decimalSymbol hook

Based on the verification info provided in comment 45, I do not feel comfortable uplifting this patch to Beta. Once Pike/l10team provides an additional round of validation, we can uplift to Beta. Please let me know if there are any concerns.

Comment 49

[Axel Hecht [:Pike]]

(In reply to Ritu Kothari (:ritu) from comment #47)
> Pike, can we get somebody in our l10n team to manually verify this fix
> before I approve uplift to Beta? Based on comment 45, my confidence in
> uplifting this fix without verification is low. Appreciate your help.

I tried to reproduce this, and failed, at least on my mac.

I've glanced at things, and looked at https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js?offset=400 for inspiration.

Which left me puzzled as it's using the gDecimalSymbol to create some sort of _() function.

Ebrahim, maybe you can reproduce your problem in a browser-context scratchpad, https://developer.mozilla.org/en-US/docs/Tools/Scratchpad#Running_Scratchpad_in_the_browser_context, and use remote debugging tools to find out what's actually turning things into NaN?

You can look at the unit test to get inspiration on how to get to the downloadutils API, and what calls to use. I'd start with 

Cu.import("resource://gre/modules/DownloadUtils.jsm");
DownloadUtils.getTransferTotal(1024*1.2, 1024*4.3)

or so.

Comment 50

[ebrahim]

This is easy to repro thing guys, just change the locale from "Change date, time, or number formats" and do the thing:

41.0b5
English (United States) locale:
Cu.import("resource://gre/modules/DownloadUtils.jsm");
DownloadUtils.getTransferTotal(1024*1.2, 1024*4.3);
> "1.2 of 4.3 KB"

Persian locale:
Cu.import("resource://gre/modules/DownloadUtils.jsm");
DownloadUtils.getTransferTotal(1024*1.2, 1024*4.3);
> "1ن2 of 4ن3 KB"

43.0a1 (2015-08-28):
English (United States):
Cu.import("resource://gre/modules/DownloadUtils.jsm");
DownloadUtils.getTransferTotal(1024*1.2, 1024*4.3);
> "1.2 of 4.3 KB"

Persian locale:
Cu.import("resource://gre/modules/DownloadUtils.jsm");
DownloadUtils.getTransferTotal(1024*1.2, 1024*4.3);
> "NaN of NaN KB"

Comment 51

[ebrahim]

Hmm, from digging things here I reached to that the issue is not related to the change here, Firefox is always returning NaN for every number when my Windows system locale is set to Persian, on 41.0b5 (which was before the change) and after the change:

(123).toLocaleString()
> "ناعدد"
(123).toLocaleString("fa")
> "ناعدد"
(123).toLocaleString("en")
> "NaN"

Maybe ICU data is not package for my installation? If so how I am getting localized string for NaN already? And how both beta and nightly installation have the same issue?

Comment 52

[Axel Hecht [:Pike]]

Attachment: Windows 7, Persian region settings, en-US language on Windows and Nightly

43.0a1 (2015-08-30) en-US Nightly, Windows UI language is set to en-US, but the Format is set to Persian.

I've also added a bunch of intermediate hints that Nightly is picking up Persian.

I don't understand the output, though. This needs someone else, I'm afraid.

Comment 53

[ebrahim]

Your output is correct and perhaps the issue is limited to my machine (Windows 8.1N). Thanks everyone.

Comment 54

[(no longer active)]

(In reply to ebrahim from comment #53)
> Your output is correct and perhaps the issue is limited to my machine
> (Windows 8.1N). Thanks everyone.

Please file a follow-up bug for this...

Comment 55

[(no longer active)]

(In reply to Ritu Kothari (:ritu) from comment #44)
> Ehsan might be able to tell us whether this affects Fennec or not.

I am not sure.  Marco, Paolo, is this code used on Fennec?

Comment 56

[:Paolo Amadini]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #55)
> I am not sure.  Marco, Paolo, is this code used on Fennec?

Looks like yes:

http://mxr.mozilla.org/mozilla-central/search?string=DownloadUtils&find=aboutDownloads.js

Comment 57

[(no longer active)]

So given that bug 864843 has not landed yet, should we really have used the Intl API for this?  :(

If not, can you please suggest a work-around?  I'm not sure about the history of this bug and why we were trying to use the Intl API...

Comment 58

[:Paolo Amadini]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #57)
> So given that bug 864843 has not landed yet, should we really have used the
> Intl API for this?  :(

Heh, apparently not for Android? I actually looked at the docs here but didn't see indication that the support on Android was partial:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl

> If not, can you please suggest a work-around?  I'm not sure about the
> history of this bug and why we were trying to use the Intl API...

Just do feature detection and use the old code as fallback, so that we have the right behavior only on Desktop? I don't know of other APIs that can do the equivalent of "-u-nu-latn" and specify the number of decimal digits (maybe they exist and I'm unaware of them).

It seems we figured out some more things to test now, so the second attempt should be better :-(

We should try to capture any regression we found using automated tests. That might not be easy since part of this depends on the OS environment.

Comment 59

[(no longer active)]

Yeah actually testing this requires changing the OS locale, I don't think we can do that.

Comment 60

[(no longer active)]

Attachment: Part 2: Revert to the old gDecimalSymbol hack if the Internationalization API is not available

This will be removed in bug 1200494.

Comment 61

[Marco Bonardo [:mak]]

Comment on attachment 8655224 [details] [diff] [review]
Part 2: Revert to the old gDecimalSymbol hack if the Internationalization API is not available

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

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +487,5 @@
> +        aBytes = getLocaleNumberFormat(fractionDigits)
> +                   .format(aBytes);
> +      } else if (gDecimalSymbol != ".") {
> +        // FIXME: Fall back to the old hack, will be fixed in bug .
> +        aBytes = aBytes.replace(".", gDecimalSymbol);

should this still call toFixed(fraction.digits) like the old code?

Comment 62

[Marco Bonardo [:mak]]

s/fraction.digits/fractionDigits/

Comment 63

[(no longer active)]

Ah, yes.

Comment 64

[(no longer active)]

Attachment: Part 2: Revert to the old gDecimalSymbol hack if the Internationalization API is not available

This will be removed in bug 1200494.

Comment 65

[Marco Bonardo [:mak]]

Comment on attachment 8656685 [details] [diff] [review]
Part 2: Revert to the old gDecimalSymbol hack if the Internationalization API is not available

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

Thank you!

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +486,5 @@
> +      if (Intl) {
> +        aBytes = getLocaleNumberFormat(fractionDigits)
> +                   .format(aBytes);
> +      } else if (gDecimalSymbol != ".") {
> +        // FIXME: Fall back to the old hack, will be fixed in bug .

the comment is missing the bug number

Comment 66

[(no longer active)]

Comment on attachment 8656685 [details] [diff] [review]
Part 2: Revert to the old gDecimalSymbol hack if the Internationalization API is not available

This is a follow-up from the previous patch to make this work on Android.

Comment 67

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf97e4f6479

Comment 68

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fcf97e4f6479

Comment 69

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8656685 [details] [diff] [review]
Part 2: Revert to the old gDecimalSymbol hack if the Internationalization API is not available

Next time, please open a new bug instead... This is harder for both relman & sheriffs.

Anyway, taking it.

Comment 70

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93acdc6a833d