Closed Bug 1354442 Opened 7 years ago Closed 7 years ago

Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat

Categories

(Toolkit :: Downloads API, enhancement)

Product:
Toolkit
Component:
Downloads API
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1354442 - Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat.
59 bytes, text/x-review-board-request
mak
: review+
Details 
http://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadUtils.jsm#340 is the last place in our code where we use nsIScriptableDateFormat.

We should migrate it to use mozIntl.DateTimeFormat nad deprecate nsIScriptableDateFormat.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
:jfkthame - I decided to just separate the two codepaths since they started diverging too much - Intl vs non-Intl.
This depends on bug 1354445 for Services.intl.
Depends on: 1354445
No longer blocks: 1354339
Comment on attachment 8856699 [details]
Bug 1354442 - Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat.

:mak, since we got you already up to speed with the shift to mozIntl.DateTimeFormat, maybe you'd be willing to review this patch as well?

This would let us remove last use of nsIScriptableDateFormat from Firefox and handle regional settings in download utils.
Attachment #8856699 - Flags: review?(jfkthame) → review?(mak77)
Comment on attachment 8856699 [details]
Bug 1354442 - Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/128632/#review131916

::: toolkit/mozapps/downloads/DownloadUtils.jsm:359
(Diff revision 2)
> +    //
> +    // For the rest of the platform, we'll use a combination of mozIntl,
> +    // Intl API and localization.
> +    if (typeof Intl === "undefined") {
> -    // Figure out if the time is from today, yesterday, this week, etc.
> +      // Figure out if the time is from today, yesterday, this week, etc.
> -    let dateTimeCompact;
> +      let dateTimeCompact;

shadowing? I bet eslint will complain!

::: toolkit/mozapps/downloads/DownloadUtils.jsm:361
(Diff revision 2)
> +    // Intl API and localization.
> +    if (typeof Intl === "undefined") {
> -    // Figure out if the time is from today, yesterday, this week, etc.
> +      // Figure out if the time is from today, yesterday, this week, etc.
> -    let dateTimeCompact;
> +      let dateTimeCompact;
> -    if (aDate >= today) {
> +      if (aDate >= today) {
> -      // After today started, show the time
> +        dateTimeCompact = aDate.toLocaleFormat("%H:%M");

scriptableDateFormat here was returning the time according to the app locale, for example in my en-US build it returns "10:30 AM".
toLocaleFormat here always uses 24hr notation, that looks wrong. Is there a way we can detect the correct notation to use?

::: toolkit/mozapps/downloads/DownloadUtils.jsm:375
(Diff revision 2)
> -                  : aDate.toLocaleDateString(undefined, { month: "long" });
> -      let date = aDate.getDate();
> +        let date = aDate.getDate();
> -      dateTimeCompact = gBundle.formatStringFromName(gStr.monthDate, [month, date], 2);
> +        dateTimeCompact = gBundle.formatStringFromName(gStr.monthDate, [month, date], 2);
> -    }
> +      }
>  
> -    let dateTimeFull = dts.FormatDateTime("",
> +      dateTimeFull = aDate.toLocaleFormat("%x %X");

This is very different, we are moving from:
April 12, 2017 at 10:45 AM
to
12/04/2017 10:44:35

On the other side, Android doesn't use dateTimeFull:
http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/mobile/android/chrome/content/aboutDownloads.js#233
so in practice it doesn't matter.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:399
(Diff revision 2)
> +      }
> +
> +      dateTimeFull = Services.intl.createDateTimeFormat(undefined, {
> +        dateStyle: "medium",
> +        timeStyle: "short"
> +      }).format(aDate);

on my system, medium is
4/12/2017, 10:51 AM
while long is
Wednesday, April 12, 2017, 10:51 AM

The old string was:
April 12, 2017 at 10:52 AM

On the other side this is not going to cause any tangible regression, since nobody uses dateTimeFull
http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/browser/components/downloads/DownloadsViewUI.jsm#248
It's basically only used in the test.

Based on the API name (get__READABLE__Dates) I'd probably go for "full".

You should check that test_DownloadUtils.js still passes.

I also wonder if we should change and simplify getReadableDates to getReadableDate, since nobody uses the fullTime return value... It may be worth filing a bug for that.
Attachment #8856699 - Flags: review?(mak77)
Comment on attachment 8856699 [details]
Bug 1354442 - Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat.

Wow! Thanks for the review! This is great! :)

I applied your feedback. One thing I can't easily fix is the 24/12h time format for non-Intl route to stick closely to the old format.

The only thing we have is %X which takes 12/24 clock but also adds seconds. I hope it's ok!
Attachment #8856699 - Flags: review?(jfkthame) → review?(mak77)
Comment on attachment 8856699 [details]
Bug 1354442 - Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat.

https://reviewboard.mozilla.org/r/128632/#review132466

Well, I guess we can't do much better without Intl...

::: toolkit/mozapps/downloads/DownloadUtils.jsm:361
(Diff revision 5)
> +    // Intl API and localization.
> +    if (typeof Intl === "undefined") {
> +      // Figure out if the time is from today, yesterday, this week, etc.
> +      if (aDate >= today) {
> +        dateTimeCompact = aDate.toLocaleFormat("%X");
> +      } else if (today - aDate < (24 * 60 * 60 * 1000)) {

You may want to add to the top of the jsm a
const MS_PER_DAY = 24 * 60 * 60 * 1000;
and use it all around in the patch. It doesn't make sense to make all of these calculations every time.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:379
(Diff revision 5)
> +      dateTimeFull = aDate.toLocaleFormat("%x %X");
> +    } else {
> +      // Figure out if the time is from today, yesterday, this week, etc.
> -    if (aDate >= today) {
> +      if (aDate >= today) {
> -      // After today started, show the time
> -      dateTimeCompact = dts.FormatTime("",
> +        let dts = Services.intl.createDateTimeFormat(undefined, {
> +          timeStyle: "short",

trailing comma

::: toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js:115
(Diff revision 5)
> -                                                   dts.timeFormatNoSeconds,
> -                                                   2000, 12, 31, 11, 30, 0));
> +                       typeof Intl === "undefined"
> +                       ? today_11_30.toLocaleFormat("%x %X")
> +                       : cDtf(undefined, {
> +                           dateStyle: "long",
> +                           timeStyle: "short"
> +                       }).format(today_11_30));

please reindent as:
let dtf = { dateStyle: "long", timeStyle: "short" };
do_check_eq(dateTimeFull, typeof Intl === "undefined"
                          ? today_11_30.toLocaleFormat("%x %X")
                          : cDtf(undefined, dtf).format(today_11_30));
Attachment #8856699 - Flags: review?(mak77) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/773bb4c632f6
Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat. r=mak
Backed out for eslint failure at DownloadUtils.jsm:399 (trailing whitespace):

https://hg.mozilla.org/integration/autoland/rev/e3f816acb82bf963860d1d04cd10c19a170fd44f

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=91348230&repo=autoland
> /home/worker/checkouts/gecko/toolkit/mozapps/downloads/DownloadUtils.jsm:399:80 | Trailing spaces not allowed. (no-trailing-spaces)
Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ea63c7d89b7
Migrate DownloadUtils.jsm to use mozIntl.DateTimeFormat. r=mak
Sorry for that!

Re-landed, now with more trailing white spaces removed!
Flags: needinfo?(gandalf)
https://hg.mozilla.org/mozilla-central/rev/8ea63c7d89b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1356718
No longer depends on: 1356718
Blocks: 1327452
Depends on: 1383463
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: