1256269 - Support endTime in DownloadItem

Status: NEW

(WebExtensions :: Request Handling, defect, P5)

Description

[Andrew Swan [:aswan]]

These are currently missing, fill them in.
Note that we need separate implementations for downloads from the current session and from history (see bug 1255507)

Comment 1

[jscher2000]

With the estimated time to completion removed from the downloads toolbar button in Firefox 54+, many users have expressed their sense of loss in not having this information easily visible (without having to open the downloads panel). Persistently displaying the time remaining could potentially be a task for an extension, but appears to require estimatedEndTime to compute. I don't know whether this UI change and potential use for this property could/should raise the priority of this bug.

Comment 2

[Thomas Wisniewski]

Andrew, would it be alright for this ticket to just be about adding support for the session download backend and defer the rest to bug 1255507? Or should we mark this as blocked by bug 1255507 instead?

Comment 3

[Andrew Swan [:aswan]]

Definitely yes to the first question.  I don't think we actually have endTime for records in places (and estimatedEndTime doesn't apply for them obviously) so I don't think we should exert a lot of effort on those.

Comment 4

[Thomas Wisniewski]

Attachment: Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

Review commit: https://reviewboard.mozilla.org/r/157586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157586/

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8886832 [details]
Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

https://reviewboard.mozilla.org/r/157586/#review162996

This looks good with the comments below addressed but please also get a review from :paolo for the DownloadCore changes.

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_misc.js:303
(Diff revision 1)
>    do_print(url);
>    let msg = await runInExtension("download", {url});
>    equal(msg.status, "success", "download() succeeded");
>    const id = msg.result;
>  
> -  let progressPromise = waitForProgress(url, INT_PARTIAL_LEN);
> +  let progressPromise = waitForProgress(url);

you're changing the meaning of the existing test here.   can you replace the `bytes` parameter to `waitForProgress()` with a callable that takes the download's `currentBytes` as a paramter, and then this code can pass in `bytes => bytes == INT_PARTIAL_LEN` and the new test can use can pass `bytes => bytes > previousBytes` or something

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_misc.js:358
(Diff revision 1)
>    equal(msg.status, "success", "search() succeeded");
>    equal(msg.result.length, 1, "search() found 1 download");
>    equal(msg.result[0].id, id, "download.id is correct");
>    equal(msg.result[0].state, "interrupted", "download.state is correct");
>    equal(msg.result[0].paused, false, "download.paused is correct");
> +  ok(msg.result[0].endTime, "download.endTime is correct");

Can you make this more specific, ie `msg.result[0].endTime instanceof Date`
Likewise with the similar instances below

::: toolkit/components/extensions/test/xpcshell/test_ext_downloads_misc.js:878
(Diff revision 1)
>  
>    webNav.close();
>  });
>  
> +add_task(async function test_startendtimes() {
> +  let url = getInterruptibleUrl() + "&stream=1";

please use a template string

Comment 6

[Thomas Wisniewski]

Comment on attachment 8886832 [details]
Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157586/diff/1-2/

Comment 7

[:Paolo Amadini]

Comment on attachment 8886832 [details]
Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

https://reviewboard.mozilla.org/r/157586/#review163382

I was previously against storing endTime in the Downloads API because it just adds more behavior specifications, more code, and more tests to maintain.

Now that we have to implement chrome.downloads the way Chrome implemented it, I'm fine with adding endTime despite the added complexity. It will also help some of the work I'm doing in bug 1381411 to create a DownloadHistoryList object.

Please file a different Downloads API bug to get this functionality implemented. There are a number of issues that should be addressed, I suggest copying or referencing this comment in the new bug.

1. All the new production code should be covered by a Downloads API regression test in different relevant scenarios.

2. The chrome.downloads endTime API is vastly underspecified in the documentation, especially in which states it's supposed to be null. I see that there are regression tests for WebExtensions code, did you run test cases on Chrome to inform these? Proceeding without testing Chrome might be faster, but we should note that we may end up having to rewrite the code and tests as compatibility bugs arise. It's also true that endTime incompatibilities are unlikely to affect add-ons in a serious way, unless they expect it to be set and it's null, breaking code execution.

We should have Downloads API regression tests for these cases:
- Never started
- In progress for the first time
- Cancelled by the user permanently
- Cancelled by the user while keeping partial data (aka paused)
- Failed
- Blocked permanently
- Blocked temporarily
- Unblocked after being blocked temporarily
- In progress again after having been paused, failed, or cancelled
- Completed after being in progress for the second time

You can likely reuse most of the existing test cases and add checks to them.

3. We need serialization of this property across sessions. We also need to provide the right value when deserializing downloads from previous sessions that didn't have the endTime set.

4. We should check that the following front-end code will be compatible, and then remove it, since we shouldn't set endTime externally anymore:

https://dxr.mozilla.org/mozilla-central/rev/e0b0865639cebc1b5afa0268a4b073fcdde0e69c/browser/components/downloads/DownloadsCommon.jsm#738-742

Note how a few lines above this code we set the endTime to an arbitrary value when the download is added to the list. We should make sure that the rest of the front-end code can deal with a null endTime.

::: toolkit/components/extensions/ext-downloads.js:67
(Diff revision 2)
> -  get endTime() { return null; } // TODO
> -  get estimatedEndTime() { return null; } // TODO
> +  get endTime() {
> +    if (this.download.endTime &&
> +        (this.download.succeeded || this.download.canceled ||
> +         this.download.stopped || this.download.error)) {
> +      return new Date(this.download.endTime);
> +    }
> +  }

Unless you need to check state explicitly for compatibility, you can probably return this.download.endTime directly. This is assuming the DownloadItem is not directly accessible by WebExtensions, and the only accessible value is the one that is converted to string in the "serialize" method.

If it is accessible in some way, then the difference between returning null and undefined is probably relevant. Note that in Firefox code the linter enforces consistent return across code paths, either always return or never return.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:509
(Diff revision 2)
>          }
>  
>          // Update the status properties for a successful download.
>          this.progress = 100;
>          this.succeeded = true;
> +        this.endTime = new Date();

This is probably unneeded because we set the value again later in this function.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:558
(Diff revision 2)
>  
>          // Update the status properties, unless a new attempt already started.
>          if (this._currentAttempt == currentAttempt || !this._currentAttempt) {
>            this._currentAttempt = null;
>            this.stopped = true;
> +          this.endTime = new Date();

If the endTime is supposed to be null while the download is in progress we should probably reset it to null explicitly when the next download attempt starts.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:652
(Diff revision 2)
>          this._promiseUnblock = null;
>          throw ex;
>        }
>  
>        this.succeeded = true;
> +      this.endTime = new Date();

We should add a code comment in the unblock interface methods, and add a test, for what happens when unblocking a temporarily blocked download. If the endTime does not change in that case, we can remove this line.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:795
(Diff revision 2)
>        // The download can already be restarted.
>        this._currentAttempt = null;
>  
>        // Notify that the cancellation request was received.
>        this.canceled = true;
> +      this.endTime = new Date();

This is also unneeded because we end up in the main code path that stops the download. If the _notifyChange missing the endTime is an issue and this code is really needed, we should add a specific test for this code path.

Comment 8

[Thomas Wisniewski]

Comment on attachment 8886832 [details]
Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

https://reviewboard.mozilla.org/r/157586/#review163382

I'm not quite sure what you mean by "blocked temporarily/permanently". Could you elaborate?

Otherwise, for the record here are my findings on how things seem to behave in Chrome's downloads extension API.

The basics: each download that's started has its own history entry. Resumed downloads have a new history entry separate from the canceled one. Pausing/resuming a download does not make a new history entry, however.

Neither endTime nor estimatedEndTime are set on a download before it actually begins, or if it fails to begin (it gets a "NETWORK_FAILED" error in that case).

endTime is not set at all until the download successfully completes or the server connection is lost (but not set when paused or canceled).

Oddly, if the server connection is lost the download's state is set to "complete" (even though the download isn't complete), with no error set and the totalBytes and fileSize reduced to the bytesReceived at the time the connection was lost. This makes it impossible to tell if the download completed successfully or the connection was lost (although I suppose you may be able to check whether an earlier event gave a higher fileSize than the end result, but that seems tenuous at best). Note that I tested this case by crashing my nginx server with kill -9 during downloads... perhaps I'm missing something there.

estimatedEndTime is not set at all except during the download, or (curiously) while it's canceled, at which point it climbs indefinitely even if the download is retried and succeeds. Paused and completed downloads have no estimatedEndTime, as expected.

I'm not sure if we want to go along with their more quirky behavior. There should be no estimatedEndTime on canceled downloads, and that if the connection is lost the fileSize/totalBytes should not be changed, and instead the download's state should be set to "interrupted" with a similar NETWORK_FAILED error. I also see no reason to not add the endTime to a canceled download as well, but I'd be willing to keep that behavior for compat. What do you guys think?

Comment 9

[:Paolo Amadini]

Comment on attachment 8886832 [details]
Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

https://reviewboard.mozilla.org/r/157586/#review163382

Blocked temporarily is used by Application Reputation, and you can test at <https://testsafebrowsing.appspot.com/>.

Blocked permanently is used by Parental Controls, and you can test by downloading <http://getstatuscode.com/450> directly.

I agree with the conclusions about not replicating some behaviors exactly. We can keep the endTime empty until the download is stopped for the first time. For canceled and failed downloads, we should probably still set the endTime in the Downloads API, because we can then use it in the history interface, but it looks like we shouldn't expose it to WebExtensions. This might help extensions that use the presence of endTime to know if a download is completed successfully.

The fact that restarting a canceled or failed download creates a new download in Chrome seems a different issue, and we may want to tackle it separately, but it might not be particularly easy.

Comment 10

[:Paolo Amadini]

Comment on attachment 8886832 [details]
Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

https://reviewboard.mozilla.org/r/157586/#review163382

Also, when a failed or canceled download is restarted we set a new startTime, and at that point we can likely reset the endTime to null. If we go for the solution above, this value wouldn't be exposed to the WebExtensions API anyways.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8886832 [details]
Bug 1256269 - Support endTime and estimatedEndTime in DownloadItem;

Clearing review for now since it looks like this is going to have a pretty big revision.

Comment 12

[Tim Nguyen :ntim]

I hit this bug when creating my extension.

Comment 13

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #12)
> I hit this bug when creating my extension.

It would be nice if it was mentioned anywhere on MDN that this isn't supported on Firefox.

Comment 14

[Tim Nguyen :ntim]

Thomas, do you think it would be possible to split estimatedEndTime into its own bug ?

It seems from to me from the review that estimatedEndTime is going to cause less trouble, and it would really be useful to have this.

Comment 15

[Thomas Wisniewski]

That sounds good, ntim. I've gone ahead and created bug 1392003 to spin off estimatedEndTime, and will adjust this patch after that lands to only deal with endTime.

Comment 16

[Irene Smith]

Changed the browser compat data for estimatedEndTime to indicate that it is not yet supported. This will need to be changed once the issue has been resolved.