1394851 - Allow download API to use Firefox Save As preference

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P5)

Description

[Ian Bicking (:ianbicking)]

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

Comment 1

[Robin Steuber (they/them) [:bytesized]]

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 2

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref

This patch changes the behavior of the downloads.download API's saveAs option. Previously, the behavior when neither value is provided (the "default behavior") was not defined by our documentation or Chrome's. Now, the default behavior is changing to rely on the Firefox "Save As" pref (browser.download.useDownloadDir). If Firefox is configured to open the "Save As" dialog for all downloads (browser.download.useDownloadDir == false), that behavior will be the default for the downloads.download API. Otherwise, the default behavior will be not to show the dialog.

This patch also moves some test functionality out of test_chrome_ext_downloads_saveAs.html. Previously, that test would test the saveAs option and also the conflictAction:"uniquify" option. In order to add testing for the new default behavior, it was necessary to move the testing of the conflictAction:"uniquify" option to a new test: test_chrome_ext_downloads_uniquify.html

Review commit: https://reviewboard.mozilla.org/r/186554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186554/

Comment 3

[Tomislav Jovanovic :zombie]

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.

Comment 4

[Tomislav Jovanovic :zombie]

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 5

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8915363 [details]
Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186554/diff/1-2/

Comment 6

[Kris Maglione [:kmag]]

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.

Comment 7

[Robin Steuber (they/them) [:bytesized]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48dc5090a52ec12da84ccf564568a018d0e24e10

Comment 8

[Robin Steuber (they/them) [:bytesized]]

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 9

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8915363 [details]
Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186554/diff/2-3/

Comment 10

[Robin Steuber (they/them) [:bytesized]]

Removing design-decision-needed as discussed on IRC with Kris Maglione.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bbb5071bd20
downloads.download API should default to use Firefox's "Save As" pref r=kmag

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3bbb5071bd20

Comment 13

[Will Bamberg [:wbamberg]]

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.

Comment 14

[Victor Carciu]

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!

Comment 15

[Robin Steuber (they/them) [:bytesized]]

I don't think manual testing is needed here. The automated testing that I added should be sufficiently thorough.