Implement chrome.downloads.show()

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aswan, Assigned: TheOne)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Part of supporting chrome.downloads in WebExtensions

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

Updated

2 years ago
Blocks: 1213445

Updated

2 years ago
Whiteboard: [downloads]
(Reporter)

Updated

2 years ago
Whiteboard: [downloads] → [downloads][good first bug]
(Assignee)

Updated

2 years ago
Assignee: nobody → awagner
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8730128 [details] [diff] [review]
patch v1
Attachment #8730128 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 2

2 years ago
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)
(Assignee)

Comment 4

2 years ago
Created attachment 8730213 [details] [diff] [review]
patch v2
(Assignee)

Updated

2 years ago
Attachment #8730128 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8730213 - Attachment description: Implement chrome.downloads.show → patch v2
Attachment #8730213 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 6

2 years ago
> > 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
(Assignee)

Comment 9

2 years ago
Created attachment 8730250 [details] [diff] [review]
patch v3
Attachment #8730250 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Blocks: 1256691
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2883faa4f9a3
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 13

2 years ago
Created attachment 8732059 [details] [diff] [review]
patch v4 - rebased

Rebased.
(Assignee)

Updated

2 years ago
Attachment #8730250 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8732059 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/dac4bc7e3d23
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dac4bc7e3d23
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.