Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Support endTime and estimatedEndTime in DownloadItem

NEW
Unassigned

Status

()

Toolkit
WebExtensions: Request Handling
P5
normal
a year ago
3 days ago

People

(Reporter: aswan, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [downloads]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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)
(Reporter)

Updated

a year ago
Whiteboard: [downloads]

Updated

a year ago
Priority: -- → P4
Whiteboard: [downloads] → [downloads]triaged

Updated

9 months ago
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Priority: P4 → P5

Comment 1

28 days ago
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

8 days ago
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?
Flags: needinfo?(aswan)
(Reporter)

Comment 3

8 days ago
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.
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

4 days ago
mozreview-review
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
Attachment #8886832 - Flags: review?(aswan) → review-
Comment hidden (mozreview-request)

Comment 7

3 days ago
mozreview-review
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.
Attachment #8886832 - Flags: review?(paolo.mozmail) → review-
You need to log in before you can comment on or make changes to this bug.