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)

enhancement

Tracking

(firefox57 wontfix, firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

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
Whiteboard: [design-decision-needed]
Severity: normal → enhancement
Priority: -- → P5
Assignee: nobody → ksteuber
Status: NEW → ASSIGNED
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
Attachment #8915363 - Flags: review?(tomica)
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 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).
Attachment #8915363 - Flags: review?(tomica) → review?(kmaglione+bmo)
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+
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
Removing design-decision-needed as discussed on IRC with Kris Maglione.
Whiteboard: [design-decision-needed]
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
https://hg.mozilla.org/mozilla-central/rev/3bbb5071bd20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
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!
Flags: needinfo?(ksteuber)
I don't think manual testing is needed here. The automated testing that I added should be sufficiently thorough.
Flags: needinfo?(ksteuber) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.