Closed Bug 1313796 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Downloads Panel, defect, P5)

defect

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch toolkit_mozapps_downloads.patch (obsolete) — Splinter Review
Attachment #8805725 - Flags: review?(mak77)
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?
Attachment #8805725 - Flags: review?(mak77)
(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.
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?
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
Priority: -- → P5
Attached patch toolkit_mozapps_downloads.patch (obsolete) — Splinter Review
: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.
Attachment #8805725 - Attachment is obsolete: true
Attachment #8807313 - Flags: review?(mak77)
(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 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 [
Attachment #8807313 - Flags: review?(mak77) → review+
Updated patch per review comment, carrying r+ from mak.
Attachment #8807313 - Attachment is obsolete: true
Attachment #8808224 - Flags: review+
Thanks for the review!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19dfd28f8b2
Remove Date.prototype.toLocaleFormat uses in toolkits/mozapps/downloads. r=mak
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a19dfd28f8b2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.