1245639 - Implement chrome.downloads.show()

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

Part of supporting chrome.downloads in WebExtensions

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

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v1

Comment 2

[Andreas Wagner [:TheOne] [use NI]]

I tested this successfully on

* Mac 10.11.3
* Windows 7
* Ubuntu 14.04 LTS

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8730128 [details] [diff] [review]
patch v1

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

::: toolkit/components/extensions/ext-downloads.js
@@ +458,5 @@
>        },
>  
> +      show(downloadId) {
> +        DownloadMap.lazyInit().then(() => {
> +          let download = DownloadMap.fromId(downloadId);

We need to check whether the extension object is valid, and that the target file exists (and tests that it fails when it doesn't).

We should probably also change this to return a promise, for the cases where it fails.

@@ +460,5 @@
> +      show(downloadId) {
> +        DownloadMap.lazyInit().then(() => {
> +          let download = DownloadMap.fromId(downloadId);
> +          let fileobj = new FileUtils.File(download.filename);
> +          fileobj.reveal();

We should be using the Download object's `showContainingDirectory` method for this.

Comment 4

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v2

Comment 5

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Kris Maglione [:kmag] from comment #3)
> Comment on attachment 8730128 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8730128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/extensions/ext-downloads.js
> @@ +458,5 @@
> >        },
> >  
> > +      show(downloadId) {
> > +        DownloadMap.lazyInit().then(() => {
> > +          let download = DownloadMap.fromId(downloadId);
> 
> We need to check whether the extension object is valid, and that the target
> file exists (and tests that it fails when it doesn't).

`fromId` already checks whether the download object is valid.
`showContainingDirectory` already checks whether the target file exists.

> 
> We should probably also change this to return a promise, for the cases where
> it fails.
> 
> @@ +460,5 @@
> > +      show(downloadId) {
> > +        DownloadMap.lazyInit().then(() => {
> > +          let download = DownloadMap.fromId(downloadId);
> > +          let fileobj = new FileUtils.File(download.filename);
> > +          fileobj.reveal();
> 
> We should be using the Download object's `showContainingDirectory` method
> for this.

Done.

Comment 6

[Andreas Wagner [:TheOne] [use NI]]

> > We should be using the Download object's `showContainingDirectory` method
> > for this.
> 
> Done.

Wait, `download.showContainingDirectory` does not exist.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Andreas Wagner [:TheOne] from comment #5)
> `fromId` already checks whether the download object is valid.
> `showContainingDirectory` already checks whether the target file exists.

They do, but the errors aren't handled, and are just eventually reported as a
promise that failed to handle rejection. They should be handled and reported
as extension errors.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Andreas Wagner [:TheOne] from comment #6)
> > > We should be using the Download object's `showContainingDirectory` method
> > > for this.
> > 
> > Done.
> 
> Wait, `download.showContainingDirectory` does not exist.

It should. https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#729

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v3

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8730250 [details] [diff] [review]
patch v3

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

This needs test coverage for the error states (i.e., the download ID is invalid, the file doesn't exist, or the download isn't complete). Please file a follow-up bug.

Comment 11

[Andreas Wagner [:TheOne] [use NI]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2883faa4f9a3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 13

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v4 - rebased

Rebased.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dac4bc7e3d23

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dac4bc7e3d23