Bug 1486984 - Fix find commands for PDF and special pages, and remove obsolete code. r=Gijs
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Tue, 23 Oct 2018 15:29:09 +0100
changeset 491066 7327a893e2a276779fe430920d2bf7c7822f1871
parent 491065 c5f42f6a73c541530a5d813f1ef349b3bcaa30a9
child 491067 006df234545bc1db46c9e4ce3f2ca75d20d63d58
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersGijs
bugs1486984
milestone65.0a1
Bug 1486984 - Fix find commands for PDF and special pages, and remove obsolete code. r=Gijs Support for finding text in a page is now determined by a blacklist of locations, simplifying handling in multi-process mode and restoring the intended behavior. Differential Revision: https://phabricator.services.mozilla.com/D8005
browser/base/content/browser.js
browser/base/content/test/general/browser_viewSourceInTabOnViewSource.js
browser/components/preferences/in-content/preferences.xul
toolkit/actors/FindBarChild.jsm
toolkit/components/viewconfig/content/config.xul
toolkit/modules/BrowserUtils.jsm
toolkit/mozapps/extensions/content/extensions.xul
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -4669,29 +4669,34 @@ var XULBrowserWindow = {
   get stopCommand() {
     delete this.stopCommand;
     return this.stopCommand = document.getElementById("Browser:Stop");
   },
   get reloadCommand() {
     delete this.reloadCommand;
     return this.reloadCommand = document.getElementById("Browser:Reload");
   },
-  get elementsForTextBasedTypes() {
-    delete this.elementsForTextBasedTypes;
-    return this.elementsForTextBasedTypes = [
+  get _elementsForTextBasedTypes() {
+    delete this._elementsForTextBasedTypes;
+    return this._elementsForTextBasedTypes = [
       document.getElementById("pageStyleMenu"),
       document.getElementById("context-viewpartialsource-selection"),
+    ];
+  },
+  get _elementsForFind() {
+    delete this._elementsForFind;
+    return this._elementsForFind = [
       document.getElementById("cmd_find"),
       document.getElementById("cmd_findAgain"),
       document.getElementById("cmd_findPrevious"),
     ];
   },
-  get elementsForViewSource() {
-    delete this.elementsForViewSource;
-    return this.elementsForViewSource = [
+  get _elementsForViewSource() {
+    delete this._elementsForViewSource;
+    return this._elementsForViewSource = [
       document.getElementById("context-viewsource"),
       document.getElementById("View:PageSource"),
     ];
   },
 
   forceInitialBrowserNonRemote(aOpener) {
     gBrowser.updateBrowserRemoteness(gBrowser.initialBrowser, false, { opener: aOpener });
   },
@@ -4835,34 +4840,28 @@ var XULBrowserWindow = {
                 break;
             }
           }
         }
 
         this.status = "";
         this.setDefaultStatus(msg);
 
-        // Disable menu entries for images, enable otherwise
+        // Disable View Source menu entries for images, enable otherwise
         let isText = browser.documentContentType &&
                      BrowserUtils.mimeTypeIsTextBased(browser.documentContentType);
-        for (let element of this.elementsForTextBasedTypes) {
-          if (isText) {
-            element.removeAttribute("disabled");
-          } else {
-            element.setAttribute("disabled", "true");
-          }
-        }
-
-        for (let element of this.elementsForViewSource) {
+        for (let element of this._elementsForViewSource) {
           if (canViewSource && isText) {
             element.removeAttribute("disabled");
           } else {
             element.setAttribute("disabled", "true");
           }
         }
+
+        this._updateElementsForContentType();
       }
 
       this.isBusy = false;
 
       if (this.busyUI) {
         this.busyUI = false;
 
         this.stopCommand.setAttribute("disabled", "true");
@@ -4887,29 +4886,16 @@ var XULBrowserWindow = {
           if (tooltipWindow == aWebProgress.DOMWindow) {
             pageTooltip.hidePopup();
             break;
           }
         }
       }
     }
 
-    let browser = gBrowser.selectedBrowser;
-
-    // Disable menu entries for images, enable otherwise
-    let isText = browser.documentContentType &&
-                 BrowserUtils.mimeTypeIsTextBased(browser.documentContentType);
-    for (let element of this.elementsForTextBasedTypes) {
-      if (isText) {
-        element.removeAttribute("disabled");
-      } else {
-        element.setAttribute("disabled", "true");
-      }
-    }
-
     this.hideOverLinkImmediately = true;
     this.setOverLink("", null);
     this.hideOverLinkImmediately = false;
 
     // We should probably not do this if the value has changed since the user
     // searched
     // Update urlbar only if a new page was loaded on the primary content area
     // Do not update urlbar if there was a subframe navigation
@@ -4934,56 +4920,17 @@ var XULBrowserWindow = {
       gIdentityHandler.onLocationChange();
 
       BrowserPageActions.onLocationChange();
 
       SafeBrowsingNotificationBox.onLocationChange(aLocationURI);
 
       gTabletModePageCounter.inc();
 
-      // Utility functions for disabling find
-      var shouldDisableFind = function(aDocument) {
-        let docElt = aDocument.documentElement;
-        return docElt && docElt.getAttribute("disablefastfind") == "true";
-      };
-
-      var disableFindCommands = function(aDisable) {
-        let findCommands = [document.getElementById("cmd_find"),
-                            document.getElementById("cmd_findAgain"),
-                            document.getElementById("cmd_findPrevious")];
-        for (let elt of findCommands) {
-          if (aDisable)
-            elt.setAttribute("disabled", "true");
-          else
-            elt.removeAttribute("disabled");
-        }
-      };
-
-      var onContentRSChange = function(e) {
-        if (e.target.readyState != "interactive" && e.target.readyState != "complete")
-          return;
-
-        e.target.removeEventListener("readystatechange", onContentRSChange);
-        disableFindCommands(shouldDisableFind(e.target));
-      };
-
-      // Disable find commands in documents that ask for them to be disabled.
-      if (!gMultiProcessBrowser && aLocationURI &&
-          (aLocationURI.schemeIs("about") || aLocationURI.schemeIs("chrome"))) {
-        // Don't need to re-enable/disable find commands for same-document location changes
-        // (e.g. the replaceStates in about:addons)
-        if (!(aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT)) {
-          if (window.content.document.readyState == "interactive" || window.content.document.readyState == "complete")
-            disableFindCommands(shouldDisableFind(window.content.document));
-          else {
-            window.content.document.addEventListener("readystatechange", onContentRSChange);
-          }
-        }
-      } else
-        disableFindCommands(false);
+      this._updateElementsForContentType();
 
       // Try not to instantiate gCustomizeMode as much as possible,
       // so don't use CustomizeMode.jsm to check for URI or customizing.
       if (location == "about:blank" &&
           gBrowser.selectedTab.hasAttribute("customizemode")) {
         gCustomizeMode.enter();
       } else if (CustomizationHandler.isEnteringCustomizeMode ||
                  CustomizationHandler.isCustomizing()) {
@@ -5020,16 +4967,42 @@ var XULBrowserWindow = {
         // Don't make noise when the crash reporter is built but not enabled.
         if (ex.result != Cr.NS_ERROR_NOT_INITIALIZED) {
           throw ex;
         }
       }
     }
   },
 
+  _updateElementsForContentType() {
+    let browser = gBrowser.selectedBrowser;
+
+    let isText = browser.documentContentType &&
+                 BrowserUtils.mimeTypeIsTextBased(browser.documentContentType);
+    for (let element of this._elementsForTextBasedTypes) {
+      if (isText) {
+        element.removeAttribute("disabled");
+      } else {
+        element.setAttribute("disabled", "true");
+      }
+    }
+
+    // Always enable find commands in PDF documents, otherwise do it only for
+    // text documents whose location is not in the blacklist.
+    let enableFind = browser.documentContentType == "application/pdf" ||
+      (isText && BrowserUtils.canFindInPage(gBrowser.currentURI.spec));
+    for (let element of this._elementsForFind) {
+      if (enableFind) {
+        element.removeAttribute("disabled");
+      } else {
+        element.setAttribute("disabled", "true");
+      }
+    }
+  },
+
   asyncUpdateUI() {
     BrowserSearch.updateOpenSearchBadge();
   },
 
   onStatusChange(aWebProgress, aRequest, aStatus, aMessage) {
     this.status = aMessage;
     StatusPanel.update();
   },
--- a/browser/base/content/test/general/browser_viewSourceInTabOnViewSource.js
+++ b/browser/base/content/test/general/browser_viewSourceInTabOnViewSource.js
@@ -8,42 +8,42 @@ function wait_while_tab_is_busy() {
         }
       },
     };
     gBrowser.addProgressListener(progressListener);
   });
 }
 
 // This function waits for the tab to stop being busy instead of waiting for it
-// to load, since the elementsForViewSource change happens at that time.
+// to load, since the _elementsForViewSource change happens at that time.
 var with_new_tab_opened = async function(options, taskFn) {
   let busyPromise = wait_while_tab_is_busy();
   let tab = await BrowserTestUtils.openNewForegroundTab(options.gBrowser, options.url, false);
   await busyPromise;
   await taskFn(tab.linkedBrowser);
   gBrowser.removeTab(tab);
 };
 
 add_task(async function test_regular_page() {
   function test_expect_view_source_enabled(browser) {
-    for (let element of [...XULBrowserWindow.elementsForViewSource]) {
+    for (let element of [...XULBrowserWindow._elementsForViewSource]) {
       ok(!element.hasAttribute("disabled"),
          "View Source should be enabled");
     }
   }
 
   await with_new_tab_opened({
     gBrowser,
     url: "http://example.com",
   }, test_expect_view_source_enabled);
 });
 
 add_task(async function test_view_source_page() {
   function test_expect_view_source_disabled(browser) {
-    for (let element of [...XULBrowserWindow.elementsForViewSource]) {
+    for (let element of [...XULBrowserWindow._elementsForViewSource]) {
       ok(element.hasAttribute("disabled"),
          "View Source should be disabled");
     }
   }
 
   await with_new_tab_opened({
     gBrowser,
     url: "view-source:http://example.com",
--- a/browser/components/preferences/in-content/preferences.xul
+++ b/browser/components/preferences/in-content/preferences.xul
@@ -28,17 +28,16 @@
 %historyDTD;
 %certManagerDTD;
 %deviceManangerDTD;
 %sanitizeDTD;
 ]>
 
 <page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
       xmlns:html="http://www.w3.org/1999/xhtml"
-      disablefastfind="true"
       data-l10n-id="pref-page"
       data-l10n-attrs="title">
 
   <linkset>
     <link rel="localization" href="branding/brand.ftl"/>
     <link rel="localization" href="browser/branding/sync-brand.ftl"/>
     <link rel="localization" href="browser/preferences/preferences.ftl"/>
     <!-- Used by fontbuilder.js -->
--- a/toolkit/actors/FindBarChild.jsm
+++ b/toolkit/actors/FindBarChild.jsm
@@ -56,18 +56,25 @@ class FindBarChild extends ActorChild {
   onKeypress(event) {
     let {FindBarContent} = this;
 
     if (!FindBarContent.inPassThrough &&
         this.eventMatchesFindShortcut(event)) {
       return FindBarContent.start(event);
     }
 
+    // disable FAYT in about:blank to prevent FAYT opening unexpectedly.
+    let location = this.content.location.href;
+    if (location == "about:blank") {
+      return null;
+    }
+
     if (event.ctrlKey || event.altKey || event.metaKey || event.defaultPrevented ||
-        !BrowserUtils.canFastFind(this.content)) {
+        !BrowserUtils.mimeTypeIsTextBased(this.content.document.contentType) ||
+        !BrowserUtils.canFindInPage(location)) {
       return null;
     }
 
     if (FindBarContent.inPassThrough || FindBarContent.inQuickFind) {
       return FindBarContent.onKeypress(event);
     }
 
     if (event.charCode && BrowserUtils.shouldFastFind(event.target)) {
--- a/toolkit/components/viewconfig/content/config.xul
+++ b/toolkit/components/viewconfig/content/config.xul
@@ -11,17 +11,16 @@
 <window id="config"
         data-l10n-id="config-window"
         aria-describedby="warningTitle warningText"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
         windowtype="Preferences:ConfigManager"
         role="application"
         width="750"
         height="500"
-        disablefastfind="true"
         onunload="onConfigUnload();"
         onload="onConfigLoad();">
 
 <linkset>
   <link rel="localization" href="toolkit/about/aboutConfig.ftl"/>
 </linkset>
 
 <script src="chrome://global/content/config.js"/>
--- a/toolkit/modules/BrowserUtils.jsm
+++ b/toolkit/modules/BrowserUtils.jsm
@@ -306,41 +306,26 @@ var BrowserUtils = {
           elt instanceof win.HTMLEmbedElement)
         return false;
     }
 
     return true;
   },
 
   /**
-   * Return true if we can FAYT for this window (could be CPOW):
+   * Returns true if we can show a find bar, including FAYT, for the specified
+   * document location. The location must not be in a blacklist of specific
+   * "about:" pages for which find is disabled.
    *
-   * @param win
-   *        The top level window that is focused
-   *
+   * This can be called from the parent process or from content processes.
    */
-  canFastFind(win) {
-    if (!win)
-      return false;
-
-    if (!this.mimeTypeIsTextBased(win.document.contentType))
-      return false;
-
-    // disable FAYT in about:blank to prevent FAYT opening unexpectedly.
-    let loc = win.location;
-    if (loc.href == "about:blank")
-      return false;
-
-    // disable FAYT in documents that ask for it to be disabled.
-    if ((loc.protocol == "about:" || loc.protocol == "chrome:") &&
-        (win.document.documentElement &&
-         win.document.documentElement.getAttribute("disablefastfind") == "true"))
-      return false;
-
-    return true;
+  canFindInPage(location) {
+    return !location.startsWith("about:addons") &&
+           !location.startsWith("about:config") &&
+           !location.startsWith("about:preferences");
   },
 
   _visibleToolbarsMap: new WeakMap(),
 
   /**
    * Return true if any or a specific toolbar that interacts with the content
    * document is visible.
    *
--- a/toolkit/mozapps/extensions/content/extensions.xul
+++ b/toolkit/mozapps/extensions/content/extensions.xul
@@ -12,18 +12,17 @@
 %brandDTD;
 <!ENTITY % extensionsDTD SYSTEM "chrome://mozapps/locale/extensions/extensions.dtd">
 %extensionsDTD;
 ]>
 
 <page xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
       xmlns:xhtml="http://www.w3.org/1999/xhtml"
       id="addons-page" data-l10n-id="addons-window"
-      role="application" windowtype="Addons:Manager"
-      disablefastfind="true">
+      role="application" windowtype="Addons:Manager">
 
   <xhtml:link rel="shortcut icon"
               href="chrome://mozapps/skin/extensions/extensionGeneric-16.svg"/>
   <linkset>
     <link rel="localization" href="branding/brand.ftl"/>
     <link rel="localization" href="toolkit/about/aboutAddons.ftl"/>
   </linkset>