Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref r=kmag
authorKirk Steuber <ksteuber@mozilla.com>
Wed, 04 Oct 2017 14:39:58 -0700
changeset 386231 3bbb5071bd20ddf96205ad8bc1888e8647591a2a
parent 386230 17fbd19e43609cb60e98328433eebfca19633224
child 386232 6d8203bb08166d32c1e73b52e1d0bac4e5bf1702
push id53299
push userksteuber@mozilla.com
push dateFri, 13 Oct 2017 22:30:55 +0000
treeherderautoland@3bbb5071bd20 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1394851
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1394851 - downloads.download API should default to use Firefox's "Save As" pref r=kmag 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 MozReview-Commit-ID: u6VA4kexlr
toolkit/components/extensions/ext-downloads.js
toolkit/components/extensions/schemas/downloads.json
toolkit/components/extensions/test/mochitest/chrome.ini
toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_uniquify.html
--- a/toolkit/components/extensions/ext-downloads.js
+++ b/toolkit/components/extensions/ext-downloads.js
@@ -41,16 +41,18 @@ const DOWNLOAD_ITEM_CHANGE_FIELDS = ["en
 const FORBIDDEN_HEADERS = ["ACCEPT-CHARSET", "ACCEPT-ENCODING",
                            "ACCESS-CONTROL-REQUEST-HEADERS", "ACCESS-CONTROL-REQUEST-METHOD",
                            "CONNECTION", "CONTENT-LENGTH", "COOKIE", "COOKIE2", "DATE", "DNT",
                            "EXPECT", "HOST", "KEEP-ALIVE", "ORIGIN", "REFERER", "TE", "TRAILER",
                            "TRANSFER-ENCODING", "UPGRADE", "VIA"];
 
 const FORBIDDEN_PREFIXES = /^PROXY-|^SEC-/i;
 
+const PROMPTLESS_DOWNLOAD_PREF = "browser.download.useDownloadDir";
+
 class DownloadItem {
   constructor(id, download, extension) {
     this.id = id;
     this.download = download;
     this.extension = extension;
     this.prechange = {};
   }
 
@@ -476,42 +478,53 @@ this.downloads = class extends Extension
               let uri = Services.io.newURI(options.url);
               if (uri instanceof Ci.nsIURL) {
                 filename = DownloadPaths.sanitize(uri.fileName);
               }
             }
 
             let target = OS.Path.join(downloadsDir, filename || "download");
 
+            let saveAs;
+            if (options.saveAs !== null) {
+              saveAs = options.saveAs;
+            } else {
+              // If options.saveAs was not specified, only show the file chooser
+              // if |browser.download.useDownloadDir == false|. That is to say,
+              // only show the file chooser if Firefox normally shows it when
+              // a file is downloaded.
+              saveAs = !Services.prefs.getBoolPref(PROMPTLESS_DOWNLOAD_PREF, true);
+            }
+
             // Create any needed subdirectories if required by filename.
             const dir = OS.Path.dirname(target);
             await OS.File.makeDir(dir, {from: downloadsDir});
 
             if (await OS.File.exists(target)) {
               // This has a race, something else could come along and create
               // the file between this test and them time the download code
               // creates the target file.  But we can't easily fix it without
               // modifying DownloadCore so we live with it for now.
               switch (options.conflictAction) {
                 case "uniquify":
                 default:
                   target = DownloadPaths.createNiceUniqueFile(new FileUtils.File(target)).path;
-                  if (options.saveAs) {
+                  if (saveAs) {
                     // createNiceUniqueFile actually creates the file, which
                     // is premature if we need to show a SaveAs dialog.
                     await OS.File.remove(target);
                   }
                   break;
 
                 case "overwrite":
                   break;
               }
             }
 
-            if (!options.saveAs) {
+            if (!saveAs) {
               return target;
             }
 
             // Setup the file picker Save As dialog.
             const picker = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
             const window = Services.wm.getMostRecentWindow("navigator:browser");
             picker.init(window, null, Ci.nsIFilePicker.modeSave);
             picker.displayDirectory = new FileUtils.File(dir);
--- a/toolkit/components/extensions/schemas/downloads.json
+++ b/toolkit/components/extensions/schemas/downloads.json
@@ -382,17 +382,17 @@
                 "default": false,
                 "type": "boolean"
               },
               "conflictAction": {
                 "$ref": "FilenameConflictAction",
                 "optional": true
               },
               "saveAs": {
-                "description": "Use a file-chooser to allow the user to select a filename.",
+                "description": "Use a file-chooser to allow the user to select a filename. If the option is not specified, the file chooser will be shown only if the Firefox \"Always ask you where to save files\" option is enabled (i.e. the pref <code>browser.download.useDownloadDir</code> is set to <code>false</code>).",
                 "optional": true,
                 "type": "boolean"
               },
               "method": {
                 "description": "The HTTP method to use if the URL uses the HTTP[S] protocol.",
                 "enum": [
                   "GET",
                   "POST"
--- a/toolkit/components/extensions/test/mochitest/chrome.ini
+++ b/toolkit/components/extensions/test/mochitest/chrome.ini
@@ -15,16 +15,17 @@ tags = webextensions in-process-webexten
 
 [test_chrome_ext_background_page.html]
 skip-if = (toolkit == 'android') # android doesn't have devtools
 [test_chrome_ext_contentscript_data_uri.html]
 [test_chrome_ext_contentscript_telemetry.html]
 [test_chrome_ext_contentscript_unrecognizedprop_warning.html]
 [test_chrome_ext_downloads_open.html]
 [test_chrome_ext_downloads_saveAs.html]
+[test_chrome_ext_downloads_uniquify.html]
 [test_chrome_ext_eventpage_warning.html]
 [test_chrome_ext_idle.html]
 [test_chrome_ext_identity.html]
 skip-if = os == 'android' # unsupported.
 [test_chrome_ext_permissions.html]
 skip-if = os == 'android' # Bug 1350559
 [test_chrome_ext_storage_cleanup.html]
 [test_chrome_ext_trackingprotection.html]
--- a/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
+++ b/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
@@ -11,60 +11,78 @@
 </head>
 <body>
 
 <script type="text/javascript">
 "use strict";
 
 Cu.import("resource://gre/modules/FileUtils.jsm");
 
-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 pickerDir;
+let defaultDir;
 
 add_task(async function setup() {
-  directory = FileUtils.getDir("TmpD", ["downloads"]);
-  directory.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
-  info(`Using download directory ${directory.path}`);
+  let downloadDir = FileUtils.getDir("TmpD", ["downloads"]);
+  pickerDir = downloadDir.clone();
+  pickerDir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
+  info(`Using file picker download directory ${pickerDir.path}`);
+  defaultDir = downloadDir.clone();
+  defaultDir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
+  info(`Using default download directory ${defaultDir.path}`);
+
+  isnot(pickerDir.path, defaultDir.path,
+        "Should be able to distinguish between files saved with or without the file picker");
 
   await SpecialPowers.pushPrefEnv({"set": [
     ["browser.download.folderList", 2],
-    ["browser.download.dir", directory.path],
+    ["browser.download.dir", defaultDir.path],
   ]});
 
   SimpleTest.registerCleanupFunction(async () => {
     await SpecialPowers.popPrefEnv();
-    directory.remove(true);
+    pickerDir.remove(true);
+    defaultDir.remove(true);
   });
 });
 
 add_task(async function test_downloads_saveAs() {
-  const file = directory.clone();
-  file.append("file_download.txt");
+  const pickerFile = pickerDir.clone();
+  pickerFile.append("file_download.txt");
 
-  const unique = directory.clone();
-  unique.append("file_download(1).txt");
+  const defaultFile = defaultDir.clone();
+  defaultFile.append("file_download.txt");
 
   const {MockFilePicker} = SpecialPowers;
   MockFilePicker.init(window);
 
   MockFilePicker.showCallback = fp => {
-    let file = directory.clone();
+    // On picker 'show' event, choose the filename that was set as the default
+    // and append it to the picker's download directory
+    let file = pickerDir.clone();
     file.append(fp.defaultString);
     MockFilePicker.setFiles([file]);
   };
 
   function background() {
     const url = URL.createObjectURL(new Blob(["file content"]));
-    browser.test.onMessage.addListener(async filename => {
+    browser.test.onMessage.addListener(async (filename, saveAs) => {
       try {
-        let id = await browser.downloads.download({
+        let options = {
           url,
           filename,
-          saveAs: true,
-          conflictAction: "uniquify",
-        });
+        };
+        // Only define the saveAs option if the argument was actually set
+        if (saveAs !== undefined) {
+          options.saveAs = saveAs;
+        }
+        let id = await browser.downloads.download(options);
         browser.downloads.onChanged.addListener(delta => {
           if (delta.id == id && delta.state.current === "complete") {
             browser.test.sendMessage("done", {ok: true, id});
           }
         });
       } catch ({message}) {
         browser.test.sendMessage("done", {ok: false, message});
       }
@@ -73,58 +91,78 @@ add_task(async function test_downloads_s
   }
 
   const manifest = {background, manifest: {permissions: ["downloads"]}};
   const extension = ExtensionTestUtils.loadExtension(manifest);
 
   await extension.startup();
   await extension.awaitMessage("ready");
 
-  // Test basic saveAs functionality.
-  MockFilePicker.returnValue = MockFilePicker.returnOK;
+  async function testExpectFilePicker(saveAs) {
+    ok(!pickerFile.exists(), "the file should have been cleaned up properly previously");
+
+    MockFilePicker.returnValue = MockFilePicker.returnOK;
 
-  extension.sendMessage("file_download.txt");
-  let result = await extension.awaitMessage("done");
-  ok(result.ok, "downloads.download() works with saveAs");
+    extension.sendMessage("file_download.txt", saveAs);
+    let result = await extension.awaitMessage("done");
+    ok(result.ok, `downloads.download() works with saveAs=${saveAs}`);
 
-  ok(file.exists(), "the file exists.");
-  is(file.fileSize, 12, "downloaded file is the correct size");
+    ok(pickerFile.exists(), "the file exists.");
+    is(pickerFile.fileSize, 12, "downloaded file is the correct size");
+    pickerFile.remove(false);
 
-  // Test exisisting file with uniquify.
-  MockFilePicker.returnValue = MockFilePicker.returnOK;
+    // Test the user canceling the save dialog.
+    MockFilePicker.returnValue = MockFilePicker.returnCancel;
+
+    extension.sendMessage("file_download.txt", saveAs);
+    result = await extension.awaitMessage("done");
 
-  extension.sendMessage("file_download.txt");
-  result = await extension.awaitMessage("done");
-  ok(result.ok, "downloads.download() works with saveAs and uniquify");
+    ok(!result.ok, "download rejected if the user cancels the dialog");
+    is(result.message, "Download canceled by the user", "with the correct message");
+    ok(!pickerFile.exists(), "file was not downloaded");
+  }
 
-  ok(unique.exists(), "the file exists.");
-  is(unique.fileSize, 12, "downloaded file is the correct size");
-  unique.remove(false);
+  async function testNoFilePicker(saveAs) {
+    ok(!defaultFile.exists(), "the file should have been cleaned up properly previously");
+
+    extension.sendMessage("file_download.txt", saveAs);
+    let result = await extension.awaitMessage("done");
+    ok(result.ok, `downloads.download() works with saveAs=${saveAs}`);
 
-  // Test canceled saveAs for an existing file and uniquify.
-  MockFilePicker.returnValue = MockFilePicker.returnCancel;
+    ok(defaultFile.exists(), "the file exists.");
+    is(defaultFile.fileSize, 12, "downloaded file is the correct size");
+    defaultFile.remove(false);
+  }
 
-  extension.sendMessage("file_download.txt");
-  result = await extension.awaitMessage("done");
+  info("Testing that saveAs=true uses the file picker as expected");
+  let saveAs = true;
+  await testExpectFilePicker(saveAs);
 
-  ok(!result.ok, "download rejected if the user cancels the dialog");
-  is(result.message, "Download canceled by the user", "with the correct message");
+  info("Testing that saveAs=false does not use the file picker");
+  saveAs = false;
+  await testNoFilePicker(saveAs);
 
-  ok(!unique.exists(), "unique file not left after SaveAs canceled.");
-  file.remove(false);
-
-  // Test the user canceling the save dialog.
-  MockFilePicker.returnValue = MockFilePicker.returnCancel;
+  // When saveAs is not set, the behavior should be determined by the Firefox
+  // pref that normally determines whether the "Save As" prompt should be
+  // displayed.
+  info(`Testing that the file picker is used when saveAs is not specified ` +
+       `but ${PROMPTLESS_DOWNLOAD_PREF} is disabled`);
+  saveAs = undefined;
+  await SpecialPowers.pushPrefEnv({"set": [
+    [PROMPTLESS_DOWNLOAD_PREF, false],
+  ]});
+  await testExpectFilePicker(saveAs);
 
-  extension.sendMessage("file_download.txt");
-  result = await extension.awaitMessage("done");
-
-  ok(!result.ok, "download rejected if the user cancels the dialog");
-  is(result.message, "Download canceled by the user", "with the correct message");
-  ok(!file.exists(), "file was not downloaded");
+  info(`Testing that the file picker is NOT used when saveAs is not ` +
+       `specified but ${PROMPTLESS_DOWNLOAD_PREF} is enabled`);
+  await SpecialPowers.popPrefEnv();
+  await SpecialPowers.pushPrefEnv({"set": [
+    [PROMPTLESS_DOWNLOAD_PREF, true],
+  ]});
+  await testNoFilePicker(saveAs);
 
   await extension.unload();
   MockFilePicker.cleanup();
 });
 
 </script>
 
 </body>
copy from toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
copy to toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_uniquify.html
--- a/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html
+++ b/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_uniquify.html
@@ -1,12 +1,12 @@
 <!doctype html>
 <html>
 <head>
-  <title>Test downloads.download() saveAs option</title>
+  <title>Test downloads.download() uniquify option</title>
   <script src="chrome://mochikit/content/tests/SimpleTest/SpawnTask.js"></script>
   <script src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
   <script src="chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js"></script>
   <script src="chrome_head.js"></script>
   <script src="head.js"></script>
   <link rel="stylesheet" href="chrome://mochikit/content/tests/SimpleTest/test.css"/>
 </head>
 <body>
@@ -29,40 +29,41 @@ add_task(async function setup() {
   ]});
 
   SimpleTest.registerCleanupFunction(async () => {
     await SpecialPowers.popPrefEnv();
     directory.remove(true);
   });
 });
 
-add_task(async function test_downloads_saveAs() {
+add_task(async function test_downloads_uniquify() {
   const file = directory.clone();
   file.append("file_download.txt");
 
   const unique = directory.clone();
   unique.append("file_download(1).txt");
 
   const {MockFilePicker} = SpecialPowers;
   MockFilePicker.init(window);
+  MockFilePicker.returnValue = MockFilePicker.returnOK;
 
   MockFilePicker.showCallback = fp => {
     let file = directory.clone();
     file.append(fp.defaultString);
     MockFilePicker.setFiles([file]);
   };
 
   function background() {
     const url = URL.createObjectURL(new Blob(["file content"]));
-    browser.test.onMessage.addListener(async filename => {
+    browser.test.onMessage.addListener(async (filename, saveAs) => {
       try {
         let id = await browser.downloads.download({
           url,
           filename,
-          saveAs: true,
+          saveAs,
           conflictAction: "uniquify",
         });
         browser.downloads.onChanged.addListener(delta => {
           if (delta.id == id && delta.state.current === "complete") {
             browser.test.sendMessage("done", {ok: true, id});
           }
         });
       } catch ({message}) {
@@ -73,58 +74,44 @@ add_task(async function test_downloads_s
   }
 
   const manifest = {background, manifest: {permissions: ["downloads"]}};
   const extension = ExtensionTestUtils.loadExtension(manifest);
 
   await extension.startup();
   await extension.awaitMessage("ready");
 
-  // Test basic saveAs functionality.
-  MockFilePicker.returnValue = MockFilePicker.returnOK;
+  async function testUniquify(saveAs) {
+    info(`Testing conflictAction:"uniquify" with saveAs=${saveAs}`);
 
-  extension.sendMessage("file_download.txt");
-  let result = await extension.awaitMessage("done");
-  ok(result.ok, "downloads.download() works with saveAs");
-
-  ok(file.exists(), "the file exists.");
-  is(file.fileSize, 12, "downloaded file is the correct size");
+    ok(!file.exists(), "downloaded file should have been cleaned up before test ran");
+    ok(!unique.exists(), "uniquified file should have been cleaned up before test ran");
 
-  // Test exisisting file with uniquify.
-  MockFilePicker.returnValue = MockFilePicker.returnOK;
-
-  extension.sendMessage("file_download.txt");
-  result = await extension.awaitMessage("done");
-  ok(result.ok, "downloads.download() works with saveAs and uniquify");
-
-  ok(unique.exists(), "the file exists.");
-  is(unique.fileSize, 12, "downloaded file is the correct size");
-  unique.remove(false);
+    // Test download without uniquify and create a conflicting file so we can
+    // test with uniquify.
+    extension.sendMessage("file_download.txt", saveAs);
+    let result = await extension.awaitMessage("done");
+    ok(result.ok, "downloads.download() works with saveAs");
 
-  // Test canceled saveAs for an existing file and uniquify.
-  MockFilePicker.returnValue = MockFilePicker.returnCancel;
+    ok(file.exists(), "the file exists.");
+    is(file.fileSize, 12, "downloaded file is the correct size");
 
-  extension.sendMessage("file_download.txt");
-  result = await extension.awaitMessage("done");
-
-  ok(!result.ok, "download rejected if the user cancels the dialog");
-  is(result.message, "Download canceled by the user", "with the correct message");
+    // Now that a conflicting file exists, test the uniquify behavior
+    extension.sendMessage("file_download.txt", saveAs);
+    result = await extension.awaitMessage("done");
+    ok(result.ok, "downloads.download() works with saveAs and uniquify");
 
-  ok(!unique.exists(), "unique file not left after SaveAs canceled.");
-  file.remove(false);
-
-  // Test the user canceling the save dialog.
-  MockFilePicker.returnValue = MockFilePicker.returnCancel;
+    ok(unique.exists(), "the file exists.");
+    is(unique.fileSize, 12, "downloaded file is the correct size");
 
-  extension.sendMessage("file_download.txt");
-  result = await extension.awaitMessage("done");
-
-  ok(!result.ok, "download rejected if the user cancels the dialog");
-  is(result.message, "Download canceled by the user", "with the correct message");
-  ok(!file.exists(), "file was not downloaded");
+    file.remove(false);
+    unique.remove(false);
+  }
+  await testUniquify(true);
+  await testUniquify(false);
 
   await extension.unload();
   MockFilePicker.cleanup();
 });
 
 </script>
 
 </body>