1200494 - Remove the hack added in bug 1009795

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[(no longer active)]

Once the Intl API is available on Android, we need to revert the hack added in that bug to fall back to gDecimalSymbol.

Comment 1

[(no longer active)]

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

This will be removed in bug 1200494.

Comment 2

[(no longer active)]

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

Oops, wrong bug!

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1200494 - Part 1 - Remove Intl API fallbacks from DownloadUtils.jsm now that Fennec no longer requires it.

Every little bit of code removal helps.

Review commit: https://reviewboard.mozilla.org/r/111810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111810/

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1200494 - Part 2 - Enable toolkit download xpcshell tests on Android.

The tests are passing and at least part of the toolkit download code is actually used on Android, so we shouldn't skip them any more.

Review commit: https://reviewboard.mozilla.org/r/111872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111872/

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8836465 [details]
Bug 1200494 - Part 1 - Remove Intl API fallbacks from DownloadUtils.jsm now that Fennec no longer requires it.

https://reviewboard.mozilla.org/r/111810/#review113766

::: toolkit/mozapps/downloads/DownloadUtils.jsm:483
(Diff revision 1)
>      // Don't try to format Infinity values using NumberFormat.
>      if (aBytes === Infinity) {
>        aBytes = "Infinity";
> -    } else if (typeof Intl != "undefined") {
> +    } else {
>        aBytes = getLocaleNumberFormat(fractionDigits)
>                   .format(aBytes);

nit: While here, I don't see a reason why this code goes 2 lines, it could be onelined.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8836466 [details]
Bug 1200494 - Part 2 - Enable toolkit download xpcshell tests on Android.

https://reviewboard.mozilla.org/r/111872/#review113770

r=me, provided you get a green Try out of this, since I can't verify otherwise if this works.

Comment 7

[Marco Bonardo [:mak]]

Actually looks like all the downloads tests are disabled on Android, so I'm not sure why we'd only enable these tests, as well as I'm not sure whether there's another reason for all these tests to be disabled
http://searchfox.org/mozilla-central/search?q=skip-if+%3D+toolkit+%3D%3D+%27android%27&path=downloads

Paolo do you know anything about this decision?

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8836466 [details]
Bug 1200494 - Part 2 - Enable toolkit download xpcshell tests on Android.

https://reviewboard.mozilla.org/r/111872/#review113770

I did, otherwise I wouldn't have included this. (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83d5df3a64ada9b963144d350e50a668af580a7&filter-searchStr=Android)

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8836465 [details]
Bug 1200494 - Part 1 - Remove Intl API fallbacks from DownloadUtils.jsm now that Fennec no longer requires it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111810/diff/1-2/

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8836466 [details]
Bug 1200494 - Part 2 - Enable toolkit download xpcshell tests on Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111872/diff/1-2/

Comment 11

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #7)
> Actually looks like all the downloads tests are disabled on Android

I don't think it was deliberate. The line was added in bug 1066735. Apparently, before that bug landed, there was a central registry of tests to which the download tests had not been added.

> not sure why we'd only enable these tests

We should actually enable as many download tests as possible, if they don't fail on Android...

Comment 12

[Jan Henning [:JanH]]

(In reply to :Paolo Amadini from comment #11)
> I don't think it was deliberate. The line was added in bug 1066735.
> Apparently, before that bug landed, there was a central registry of tests to
> which the download tests had not been added.

That way my impression as well. So this means I can go ahead at least with this now - except I've noticed I still need to remove the //TODO comments in Part 1 as well. I'll fix this up and push...

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8836465 [details]
Bug 1200494 - Part 1 - Remove Intl API fallbacks from DownloadUtils.jsm now that Fennec no longer requires it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111810/diff/2-3/

Comment 14

[Jan Henning [:JanH]]

Comment on attachment 8836466 [details]
Bug 1200494 - Part 2 - Enable toolkit download xpcshell tests on Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111872/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e07ec12de6c0
Part 1 - Remove Intl API fallbacks from DownloadUtils.jsm now that Fennec no longer requires it. r=mak
https://hg.mozilla.org/integration/autoland/rev/2266ec3d71f5
Part 2 - Enable toolkit download xpcshell tests on Android. r=mak

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e07ec12de6c0
https://hg.mozilla.org/mozilla-central/rev/2266ec3d71f5