Closed
Bug 1394851
Opened 6 years ago
Closed 6 years ago
Allow download API to use Firefox Save As preference
Categories
(WebExtensions :: General, enhancement, P5)
WebExtensions
General
Tracking
(firefox57 wontfix, firefox58 fixed)
RESOLVED
FIXED
mozilla58
People
(Reporter: ianbicking, Assigned: bytesized)
Details
(Keywords: addon-compat, dev-doc-complete)
Attachments
(1 file)
The download API (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/download) has a saveAs option. Firefox also has an option to default to a download location, or to ask every time. It would be preferable if the WebExtension API could saveAs or not based on the Firefox pref. This came up in a Screenshots feature request: https://github.com/mozilla-services/screenshots/issues/3420
Updated•6 years ago
|
Whiteboard: [design-decision-needed]
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ksteuber
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•6 years ago
|
||
It seems that the best way to integrate this into the API is to have this behavior be the default. That is to say, if the saveAs option is not specified, the Firefox preference will be used to determine whether the user should be prompted to choose a download path. I am happy with this option since neither our documentation [1] nor Chrome's [2] specifies a default action. If the caller does not specify, presumably they are happy with either behavior. [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/download [2] https://developer.chrome.com/extensions/downloads#method-download
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8915363 -
Flags: review?(tomica)
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8915363 [details] Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref https://reviewboard.mozilla.org/r/186554/#review192380 The code itself looks good (a few minor issues). Please update the json schema description to clarify the new behavior. Also, add the `dev-doc-needed` and probably even `addon-compat` flags to the bug. I understand this solution was proposed by Kris, but I disagree with changing the default behavior. And since this is also still marked as `design-decision-needed`, I'm reluctant to r+, but feel free to ask :kmag or :aswan for final review. ::: toolkit/components/extensions/ext-downloads.js:487 (Diff revision 1) > } > > let target = OS.Path.join(downloadsDir, filename || "download"); > > + let saveAs; > + if (options.saveAs != null) { nit: `!==` schema code normalizes to `null`, and style guide prefers `!==` ::: toolkit/components/extensions/ext-downloads.js:490 (Diff revision 1) > > + let saveAs; > + if (options.saveAs != null) { > + saveAs = options.saveAs; > + } else { > + saveAs = !Services.prefs.getBoolPref(PROMPTLESS_DOWNLOAD_PREF, false); Please add a comment line stating the intention here, the double-negative doesn't help. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:25 (Diff revision 1) > -let directory; > +const PROMPTLESS_DOWNLOAD_PREF = "browser.download.useDownloadDir"; > + > +// We need to be able to distinguish files downloaded by the file picker from > +// files downloaded without it. > +let picker_directory; > +let default_directory; nit: use camelCase in JS please, `defaultDir` and `pickerDir` ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:28 (Diff revision 1) > +// files downloaded without it. > +let picker_directory; > +let default_directory; > > add_task(async function setup() { > - directory = FileUtils.getDir("TmpD", ["downloads"]); > + let download_directory = FileUtils.getDir("TmpD", ["downloads"]); nit: ditto ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:52 (Diff revision 1) > + default_directory.remove(true); > }); > }); > > add_task(async function test_downloads_saveAs() { > - const file = directory.clone(); > + const picker_file = picker_directory.clone(); nit: you get the idea ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:106 (Diff revision 1) > const extension = ExtensionTestUtils.loadExtension(manifest); > > await extension.startup(); > await extension.awaitMessage("ready"); > > - // Test basic saveAs functionality. > + async function testWithFilePicker(saveAs) { Perhaps `expectFilePicker` would be clearer here, I was confused for a bit. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:111 (Diff revision 1) > - // Test basic saveAs functionality. > + async function testWithFilePicker(saveAs) { > - MockFilePicker.returnValue = MockFilePicker.returnOK; > + MockFilePicker.returnValue = MockFilePicker.returnOK; > > - extension.sendMessage("file_download.txt"); > + extension.sendMessage("file_download.txt", saveAs); > - let result = await extension.awaitMessage("done"); > + let result = await extension.awaitMessage("done"); > - ok(result.ok, "downloads.download() works with saveAs"); > + ok(result.ok, "downloads.download() works with saveAs=true"); This is also tested with saveAs=undefined ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:133 (Diff revision 1) > - ok(!unique.exists(), "unique file not left after SaveAs canceled."); > - file.remove(false); > + async function testNoFilePicker(saveAs) { > + extension.sendMessage("file_download.txt", saveAs); > + let result = await extension.awaitMessage("done"); > + ok(result.ok, "downloads.download() works with saveAs=false"); > > - // Test the user canceling the save dialog. > + ok(default_file.exists(), "the file exists."); Since this will be executed multiple times, how about ensuring the file doesn't exist before the download? ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_uniquify.html:4 (Diff revision 1) > +<!doctype html> > +<html> > +<head> > + <title>Test downloads.download() saveAs option</title> nit: update the title please. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_uniquify.html:37 (Diff revision 1) > + await SpecialPowers.popPrefEnv(); > + directory.remove(true); > + }); > +}); > + > +add_task(async function test_downloads_saveAs() { and test title ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_uniquify.html:91 (Diff revision 1) > + // test with uniquify. > + extension.sendMessage("file_download.txt", saveAs); > + let result = await extension.awaitMessage("done"); > + ok(result.ok, "downloads.download() works with saveAs"); > + > + ok(file.exists(), "the file exists."); Let's ensure these don't exist before the download either.
Attachment #8915363 -
Flags: review?(tomica)
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8915363 [details] Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref https://reviewboard.mozilla.org/r/186554/#review192404 ::: toolkit/components/extensions/ext-downloads.js:490 (Diff revision 1) > > + let saveAs; > + if (options.saveAs != null) { > + saveAs = options.saveAs; > + } else { > + saveAs = !Services.prefs.getBoolPref(PROMPTLESS_DOWNLOAD_PREF, false); Also, I'm not sure if this should default to false, since the current default in firefox is true (which at least means this will not change the extension behavior for most users).
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8915363 -
Flags: review?(tomica) → review?(kmaglione+bmo)
Assignee | ||
Updated•6 years ago
|
Keywords: addon-compat,
dev-doc-needed
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8915363 [details] Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref https://reviewboard.mozilla.org/r/186554/#review194262 ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html:145 (Diff revision 2) > + info("Testing that saveAs=false does not use the file picker"); > + saveAs = false; > + await testNoFilePicker(saveAs); > + > + SimpleTest.registerCleanupFunction(async () => { > + Services.prefs.clearUserPref(PROMPTLESS_DOWNLOAD_PREF); Please use `SpecialPowers.pushPrefEnv` and `popPrefEnv` for these kinds of changes. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_uniquify.html:1 (Diff revision 2) > +<!doctype html> Please use `hg cp` for this.
Attachment #8915363 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48dc5090a52ec12da84ccf564568a018d0e24e10
Assignee | ||
Comment 8•6 years ago
|
||
When I went to re-run the tests before uploading my revised patch, I ran them on a debug build this time and could not get them to pass. However, I couldn't get the existing version of the test to pass either, so I don't think that I broke it exactly. When running locally in a release build, they both run fine. I also ran them through Try in both modes [1] and they seem to pass fine there. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6136aea06c141701a6e4b2c15d396979593e143f&group_state=expanded&selectedJob=136905122
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
Removing design-decision-needed as discussed on IRC with Kris Maglione.
Whiteboard: [design-decision-needed]
Comment 11•6 years ago
|
||
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3bbb5071bd20 downloads.download API should default to use Firefox's "Save As" pref r=kmag
![]() |
||
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3bbb5071bd20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 13•6 years ago
|
||
I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/download to clarify that downloads.download should respect the preference if saveAs is omitted, and that Firefox before version 58 did not so this. Marking dev-doc-complete, but let me know if you need anything else here.
Keywords: dev-doc-needed → dev-doc-complete
Comment 14•6 years ago
|
||
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag. Thanks!
Updated•6 years ago
|
Flags: needinfo?(ksteuber)
Assignee | ||
Comment 15•6 years ago
|
||
I don't think manual testing is needed here. The automated testing that I added should be sufficiently thorough.
Flags: needinfo?(ksteuber) → qe-verify-
Updated•5 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•