Cloud Storage API - Handle null while getting download folder

RESOLVED FIXED in Firefox 56

Status

()

Firefox
General
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: pdahiya, Assigned: pdahiya)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 months 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

4 months ago
Assignee: nobody → pdahiya

Updated

4 months ago
Blocks: 1357171
status-firefox56: --- → affected
(Assignee)

Comment 2

4 months ago
Link to try server

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef6d3e493a78355f6aa764027ad26c9de277157c
Comment hidden (mozreview-request)
(Assignee)

Comment 4

4 months 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

4 months 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

4 months ago
Updated patch with review feedback. Please review. Thanks!

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

Comment 8

4 months 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

4 months 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

4 months 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: 4 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Comment 13

4 months ago
Link to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e6626fb2bc53a7b920ac0719a5f2e1c09e1a716
(Assignee)

Updated

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

Updated

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