Closed
Bug 1385402
Opened 7 years ago
Closed 7 years ago
Cloud Storage API - Handle null while getting download folder
Categories
(Firefox :: General, defect)
Firefox
General
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.
Assignee | ||
Comment 1•7 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•7 years ago
|
Assignee: nobody → pdahiya
Updated•7 years ago
|
Blocks: 1357171
status-firefox56:
--- → affected
Assignee | ||
Comment 2•7 years ago
|
||
Link to try server https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef6d3e493a78355f6aa764027ad26c9de277157c
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 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•7 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•7 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•7 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•7 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•7 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
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/efc1451e4b0b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Assignee | ||
Comment 13•7 years ago
|
||
Link to try server https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e6626fb2bc53a7b920ac0719a5f2e1c09e1a716
Assignee | ||
Updated•7 years ago
|
Summary: Cloud Storage API - getDownloadFolder to always return valid download path → Cloud Storage API - Handle null while getting download folder
You need to log in
before you can comment on or make changes to this bug.
Description
•