Cloud Storage API - Handle null while getting download folder

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pdahiya, Assigned: pdahiya)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
This is a follow up bug to CloudStorage API landed in bug 1357171.

Cloud Storage API getDownloadfolder returns null when it fails to find a valid provider download folder path. This results in getPreferredDownloadsDirectory inside DownloadIntegration.jsm to return null.

Consumers of getPreferredDownloadDirectory such as nsHelperAPPDialog.js doesn't support null and expect getPreferredDownloadsDirectory to return a download path
 

Steps to replicate.
1) Set browser.download.folderlist to 3
2) Set browser.download.useDownloadDir to true
3) Open link e.g. https://www.mozilla.org/en-US/firefox/all/
4) Click on any download link. If you see the prompt 'You have chosen to open' asking to choose between open and save file - select save file.
5) Download fails 

Expected:
Download should succeed with download path set to system default download folder
(Assignee)

Updated

2 years ago
Assignee: nobody → pdahiya

Updated

2 years ago
Blocks: 1357171
status-firefox56: --- → affected
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
Hi Gijs

This bug is fallout of changing return type of getDownloadFolder from nsIFile to string path. Submitting patch with the fix for review. 

Thanks!

Comment 5

2 years ago
mozreview-review
Comment on attachment 8891539 [details]
Bug 1385402 - Cloud Storage API - Handle null while getting download folder

https://reviewboard.mozilla.org/r/162662/#review168074

I don't think this is the right approach, because now we're getting the default download directory in this code as well as in DownloadIntegration.jsm, duplicating and spreading around logic. We should just update the single callsite in DownloadIntegration.jsm to check for null and use the same default as in the catch() statement there.
Attachment #8891539 - Flags: review?(gijskruitbosch+bugs) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Updated patch with review feedback. Please review. Thanks!

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b30b65016bbfbe880d8113daca2833918032195

Comment 8

2 years ago
mozreview-review
Comment on attachment 8891539 [details]
Bug 1385402 - Cloud Storage API - Handle null while getting download folder

https://reviewboard.mozilla.org/r/162662/#review168154

with the issue below fixed, r=me

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:319
(Diff revision 2)
> +          if (!directoryPath) {
> +            directoryPath = await this.getSystemDownloadsDirectory();
> +          }

This is duplicated with the catch statement.

`directoryPath` starts out null, so you can just put this if statement after the try...catch(), and empty the catch statement.
Attachment #8891539 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
(In reply to :Gijs from comment #8)
> Comment on attachment 8891539 [details]
> Bug 1385402 - Cloud Storage API - Handle null while getting download folder
> 
> https://reviewboard.mozilla.org/r/162662/#review168154
> 
> with the issue below fixed, r=me
> 
> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:319
> (Diff revision 2)
> > +          if (!directoryPath) {
> > +            directoryPath = await this.getSystemDownloadsDirectory();
> > +          }
> 
> This is duplicated with the catch statement.
> 
> `directoryPath` starts out null, so you can just put this if statement after
> the try...catch(), and empty the catch statement.

Updated patch with above feedback. Thanks Gijs for review!

Comment 11

2 years ago
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/efc1451e4b0b
Cloud Storage API - Handle null while getting download folder r=Gijs
https://hg.mozilla.org/mozilla-central/rev/efc1451e4b0b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Updated

2 years ago
Summary: Cloud Storage API - getDownloadFolder to always return valid download path → Cloud Storage API - Handle null while getting download folder
(Assignee)

Updated

2 years ago
Blocks: 1357160
You need to log in before you can comment on or make changes to this bug.