Closed Bug 1245639 Opened 8 years ago Closed 8 years ago

Implement chrome.downloads.show()

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: aswan, Assigned: TheOne)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 3 obsolete files)

Part of supporting chrome.downloads in WebExtensions

https://developer.chrome.com/extensions/downloads#method-show
Blocks: 1213445
Whiteboard: [downloads]
Whiteboard: [downloads] → [downloads][good first bug]
Assignee: nobody → awagner
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8730128 - Flags: review?(kmaglione+bmo)
I tested this successfully on

* Mac 10.11.3
* Windows 7
* Ubuntu 14.04 LTS
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.
Attachment #8730128 - Flags: review?(kmaglione+bmo)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8730128 - Attachment is obsolete: true
(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.
Attachment #8730213 - Attachment description: Implement chrome.downloads.show → patch v2
Attachment #8730213 - Flags: review?(kmaglione+bmo)
> > We should be using the Download object's `showContainingDirectory` method
> > for this.
> 
> Done.

Wait, `download.showContainingDirectory` does not exist.
(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.
(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
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8730250 - Flags: review?(kmaglione+bmo)
Attachment #8730213 - Attachment is obsolete: true
Attachment #8730213 - Flags: review?(kmaglione+bmo)
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.
Attachment #8730250 - Flags: review?(kmaglione+bmo) → review+
Blocks: 1256691
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Rebased.
Attachment #8730250 - Attachment is obsolete: true
Attachment #8732059 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dac4bc7e3d23
Status: ASSIGNED → RESOLVED
Closed: 8 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.