852478 - Store the date and time of downloads

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

In the new API, downloads should have an associated date and time, to be used
for example by the Downloads Panel for reference. Currently, the Downloads
Panel only shows the end date and time of completed downloads.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

I've added a property to Download object.  Please let me know if this doesn't match what this bug is asking for?

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 3

[:Paolo Amadini]

Comment on attachment 744901 [details] [diff] [review]
v2

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

Looks good overall!

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +131,5 @@
>    /**
> +   * Indicates that the start time of the download.  When the download starts,
> +   * this property is set to a valid Date object.
> +   */
> +  startTime: null,

Typo: "Indicates the". Also, we should be more explicit in the comment, because it works as an interface documentation, and readers won't necessarily see from the code that the default value is null.

So, we must indicate that null is the initial value on creation, that the current date is set when the download is started, and that the value may change in case the download is later restarted by calling the "start" method again after the download failed or was canceled.

We also need a new test for this:
 - Start a simple download
 - Verify the start date and memorize it
 - Cancel it immediately
 - Wait for the current date to change (avoids intermittent failures)
   - A simple solution involving a timeout of 20ms may be sufficient
 - Start the download again
 - Verify the start date and check that it is different

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +57,5 @@
>    do_check_false(download.canceled);
>    do_check_true(download.error === null);
>    do_check_eq(download.progress, 100);
> +  do_check_true(download.startTime &&
> +                !isNaN(new Date(download.startTime.getTime())));

You may just use "(value instanceof Date) && !isNaN(value)".

For clarity, can also be factored into a function "isValidDate" in head.js.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v3

I have found an odd issue.  I've tried to add (aDate instanceof Date) to the check in the isValidDate() in head.js, however, download.startTime always return false.  Any ideas?

Comment 5

[:Paolo Amadini]

(In reply to Raymond Lee [:raymondlee] from comment #4)
> I have found an odd issue.  I've tried to add (aDate instanceof Date) to the
> check in the isValidDate() in head.js, however, download.startTime always
> return false.  Any ideas?

Oh, that may be related to the Date prototype in the JSM not being the exact same
as the one in the test JS file. I tested the code in the JavaScript console where
this problem was not present. So, I guess we're stuck with checking:

aDate && aDate.getTime && !isNaN(aDate.getTime());

Comment 6

[:Paolo Amadini]

Comment on attachment 747318 [details] [diff] [review]
v3

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

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +695,5 @@
> +  do_check_eq(download.startTime.getTime(), startTime.getTime());
> +
> +  // Wait for a delay, restart the download and verify the start time.
> +  do_timeout(10, function() {
> +    Task.spawn(function() {

To avoid that the following test, if present, runs before this test finishes, you should create a promiseTimout function in head.js, on the model of promiseExecuteSoon, and wait on it in the main task, instead of calling do_timeout and creating a different task.

Looks good for the rest!

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

(In reply to Paolo Amadini [:paolo] from comment #6)
> Comment on attachment 747318 [details] [diff] [review]
> v3
> 
> Review of attachment 747318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
> @@ +695,5 @@
> > +  do_check_eq(download.startTime.getTime(), startTime.getTime());
> > +
> > +  // Wait for a delay, restart the download and verify the start time.
> > +  do_timeout(10, function() {
> > +    Task.spawn(function() {
> 
> To avoid that the following test, if present, runs before this test
> finishes, you should create a promiseTimout function in head.js, on the
> model of promiseExecuteSoon, and wait on it in the main task, instead of
> calling do_timeout and creating a different task.
> 
Fixed

Comment 8

[Raymond Lee [:raymondlee]]

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=92fecfd7dc92

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Just updated the patch description

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: Patch for check in

I've pushed to try again to ensure the oranges are intermittents.  Look fine!
https://tbpl.mozilla.org/?tree=Try&rev=732b2656f96a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36401ffaee14

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/36401ffaee14