Bug 1719901 - Show "Save as dialog" instead of "Unknown Content" dialog. r=mtigley,Gijs,preferences-reviewers

Also solves 1719902 - the default system action is changed from "opening with file" to saving.

Differential Revision: https://phabricator.services.mozilla.com/D120839

browser/components/preferences/tests/browser_open_download_preferences.js

@@ -1,3 +1,8 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
 const { HandlerServiceTestUtils } = ChromeUtils.import(
   "resource://testing-common/HandlerServiceTestUtils.jsm"
 );
@@ -277,3 +282,62 @@ add_task(async function useSystemDefaultPreferenceWorks() {
 
   gBrowser.removeCurrentTab();
 });
+
+add_task(async function useSystemDefaultAndAskForDestinationWorks() {
+  await SpecialPowers.pushPrefEnv({
+    set: [
+      ["browser.download.improvements_to_download_panel", true],
+      ["browser.download.useDownloadDir", false],
+    ],
+  });
+
+  let pdfCategory = await selectPdfCategoryItem();
+  let list = pdfCategory.querySelector(".actionsMenu");
+
+  let useSystemDefaultItem = list.querySelector(
+    `menuitem[action='${Ci.nsIHandlerInfo.useSystemDefault}']`
+  );
+
+  // Whether there's a "use default" item depends on the OS, there might not be a system default viewer.
+  if (!useSystemDefaultItem) {
+    info(
+      "No 'Use default' item, so no testing for setting 'use system default' preference"
+    );
+    gBrowser.removeCurrentTab();
+    return;
+  }
+
+  await selectItemInPopup(useSystemDefaultItem, list);
+  Assert.equal(
+    list.selectedItem,
+    useSystemDefaultItem,
+    "Should have selected 'use system default' for pdf"
+  );
+
+  let MockFilePicker = SpecialPowers.MockFilePicker;
+  MockFilePicker.init(window);
+  let filePickerShown = new Promise(resolve => {
+    MockFilePicker.showCallback = function(fp) {
+      ok(true, "filepicker should have been shown");
+      setTimeout(resolve, 0);
+      return Ci.nsIFilePicker.returnCancel;
+    };
+  });
+
+  let publicList = await Downloads.getList(Downloads.PUBLIC);
+  registerCleanupFunction(async () => {
+    await publicList.removeFinished();
+    MockFilePicker.cleanup();
+  });
+
+  let loadingTab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    TEST_PATH + "empty_pdf_file.pdf"
+  );
+
+  await filePickerShown;
+
+  BrowserTestUtils.removeTab(loadingTab);
+
+  gBrowser.removeCurrentTab();
+});

toolkit/mozapps/downloads/HelperAppDlg.jsm

@@ -1081,7 +1081,7 @@ nsUnknownContentTypeDialog.prototype = {
         );
         this._saveToDiskTimer.initWithCallback(this, 0, nsITimer.TYPE_ONE_SHOT);
       } else {
-        this.mLauncher.launchWithApplication(this.handleInternally);
+        this.mLauncher.launchWithApplication(this.handleInternally, null);
       }
 
       // Update user pref for this mime type (if necessary). We do not

uriloader/exthandler/mac/nsOSHelperAppService.mm

@@ -7,6 +7,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include "mozilla/net/NeckoCommon.h"
+#include "mozilla/StaticPrefs_browser.h"
 #include "nsComponentManagerUtils.h"
 #include "nsOSHelperAppService.h"
 #include "nsObjCExceptions.h"
@@ -511,7 +512,11 @@ nsresult nsOSHelperAppService::GetMIMEInfoFromOS(const nsACString& aMIMEType,
     }
 
     mimeInfoMac->SetDefaultApplication(app);
-    mimeInfoMac->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
+
+    mozilla::StaticPrefs::browser_download_improvements_to_download_panel()
+        ? mimeInfoMac->SetPreferredAction(nsIMIMEInfo::saveToDisk)
+        : mimeInfoMac->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
+
   } else {
     mimeInfoMac->SetPreferredAction(nsIMIMEInfo::saveToDisk);
   }

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -1814,10 +1814,8 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) {
 
   bool alwaysAsk = true;
 
-  // Skip showing UnknownContentType dialog, unless it is explicitly
-  // set in preferences.
+  // Skip showing UnknownContentType dialog by default if the pref is set.
   bool skipShowingDialog =
-      Preferences::GetBool("browser.download.useDownloadDir") &&
       StaticPrefs::browser_download_improvements_to_download_panel();
 
   if (skipShowingDialog) {
@@ -1955,10 +1953,15 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) {
     }
 
 #endif
-    if (action == nsIMIMEInfo::useHelperApp ||
-        action == nsIMIMEInfo::useSystemDefault ||
-        shouldAutomaticallyHandleInternally) {
-      rv = LaunchWithApplication(shouldAutomaticallyHandleInternally);
+    bool alwaysAskWhereToSave =
+        !Preferences::GetBool("browser.download.useDownloadDir") &&
+        StaticPrefs::browser_download_improvements_to_download_panel();
+
+    if ((action == nsIMIMEInfo::useHelperApp ||
+         action == nsIMIMEInfo::useSystemDefault ||
+         shouldAutomaticallyHandleInternally) &&
+        !alwaysAskWhereToSave) {
+      rv = LaunchWithApplication(shouldAutomaticallyHandleInternally, nullptr);
     } else {
       rv = PromptForSaveDestination();
     }
@@ -2489,7 +2492,9 @@ void nsExternalAppHandler::RequestSaveDestination(
 NS_IMETHODIMP nsExternalAppHandler::PromptForSaveDestination() {
   if (mCanceled) return NS_OK;
 
-  mMimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);
+  if (!StaticPrefs::browser_download_improvements_to_download_panel()) {
+    mMimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);
+  }
 
   if (mSuggestedFileName.IsEmpty()) {
     RequestSaveDestination(mTempLeafName, mTempFileExtension);
@@ -2513,14 +2518,28 @@ nsresult nsExternalAppHandler::ContinueSave(nsIFile* aNewFileLocation) {
 
   MOZ_ASSERT(aNewFileLocation, "Must be called with a non-null file");
 
+  int32_t action = nsIMIMEInfo::saveToDisk;
+  mMimeInfo->GetPreferredAction(&action);
+  bool shouldAutomaticallyHandleInternally =
+      action == nsIMIMEInfo::handleInternally &&
+      StaticPrefs::browser_download_improvements_to_download_panel();
+
+  if (StaticPrefs::browser_download_improvements_to_download_panel() &&
+      (action == nsIMIMEInfo::useHelperApp ||
+       action == nsIMIMEInfo::useSystemDefault ||
+       shouldAutomaticallyHandleInternally)) {
+    return LaunchWithApplication(shouldAutomaticallyHandleInternally,
+                                 aNewFileLocation);
+  }
+
   nsresult rv = NS_OK;
   nsCOMPtr<nsIFile> fileToUse = aNewFileLocation;
   mFinalFileDestination = fileToUse;
 
   // Move what we have in the final directory, but append .part
   // to it, to indicate that it's unfinished.  Do not call SetTarget on the
-  // saver if we are done (Finish has been called) but OnSaverComplete has not
-  // been called.
+  // saver if we are done (Finish has been called) but OnSaverComplete has
+  // not been called.
   if (mFinalFileDestination && mSaver && !mStopRequestIssued) {
     nsCOMPtr<nsIFile> movedFile;
     mFinalFileDestination->Clone(getter_AddRefs(movedFile));
@@ -2559,7 +2578,7 @@ nsresult nsExternalAppHandler::ContinueSave(nsIFile* aNewFileLocation) {
 // LaunchWithApplication should only be called by the helper app dialog which
 // allows the user to say launch with application or save to disk.
 NS_IMETHODIMP nsExternalAppHandler::LaunchWithApplication(
-    bool aHandleInternally) {
+    bool aHandleInternally, nsIFile* aNewFileLocation) {
   if (mCanceled) return NS_OK;
 
   mHandleInternally = aHandleInternally;
@@ -2591,19 +2610,24 @@ NS_IMETHODIMP nsExternalAppHandler::LaunchWithApplication(
   // directory as originally downloaded so the download can be renamed in place
   // later.
   nsCOMPtr<nsIFile> fileToUse;
-  (void)GetDownloadDirectory(getter_AddRefs(fileToUse));
+  if (aNewFileLocation &&
+      StaticPrefs::browser_download_improvements_to_download_panel()) {
+    fileToUse = aNewFileLocation;
+  } else {
+    (void)GetDownloadDirectory(getter_AddRefs(fileToUse));
 
-  if (mSuggestedFileName.IsEmpty()) {
-    // Keep using the leafname of the temp file, since we're just starting a
-    // helper
-    mSuggestedFileName = mTempLeafName;
-  }
+    if (mSuggestedFileName.IsEmpty()) {
+      // Keep using the leafname of the temp file, since we're just starting a
+      // helper
+      mSuggestedFileName = mTempLeafName;
+    }
 
 #ifdef XP_WIN
-  fileToUse->Append(mSuggestedFileName + mTempFileExtension);
+    fileToUse->Append(mSuggestedFileName + mTempFileExtension);
 #else
-  fileToUse->Append(mSuggestedFileName);
+    fileToUse->Append(mSuggestedFileName);
 #endif
+  }
 
   nsresult rv = fileToUse->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
   if (NS_SUCCEEDED(rv)) {

uriloader/exthandler/nsIExternalHelperAppService.idl

@@ -134,8 +134,10 @@ interface nsIHelperAppLauncher : nsICancelable
    * Tell the launcher that we will want to open the file.
    * NOTE: This will release the reference to the nsIHelperAppLauncherDialog.
    * @param aHandleInternally TRUE if we should handle opening this internally.
+   * @param aNewFileLocation a preferred location choosen through the File Picker.
+   * Null if going through the fast save without File Picker.
    */
-  void launchWithApplication(in boolean aHandleInternally);
+  void launchWithApplication(in boolean aHandleInternally, in nsIFile aFile);
 
   /**
    * Callback invoked by nsIHelperAppLauncherDialog::promptForSaveToFileAsync

uriloader/exthandler/tests/mochitest/browser.ini

@@ -45,6 +45,7 @@ support-files =
 skip-if = (os == 'mac') || (os == 'android')
 support-files =
   file_pdf_application_pdf.pdf
+[browser_shows_where_to_save_dialog.js]
 [browser_open_internal_choice_persistence.js]
 support-files =
   file_pdf_application_pdf.pdf

uriloader/exthandler/tests/mochitest/browser_launched_app_save_directory.js

@@ -15,49 +15,28 @@ const TEST_PATH = getRootDirectory(gTestPath).replace(
 // is actually saved in default Downloads directory.
 add_task(async function aDownloadLaunchedWithAppIsSavedInDownloadsFolder() {
   await SpecialPowers.pushPrefEnv({
-    set: [
-      ["browser.download.improvements_to_download_panel", true],
-      ["browser.download.useDownloadDir", false],
-    ],
+    set: [["browser.download.improvements_to_download_panel", true]],
   });
 
   let publicList = await Downloads.getList(Downloads.PUBLIC);
   registerCleanupFunction(async () => {
     await publicList.removeFinished();
   });
-  let dialogWindowPromise = BrowserTestUtils.domWindowOpenedAndLoaded();
+  let downloadFinishedPromise = promiseDownloadFinished(publicList);
+  let initialTabsCount = gBrowser.tabs.length;
+
   let loadingTab = await BrowserTestUtils.openNewForegroundTab(
     gBrowser,
     TEST_PATH + "file_pdf_application_pdf.pdf"
   );
 
-  let dialogWindow = await dialogWindowPromise;
-  let doc = dialogWindow.document;
-  let dialog = doc.querySelector("#unknownContentType");
-  let button = dialog.getButton("accept");
-
-  await TestUtils.waitForCondition(
-    () => !button.disabled,
-    "Wait for Accept button to get enabled"
+  let download = await downloadFinishedPromise;
+  await BrowserTestUtils.waitForCondition(
+    () => gBrowser.tabs.length == initialTabsCount + 2
   );
 
-  let downloadFinishedPromise = promiseDownloadFinished(publicList);
-  let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser);
-
-  button.disabled = false;
-  dialog.acceptDialog();
-  let newTab = await newTabPromise;
-
-  await ContentTask.spawn(newTab.linkedBrowser, null, async () => {
-    await ContentTaskUtils.waitForCondition(
-      () => content.document.readyState == "complete"
-    );
-  });
-
-  let download = await downloadFinishedPromise;
-
+  gBrowser.removeCurrentTab();
   BrowserTestUtils.removeTab(loadingTab);
-  BrowserTestUtils.removeTab(newTab);
 
   let downloadDir = await DownloadIntegration.getSystemDownloadsDirectory();
   ok(

uriloader/exthandler/tests/mochitest/browser_shows_where_to_save_dialog.js

@@ -0,0 +1,45 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const TEST_PATH = getRootDirectory(gTestPath).replace(
+  "chrome://mochitests/content",
+  "https://example.com"
+);
+
+// This test ensures that a "Save as..." filepicker dialog is shown for a file
+// if useDownloadDir ("Always ask where to save files") is set to false
+add_task(async function aDownloadLaunchedWithAppPromptsForFolder() {
+  await SpecialPowers.pushPrefEnv({
+    set: [
+      ["browser.download.improvements_to_download_panel", true],
+      ["browser.download.useDownloadDir", false],
+    ],
+  });
+
+  let MockFilePicker = SpecialPowers.MockFilePicker;
+  MockFilePicker.init(window);
+
+  let publicList = await Downloads.getList(Downloads.PUBLIC);
+  registerCleanupFunction(async () => {
+    await publicList.removeFinished();
+    MockFilePicker.cleanup();
+  });
+  let filePickerShown = new Promise(resolve => {
+    MockFilePicker.showCallback = function(fp) {
+      ok(true, "filepicker should have been shown");
+      setTimeout(resolve, 0);
+      return Ci.nsIFilePicker.returnCancel;
+    };
+  });
+
+  let loadingTab = await BrowserTestUtils.openNewForegroundTab(
+    gBrowser,
+    TEST_PATH + "file_txt_attachment_test.txt"
+  );
+
+  await filePickerShown;
+
+  BrowserTestUtils.removeTab(loadingTab);
+});

uriloader/exthandler/unix/nsOSHelperAppService.cpp

@@ -34,6 +34,7 @@
 #include "ContentHandlerService.h"
 #include "prenv.h"  // for PR_GetEnv()
 #include "mozilla/Preferences.h"
+#include "mozilla/StaticPrefs_browser.h"
 #include "nsMimeTypes.h"
 
 using namespace mozilla;
@@ -1209,7 +1210,9 @@ already_AddRefed<nsMIMEInfoBase> nsOSHelperAppService::GetFromExtension(
 
     if (NS_SUCCEEDED(rv)) {
       mimeInfo->SetDefaultApplication(handlerFile);
-      mimeInfo->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
+      mozilla::StaticPrefs::browser_download_improvements_to_download_panel()
+          ? mimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk)
+          : mimeInfo->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
       mimeInfo->SetDefaultDescription(handler);
     }
   }
@@ -1325,7 +1328,9 @@ already_AddRefed<nsMIMEInfoBase> nsOSHelperAppService::GetFromType(
 
   if (NS_SUCCEEDED(rv)) {
     mimeInfo->SetDefaultApplication(handlerFile);
-    mimeInfo->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
+    StaticPrefs::browser_download_improvements_to_download_panel()
+        ? mimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk)
+        : mimeInfo->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
     mimeInfo->SetDefaultDescription(handler);
   } else {
     mimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);

uriloader/exthandler/win/nsOSHelperAppService.cpp

@@ -17,6 +17,7 @@
 #include "nsLocalFile.h"
 #include "nsIWindowsRegKey.h"
 #include "nsXULAppAPI.h"
+#include "mozilla/StaticPrefs_browser.h"
 #include "mozilla/UniquePtrExtensions.h"
 #include "mozilla/WindowsVersion.h"
 
@@ -377,7 +378,9 @@ already_AddRefed<nsMIMEInfoWin> nsOSHelperAppService::GetByExtension(
       NS_ConvertUTF16toUTF8(Substring(fileExtToUse, 1));
   ToLowerCase(lowerFileExt);
   mimeInfo->AppendExtension(lowerFileExt);
-  mimeInfo->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
+  mozilla::StaticPrefs::browser_download_improvements_to_download_panel()
+      ? mimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk)
+      : mimeInfo->SetPreferredAction(nsIMIMEInfo::useSystemDefault);
 
   nsAutoString appInfo;
   bool found;