Closed Bug 1245636 Opened 4 years ago Closed 4 years ago

Implement chrome.downloads.open()

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: aswan, Assigned: mstriemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [downloads][good first bug])

Attachments

(1 file, 1 obsolete file)

Part of supporting chrome.downloads in WebExtensions

https://developer.chrome.com/extensions/downloads#method-open
Blocks: 1213445
Whiteboard: [downloads]
Whiteboard: [downloads] → [downloads][good first bug]
Assignee: nobody → mstriemer
Status: NEW → ASSIGNED
Attached patch Implement chrome.downloads.open (obsolete) — Splinter Review
Attachment #8730130 - Flags: review?(kmaglione+bmo)
Comment on attachment 8730130 [details] [diff] [review]
Implement chrome.downloads.open

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

This looks good, but we need some tests, and some additional error checking.

::: toolkit/components/extensions/ext-downloads.js
@@ +458,5 @@
>        },
>  
> +      open(downloadId) {
> +        if (!extension.hasPermission("downloads.open")) {
> +          throw new context.cloneScope.Error("Permission denied because 'downloads.open' permission is missing.");

We need a test that this permission is enforced.

@@ +461,5 @@
> +        if (!extension.hasPermission("downloads.open")) {
> +          throw new context.cloneScope.Error("Permission denied because 'downloads.open' permission is missing.");
> +        }
> +        DownloadMap.lazyInit().then(() => {
> +          let download = DownloadMap.fromId(downloadId);

We need to handle cases where the download ID is invalid, the download is incomplete, or the file doesn't exist.

We should also change this API to return a promise, since the Chrome API requires us to set `lastError` if this fails.

@@ +463,5 @@
> +        }
> +        DownloadMap.lazyInit().then(() => {
> +          let download = DownloadMap.fromId(downloadId);
> +          let fileobj = new FileUtils.File(download.filename);
> +          fileobj.launch();

We should be calling the Download object's launch() method instead of creating a nsIFile object.

We also need to fire an `onChanged` event for this, apparently. We probably need to do the same if it's opened from the ordinary download manager.
Attachment #8730130 - Flags: review?(kmaglione+bmo)
Attachment #8730130 - Attachment is obsolete: true
As discussed in person, Chrome does not actually fire the `onChanged` event so this does not fire it either.
Attachment #8730253 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8730253 [details]
MozReview Request: bug 1245636 - Implement chrome.downloads.open r?kmag

https://reviewboard.mozilla.org/r/39779/#review36601

::: toolkit/components/extensions/ext-downloads.js:469
(Diff revision 1)
> +        }
> +        return DownloadMap.lazyInit().then(() => {
> +          try {
> +            let download = DownloadMap.fromId(downloadId).download;
> +            if (download.succeeded) {
> +              download.launch();

This returns a promise, so you need to return it.

::: toolkit/components/extensions/ext-downloads.js:473
(Diff revision 1)
> +            if (download.succeeded) {
> +              download.launch();
> +            } else {
> +              return Promise.reject({message: "Download has not completed."});
> +            }
> +          } catch (error) {

Please make this a `.catch()` clause rather than a try-catch statement.
Comment on attachment 8730253 [details]
MozReview Request: bug 1245636 - Implement chrome.downloads.open r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39779/diff/1-2/
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Comment on attachment 8730253 [details]
MozReview Request: bug 1245636 - Implement chrome.downloads.open r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39779/diff/2-3/
Rebased.
Keywords: checkin-needed
Sorry to do this to you, but this needs rebasing against fx-team again :(
Keywords: checkin-needed
Rebased again, fingers crossed.
Keywords: checkin-needed
Comment on attachment 8730253 [details]
MozReview Request: bug 1245636 - Implement chrome.downloads.open r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39779/diff/3-4/
https://hg.mozilla.org/mozilla-central/rev/058cc8ccfff3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.