Closed Bug 1385402 Opened 7 years ago Closed 7 years ago

Cloud Storage API - Handle null while getting download folder

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: pdahiya, Assigned: pdahiya)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

      No description provided.
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: nobody → pdahiya
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 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-
Updated patch with review feedback. Please review. Thanks!

Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b30b65016bbfbe880d8113daca2833918032195
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+
(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!
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Summary: Cloud Storage API - getDownloadFolder to always return valid download path → Cloud Storage API - Handle null while getting download folder
Blocks: 1357160
You need to log in before you can comment on or make changes to this bug.