Store the date and time of downloads

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Paolo, Assigned: raymondlee)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 744523 [details] [diff] [review]
v1

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)
(Assignee)

Updated

5 years ago
Attachment #744523 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

5 years ago
Assignee: nobody → raymond
(Assignee)

Comment 2

5 years ago
Created attachment 744901 [details] [diff] [review]
v2
Attachment #744523 - Attachment is obsolete: true
Attachment #744523 - Flags: review?(paolo.mozmail)
Attachment #744901 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
Created attachment 747318 [details] [diff] [review]
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?
Attachment #744901 - Attachment is obsolete: true
Attachment #747318 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 5

5 years ago
(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());
(Reporter)

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 747800 [details] [diff] [review]
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
Attachment #747318 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=92fecfd7dc92
(Assignee)

Comment 9

5 years ago
Created attachment 747802 [details] [diff] [review]
Patch for checkin

Just updated the patch description
Attachment #747800 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 748037 [details] [diff] [review]
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
(Assignee)

Updated

5 years ago
Attachment #747802 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/36401ffaee14
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.