1313796 - Remove Date.prototype.toLocaleFormat uses in toolkits/mozapps/downloads

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P5)

Description

[André Bargull [:anba]]

Date.prototype.toLocaleFormat is a non-standard API and we plan to warn when it's used (bug 1299900) and eventually want to remove it completely (bug 818634). As a first step we need to replace all Date.prototype.toLocaleFormat uses in Firefox with standardized APIs.

Date.prototype.toLocaleFormat is used in toolkit/mozapps/downloads/DownloadUtils.jsm and toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js. When the Intl object is available, we can simply use the additional parameters for Date.prototype.toLocaleString to retrieve the month/week name. But when Intl isn't available (bug 1215247), JavaScript doesn't provide any standard methods to get the localized month/week name. So for the time being, toLocaleFormat still needs to be used until bug 1215247 is fixed. Nevertheless let's try to switch to the standardized APIs for those platforms which provide Intl support.

Comment 1

[André Bargull [:anba]]

Attachment: toolkit_mozapps_downloads.patch

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8805725 [details] [diff] [review]
toolkit_mozapps_downloads.patch

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

This seems to overlap with the work in bug 8804706, and it may need an unbitrot, since that one just landed 2 days ago.

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +364,5 @@
> +        return date.toLocaleFormat("%A");
> +
> +      const locale = Intl.DateTimeFormat().resolvedOptions().locale + "-u-ca-gregory";
> +      return date.toLocaleString(locale, {weekday: "long"});
> +    }

I'm not sure why we should redefine these functions every time getReadableDates is invoked.
Could you please move them to the jsm global object?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #2)
> Comment on attachment 8805725 [details] [diff] [review]
> toolkit_mozapps_downloads.patch
> 
> Review of attachment 8805725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to overlap with the work in bug 8804706

Sorry, I meant bug 1301655.

Comment 4

[Marco Bonardo [:mak]]

Hm, looks like Part 6 didn't land there to wait for bug 1215247...
should we just merge this work there and have a unique patch?

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Marco Bonardo [::mak] from comment #4)
> Hm, looks like Part 6 didn't land there to wait for bug 1215247...
> should we just merge this work there and have a unique patch?

Feel free to do that, or to go ahead here and we'll rebase the part 6 patch over there as needed - either way is fine with me.

Comment 6

[André Bargull [:anba]]

Attachment: toolkit_mozapps_downloads.patch

:gandalf told me on IRC that the UI locale instead of system locale should be used, updated the patch accordingly. Also changed the patch to be more similar to bug 1301655, part 6.

Comment 7

[André Bargull [:anba]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Marco Bonardo [::mak] from comment #4)
> > Hm, looks like Part 6 didn't land there to wait for bug 1215247...
> > should we just merge this work there and have a unique patch?
> 
> Feel free to do that, or to go ahead here and we'll rebase the part 6 patch
> over there as needed - either way is fine with me.

I went with updating this patch, because I don't know how long it'll take for bug 1215247 to land.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8807313 [details] [diff] [review]
toolkit_mozapps_downloads.patch

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

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +360,5 @@
> +    const locale = typeof Intl === "undefined"
> +                   ? undefined
> +                   : Cc["@mozilla.org/chrome/chrome-registry;1"]
> +                     .getService(Ci.nsIXULChromeRegistry)
> +                     .getSelectedLocale("global", true);

nit: please align dots with [

Comment 9

[André Bargull [:anba]]

Attachment: toolkit_mozapps_downloads.patch

Updated patch per review comment, carrying r+ from mak.

Comment 10

[André Bargull [:anba]]

Thanks for the review!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19dfd28f8b2
Remove Date.prototype.toLocaleFormat uses in toolkits/mozapps/downloads. r=mak

Comment 12

[Phil Ringnalda (:philor)]

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