Part of supporting chrome.downloads in WebExtensions https://developer.chrome.com/extensions/downloads#method-open

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.

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

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

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/

Needs rebasing.

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.

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

Rebased again, fingers crossed.

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/