1245636 - Implement chrome.downloads.open()

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

Part of supporting chrome.downloads in WebExtensions

https://developer.chrome.com/extensions/downloads#method-open

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: Implement chrome.downloads.open

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Mark Striemer [:mstriemer]]

Attachment: MozReview Request: bug 1245636 - Implement chrome.downloads.open r?kmag

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

Comment 4

[Mark Striemer [:mstriemer]]

As discussed in person, Chrome does not actually fire the `onChanged` event so this does not fire it either.

Comment 5

[Kris Maglione [:kmag]]

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 6

[Mark Striemer [:mstriemer]]

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/

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 8

[Mark Striemer [:mstriemer]]

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/

Comment 9

[Mark Striemer [:mstriemer]]

Rebased.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Sorry to do this to you, but this needs rebasing against fx-team again :(

Comment 11

[Mark Striemer [:mstriemer]]

Rebased again, fingers crossed.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/058cc8ccfff3

Comment 13

[Mark Striemer [:mstriemer]]

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/

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/058cc8ccfff3