Bug 1639067 part 2 - Extend "Open with" and overwrite description to all types viewable internally. r=Gijs

Also improve detection of types needing overwrite by using the final extension settled on for the
type.

I didn't fold PDF fully into this part, as it has different conditions for showing the radio button vs.
viewing internally from the downloads list.

Also fix intermittent bug 1641774 (wouldn't run in verify) by saving handler info at the beginning
of browser_download_open_with_internal_handler, instead of at the start of a subtest.

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

toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties

@@ -16,4 +16,8 @@ unknownCancel.label=Cancel
 fileType=%S file
 # LOCALIZATION NOTE (orderedFileSizeWithType): first %S is type, second %S is size, and third %S is unit
 orderedFileSizeWithType=%1$S (%2$S %3$S)
-pdfHandlerDescription=Portable Document Format
+avifExtHandlerDescription=AV1 Image File (AVIF)
+pdfExtHandlerDescription=Portable Document Format (PDF)
+svgExtHandlerDescription=Scalable Vector Graphics (SVG)
+webpExtHandlerDescription=WebP Image
+xmlExtHandlerDescription=Extensible Markup Language (XML)

toolkit/mozapps/downloads/HelperAppDlg.jsm

@@ -22,6 +22,16 @@ XPCOMUtils.defineLazyServiceGetter(
   Ci.nsIApplicationReputationService
 );
 
+const { Integration } = ChromeUtils.import(
+  "resource://gre/modules/Integration.jsm"
+);
+/* global DownloadIntegration */
+Integration.downloads.defineModuleGetter(
+  this,
+  "DownloadIntegration",
+  "resource://gre/modules/DownloadIntegration.jsm"
+);
+
 // /////////////////////////////////////////////////////////////////////////////
 // // Helper Functions
 
@@ -1272,8 +1282,6 @@ nsUnknownContentTypeDialog.prototype = {
   },
 
   shouldShowInternalHandlerOption() {
-    // This is currently available only for PDF files and when
-    // pdf.js is enabled.
     let browsingContext = this.mDialog.BrowsingContext.get(
       this.mLauncher.browsingContextId
     );
@@ -1283,17 +1291,26 @@ nsUnknownContentTypeDialog.prototype = {
       // known extensions for this mimetype.
       primaryExtension = this.mLauncher.MIMEInfo.primaryExtension;
     } catch (e) {}
+
+    // Only available for PDF files when pdf.js is enabled.
+    // Skip if the current window uses the resource scheme, to avoid
+    // showing the option when using the Download button in pdf.js.
+    if (primaryExtension == "pdf") {
       return (
         !browsingContext?.currentWindowGlobal?.documentPrincipal?.URI?.schemeIs(
           "resource"
         ) &&
-      primaryExtension == "pdf" &&
         !Services.prefs.getBoolPref("pdfjs.disabled", true) &&
         Services.prefs.getBoolPref(
           "browser.helperApps.showOpenOptionForPdfJS",
           false
         )
       );
+    }
+
+    return DownloadIntegration.shouldViewDownloadInternally(
+      this.mLauncher.MIMEInfo.MIMEType
+    );
   },
 
   getOpenWithActionForTelemetry() {

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -539,6 +539,16 @@ static const nsDefaultMimeTypeEntry nonDecodableExtensions[] = {
     {APPLICATION_COMPRESS, "z"},
     {APPLICATION_GZIP, "svgz"}};
 
+/**
+ * Primary extensions of types whose descriptions should be overwritten.
+ * This extension is concatenated with "ExtHandlerDescription" to look up the
+ * description in unknownContentType.properties.
+ * NOTE: These MUST be lower-case and ASCII.
+ */
+static const char* descriptionOverwriteExtensions[] = {
+    "avif", "pdf", "svg", "webp", "xml",
+};
+
 static StaticRefPtr<nsExternalHelperAppService> sExtHelperAppSvcSingleton;
 
 /**
@@ -2566,28 +2576,6 @@ NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(
     found = found || NS_SUCCEEDED(rv);
   }
 
-  // Next, overwrite with generic description if the extension is PDF
-  // since the file format is supported by Firefox and we don't want
-  // other brands positioning themselves as the sole viewer for a system.
-  if (aFileExt.LowerCaseEqualsASCII("pdf") ||
-      aFileExt.LowerCaseEqualsASCII(".pdf")) {
-    nsCOMPtr<nsIStringBundleService> bundleService =
-        do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
-    NS_ENSURE_SUCCESS(rv, rv);
-    nsCOMPtr<nsIStringBundle> unknownContentTypeBundle;
-    rv = bundleService->CreateBundle(
-        "chrome://mozapps/locale/downloads/unknownContentType.properties",
-        getter_AddRefs(unknownContentTypeBundle));
-    if (NS_SUCCEEDED(rv)) {
-      nsAutoString pdfHandlerDescription;
-      rv = unknownContentTypeBundle->GetStringFromName("pdfHandlerDescription",
-                                                       pdfHandlerDescription);
-      if (NS_SUCCEEDED(rv)) {
-        (*_retval)->SetDescription(pdfHandlerDescription);
-      }
-    }
-  }
-
   // Now, let's see if we can find something in our datastore.
   // This will not overwrite the OS information that interests us
   // (i.e. default application, default app. description)
@@ -2657,6 +2645,37 @@ NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(
     }
   }
 
+  // Overwrite with a generic description if the primary extension for the
+  // type is in our list; these are file formats supported by Firefox and
+  // we don't want other brands positioning themselves as the sole viewer
+  // for a system.
+  nsAutoCString primaryExtension;
+  rv = (*_retval)->GetPrimaryExtension(primaryExtension);
+  if (NS_SUCCEEDED(rv)) {
+    for (const char* ext : descriptionOverwriteExtensions) {
+      if (primaryExtension.Equals(ext)) {
+        nsCOMPtr<nsIStringBundleService> bundleService =
+            do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
+        NS_ENSURE_SUCCESS(rv, rv);
+        nsCOMPtr<nsIStringBundle> unknownContentTypeBundle;
+        rv = bundleService->CreateBundle(
+            "chrome://mozapps/locale/downloads/unknownContentType.properties",
+            getter_AddRefs(unknownContentTypeBundle));
+        if (NS_SUCCEEDED(rv)) {
+          nsAutoCString stringName(ext);
+          stringName.AppendLiteral("ExtHandlerDescription");
+          nsAutoString handlerDescription;
+          rv = unknownContentTypeBundle->GetStringFromName(stringName.get(),
+                                                           handlerDescription);
+          if (NS_SUCCEEDED(rv)) {
+            (*_retval)->SetDescription(handlerDescription);
+          }
+        }
+        break;
+      }
+    }
+  }
+
   if (LOG_ENABLED()) {
     nsAutoCString type;
     (*_retval)->GetMIMEType(type);

uriloader/exthandler/tests/mochitest/browser.ini

@@ -21,6 +21,8 @@ support-files =
   file_pdf_binary_octet_stream.pdf^headers^
   file_txt_attachment_test.txt
   file_txt_attachment_test.txt^headers^
+  file_xml_attachment_test.xml
+  file_xml_attachment_test.xml^headers^
 [browser_download_urlescape.js]
 support-files =
   file_with@@funny_name.png

uriloader/exthandler/tests/mochitest/browser_download_open_with_internal_handler.js

@@ -48,6 +48,26 @@ add_task(async function setup() {
       ["browser.helperApps.showOpenOptionForPdfJS", true],
     ],
   });
+
+  // Restore handlers after the whole test has run
+  const mimeSvc = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService);
+  const handlerSvc = Cc["@mozilla.org/uriloader/handler-service;1"].getService(
+    Ci.nsIHandlerService
+  );
+  const registerRestoreHandler = function(type, ext) {
+    const mimeInfo = mimeSvc.getFromTypeAndExtension(type, ext);
+    const existed = handlerSvc.exists(mimeInfo);
+    registerCleanupFunction(() => {
+      if (existed) {
+        handlerSvc.store(mimeInfo);
+      } else {
+        handlerSvc.remove(mimeInfo);
+      }
+    });
+  };
+  registerRestoreHandler("application/pdf", "pdf");
+  registerRestoreHandler("binary/octet-stream", "pdf");
+  registerRestoreHandler("application/unknown", "pdf");
 });
 
 /**
@@ -249,20 +269,9 @@ add_task(async function test_check_open_with_external_then_internal() {
     Ci.nsIHandlerService
   );
   const mimeInfo = mimeSvc.getFromTypeAndExtension("application/pdf", "pdf");
-  const exists = handlerSvc.exists(mimeInfo);
-  const { preferredAction, alwaysAskBeforeHandling } = mimeInfo;
   mimeInfo.preferredAction = mimeInfo.alwaysAsk;
   mimeInfo.alwaysAskBeforeHandling = true;
   handlerSvc.store(mimeInfo);
-  registerCleanupFunction(() => {
-    // Restore old nsIMIMEInfo
-    if (exists) {
-      Object.assign(mimeInfo, { preferredAction, alwaysAskBeforeHandling });
-      handlerSvc.store(mimeInfo);
-    } else {
-      handlerSvc.remove(mimeInfo);
-    }
-  });
 
   for (let [file, mimeType] of [
     ["file_pdf_application_pdf.pdf", "application/pdf"],
@@ -391,10 +400,43 @@ add_task(async function test_check_open_with_external_then_internal() {
 });
 
 /**
- * Check that the "Open with internal handler" option is not presented
- * for non-PDF types.
+ * Check that the "Open with internal handler" option is presented
+ * for other viewable internally types.
  */
-add_task(async function test_internal_handler_hidden_with_nonpdf_type() {
+add_task(
+  async function test_internal_handler_hidden_with_viewable_internally_type() {
+    let dialogWindowPromise = BrowserTestUtils.domWindowOpenedAndLoaded();
+    let loadingTab = await BrowserTestUtils.openNewForegroundTab(
+      gBrowser,
+      TEST_PATH + "file_xml_attachment_test.xml"
+    );
+    let dialogWindow = await dialogWindowPromise;
+    is(
+      dialogWindow.location.href,
+      "chrome://mozapps/content/downloads/unknownContentType.xhtml",
+      "Should have seen the unknown content dialogWindow."
+    );
+    let doc = dialogWindow.document;
+    let internalHandlerRadio = doc.querySelector("#handleInternally");
+
+    // Prevent racing with initialization of the dialog and make sure that
+    // the final state of the dialog has the correct visibility of the internal-handler option.
+    await waitForAcceptButtonToGetEnabled(doc);
+
+    ok(!internalHandlerRadio.hidden, "The option should be visible for XML");
+    ok(internalHandlerRadio.selected, "The option should be selected");
+
+    let dialog = doc.querySelector("#unknownContentType");
+    dialog.cancelDialog();
+    BrowserTestUtils.removeTab(loadingTab);
+  }
+);
+
+/**
+ * Check that the "Open with internal handler" option is not presented
+ * for non-PDF, non-viewable-internally types.
+ */
+add_task(async function test_internal_handler_hidden_with_other_type() {
   let dialogWindowPromise = BrowserTestUtils.domWindowOpenedAndLoaded();
   let loadingTab = await BrowserTestUtils.openNewForegroundTab(
     gBrowser,

uriloader/exthandler/tests/mochitest/file_xml_attachment_test.xml

@@ -0,0 +1,4 @@
+<?xml version = "1.0" encoding = "utf-8"?>
+
+<something>
+</something>

uriloader/exthandler/tests/mochitest/file_xml_attachment_test.xml^headers^

@@ -0,0 +1,2 @@
+Content-Disposition: attachment; filename=file_xml_attachment_test.xml
+Content-Type: text/xml

uriloader/exthandler/tests/unit/test_getMIMEInfo_pdf.js

@@ -9,6 +9,12 @@ XPCOMUtils.defineLazyServiceGetter(
   "@mozilla.org/mime;1",
   "nsIMIMEService"
 );
+XPCOMUtils.defineLazyServiceGetter(
+  this,
+  "gBundleService",
+  "@mozilla.org/intl/stringbundle;1",
+  "nsIStringBundleService"
+);
 
 // PDF files should always have a generic description instead
 // of relying on what is registered with the Operating System.
@@ -19,9 +25,12 @@ add_task(async function test_check_unknown_mime_type() {
   let extension = mimeService.getPrimaryExtension("application/pdf", "");
   Assert.equal(extension, "pdf", "Expect pdf extension when given mime");
   let mimeInfo = gMIMEService.getFromTypeAndExtension("", "pdf");
+  let stringBundle = gBundleService.createBundle(
+    "chrome://mozapps/locale/downloads/unknownContentType.properties"
+  );
   Assert.equal(
     mimeInfo.description,
-    "Portable Document Format",
+    stringBundle.GetStringFromName("pdfExtHandlerDescription"),
     "PDF has generic description"
   );
 });