Closed Bug 852478 Opened 7 years ago Closed 7 years ago

Store the date and time of downloads

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Paolo, Assigned: raymondlee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
I've added a property to Download object.  Please let me know if this doesn't match what this bug is asking for?
Attachment #744523 - Flags: review?(paolo.mozmail)
Attachment #744523 - Flags: review?(paolo.mozmail)
Assignee: nobody → raymond
Attached patch v2 (obsolete) — Splinter Review
Attachment #744523 - Attachment is obsolete: true
Attachment #744523 - Flags: review?(paolo.mozmail)
Attachment #744901 - Flags: review?(paolo.mozmail)
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.
Attachment #744901 - Flags: review?(paolo.mozmail)
Attached patch v3 (obsolete) — Splinter Review
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?
Attachment #744901 - Attachment is obsolete: true
Attachment #747318 - Flags: review?(paolo.mozmail)
(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 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!
Attachment #747318 - Flags: review?(paolo.mozmail) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
(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
Attachment #747318 - Attachment is obsolete: true
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=92fecfd7dc92
Attached patch Patch for checkin (obsolete) — Splinter Review
Just updated the patch description
Attachment #747800 - Attachment is obsolete: true
I've pushed to try again to ensure the oranges are intermittents.  Look fine!
https://tbpl.mozilla.org/?tree=Try&rev=732b2656f96a
Attachment #747802 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/36401ffaee14
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.