Closed Bug 1200494 Opened 4 years ago Closed 3 years ago

Remove the hack added in bug 1009795

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ehsan, Assigned: JanH)

References

Details

Attachments

(2 files, 1 obsolete file)

Once the Intl API is available on Android, we need to revert the hack added in that bug to fall back to gDecimalSymbol.
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!
Attachment #8655223 - Attachment is obsolete: true
Attachment #8655223 - Flags: review?(mak77)
Depends on: 1215247
Depends on: 1215252
Assignee: nobody → jh+bugzilla
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.
Attachment #8836465 - Flags: review?(mak77) → review+
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.
Attachment #8836466 - Flags: review?(mak77) → review+
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?
Flags: needinfo?(paolo.mozmail)
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)
(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...
Flags: needinfo?(paolo.mozmail)
(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...
Pushed by mozilla@buttercookie.de:
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
https://hg.mozilla.org/mozilla-central/rev/e07ec12de6c0
https://hg.mozilla.org/mozilla-central/rev/2266ec3d71f5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1344143
You need to log in before you can comment on or make changes to this bug.