Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar. r=nalexander
authorMike de Boer <mdeboer@mozilla.com>
Mon, 10 Oct 2016 11:53:25 +0200
changeset 317240 a2c1d4e7e5e01c580877d5e4d7b2680047529bde
parent 317239 515e6d07c0c23ff4e8ad1530f5562f582b952f77
child 317241 8e35ac64eaf6bd5dc63ed15e2e1b8aa22b995975
push id30799
push userphilringnalda@gmail.com
push dateTue, 11 Oct 2016 02:02:17 +0000
treeherdermozilla-central@a38df6f51b67 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander
bugs1303008
milestone52.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 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar. r=nalexander MozReview-Commit-ID: KZIstsbHAyT
mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java
mobile/android/chrome/content/FindHelper.js
toolkit/content/widgets/findbar.xml
toolkit/modules/Finder.jsm
--- a/mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java
+++ b/mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java
@@ -62,17 +62,19 @@ public class FindInPageBar extends Linea
                 }
                 return false;
             }
         });
 
         mStatusText = (TextView) content.findViewById(R.id.find_status);
 
         mInflated = true;
-        GeckoApp.getEventDispatcher().registerGeckoThreadListener(this, "TextSelection:Data");
+        GeckoApp.getEventDispatcher().registerGeckoThreadListener(this,
+            "FindInPage:MatchesCountResult",
+            "TextSelection:Data");
     }
 
     public void show() {
         if (!mInflated)
             inflateContent();
 
         setVisibility(VISIBLE);
         mFindText.requestFocus();
@@ -105,17 +107,44 @@ public class FindInPageBar extends Linea
         Context context = view.getContext();
         return (InputMethodManager) context.getSystemService(Context.INPUT_METHOD_SERVICE);
      }
 
     public void onDestroy() {
         if (!mInflated) {
             return;
         }
-        GeckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "TextSelection:Data");
+        GeckoApp.getEventDispatcher().unregisterGeckoThreadListener(this,
+            "FindInPage:MatchesCountResult",
+            "TextSelection:Data");
+    }
+
+    private void onMatchesCountResult(final int total, final int current, final int limit, final String searchString) {
+        if (total == -1) {
+            updateResult(Integer.toString(limit) + "+");
+        } else if (total > 0) {
+            updateResult(Integer.toString(current) + "/" + Integer.toString(total));
+        } else if (TextUtils.isEmpty(searchString)) {
+            updateResult("");
+        } else {
+            // We display 0/0, when there were no
+            // matches found, or if matching has been turned off by setting
+            // pref accessibility.typeaheadfind.matchesCountLimit to 0.
+            updateResult("0/0");
+        }
+    }
+
+    private void updateResult(final String statusText) {
+        ThreadUtils.postToUiThread(new Runnable() {
+            @Override
+            public void run() {
+                mStatusText.setVisibility(statusText.isEmpty() ? View.GONE : View.VISIBLE);
+                mStatusText.setText(statusText);
+            }
+        });
     }
 
     // TextWatcher implementation
 
     @Override
     public void afterTextChanged(Editable s) {
         sendRequestToFinderHelper("FindInPage:Find", s.toString());
     }
@@ -155,16 +184,24 @@ public class FindInPageBar extends Linea
             hide();
         }
     }
 
     // GeckoEventListener implementation
 
     @Override
     public void handleMessage(String event, JSONObject message) {
+        if (event.equals("FindInPage:MatchesCountResult")) {
+            onMatchesCountResult(message.optInt("total", 0),
+                message.optInt("current", 0),
+                message.optInt("limit", 0),
+                message.optString("searchString"));
+            return;
+        }
+
         if (!event.equals("TextSelection:Data") || !REQUEST_ID.equals(message.optString("requestId"))) {
             return;
         }
 
         final String text = message.optString("text");
 
         // Populate an initial find string, virtual keyboard not required.
         if (!TextUtils.isEmpty(text)) {
@@ -198,45 +235,22 @@ public class FindInPageBar extends Linea
 
     /**
      * Request find operation, and update matchCount results (current count and total).
      */
     private void sendRequestToFinderHelper(final String request, final String searchString) {
         GeckoAppShell.sendRequestToGecko(new GeckoRequest(request, searchString) {
             @Override
             public void onResponse(NativeJSObject nativeJSObject) {
-                final int total = nativeJSObject.optInt("total", 0);
-                if (total == -1) {
-                    final int limit = nativeJSObject.optInt("limit", 0);
-                    updateResult(Integer.toString(limit) + "+");
-                } else if (total > 0) {
-                    final int current = nativeJSObject.optInt("current", 0);
-                    updateResult(Integer.toString(current) + "/" + Integer.toString(total));
-                } else if (TextUtils.isEmpty(searchString)) {
-                    updateResult("");
-                } else {
-                    // We display 0/0, when there were no
-                    // matches found, or if matching has been turned off by setting
-                    // pref accessibility.typeaheadfind.matchesCountLimit to 0.
-                    updateResult("0/0");
-                }
+                // We don't care about the return value, because `onMatchesCountResult`
+                // does the heavy lifting.
             }
 
             @Override
             public void onError(NativeJSObject error) {
                 // Gecko didn't respond due to state change, javascript error, etc.
                 Log.d(LOGTAG, "No response from Gecko on request to match string: [" +
                     searchString + "]");
                 updateResult("");
             }
-
-            private void updateResult(final String statusText) {
-                ThreadUtils.postToUiThread(new Runnable() {
-                    @Override
-                    public void run() {
-                        mStatusText.setVisibility(statusText.isEmpty() ? View.GONE : View.VISIBLE);
-                        mStatusText.setText(statusText);
-                    }
-                });
-            }
         });
     }
 }
--- a/mobile/android/chrome/content/FindHelper.js
+++ b/mobile/android/chrome/content/FindHelper.js
@@ -4,17 +4,18 @@
 "use strict";
 
 var FindHelper = {
   _finder: null,
   _targetTab: null,
   _initialViewport: null,
   _viewportChanged: false,
   _result: null,
-  _limit: 0,
+
+  // Start of nsIObserver implementation.
 
   observe: function(aMessage, aTopic, aData) {
     switch(aTopic) {
       case "FindInPage:Opened": {
         this._findOpened();
         break;
       }
 
@@ -26,44 +27,35 @@ var FindHelper = {
 
       case "FindInPage:Closed":
         this._uninit();
         this._findClosed();
         break;
     }
   },
 
+  /**
+   * When the FindInPageBar opens/ becomes visible, it's time to:
+   * 1. Add listeners for other message types sent from the FindInPageBar
+   * 2. initialize the Finder instance, if necessary.
+   */
   _findOpened: function() {
-    try {
-      this._limit = Services.prefs.getIntPref("accessibility.typeaheadfind.matchesCountLimit");
-    } catch (e) {
-      // Pref not available, assume 0, no match counting.
-      this._limit = 0;
-    }
-
-    Messaging.addListener((data) => {
-      this.doFind(data);
-      return this._getMatchesCountResult(data);
-    }, "FindInPage:Find");
-
-    Messaging.addListener((data) => {
-      this.findAgain(data, false);
-      return this._getMatchesCountResult(data);
-    }, "FindInPage:Next");
-
-    Messaging.addListener((data) => {
-      this.findAgain(data, true);
-      return this._getMatchesCountResult(data);
-    }, "FindInPage:Prev");
+    Messaging.addListener(data => this.doFind(data), "FindInPage:Find");
+    Messaging.addListener(data => this.findAgain(data, false), "FindInPage:Next");
+    Messaging.addListener(data => this.findAgain(data, true), "FindInPage:Prev");
 
     // Initialize the finder component for the current page by performing a fake find.
     this._init();
-    this._finder.requestMatchesCount("", 1);
+    this._finder.requestMatchesCount("");
   },
 
+  /**
+   * Fetch the Finder instance from the active tabs' browser and start tracking
+   * the active viewport.
+   */
   _init: function() {
     // If there's no find in progress, start one.
     if (this._finder) {
       return;
     }
 
     this._targetTab = BrowserApp.selectedTab;
     try {
@@ -73,78 +65,124 @@ var FindHelper = {
         "JS stack: \n" + (e.stack || Components.stack.formattedStack));
     }
 
     this._finder.addResultListener(this);
     this._initialViewport = JSON.stringify(this._targetTab.getViewport());
     this._viewportChanged = false;
   },
 
+  /**
+   * Detach from the Finder instance (so stop listening for messages) and stop
+   * tracking the active viewport.
+   */
   _uninit: function() {
     // If there's no find in progress, there's nothing to clean up.
     if (!this._finder) {
       return;
     }
 
     this._finder.removeSelection();
     this._finder.removeResultListener(this);
     this._finder = null;
     this._targetTab = null;
     this._initialViewport = null;
     this._viewportChanged = false;
   },
 
+  /**
+   * When the FindInPageBar closes, it's time to stop listening for its messages.
+   */
   _findClosed: function() {
     Messaging.removeListener("FindInPage:Find");
     Messaging.removeListener("FindInPage:Next");
     Messaging.removeListener("FindInPage:Prev");
   },
 
   /**
-   * Request, wait for, and return the current matchesCount results for a string.
+   * Start an asynchronous find-in-page operation, using the current Finder
+   * instance and request to count the amount of matches.
+   * If no Finder instance is currently active, we'll lazily initialize it here.
+   *
+   * @param  {String} searchString Word to search for in the current document
+   * @return {Object}              Echo of the current find action
    */
-  _getMatchesCountResult: function(findString) {
-    // Count matches up to any provided limit.
-    if (this._limit <= 0) {
-      return { total: 0, current: 0, limit: 0 };
-    }
-
-    // Sync call to Finder, results available immediately.
-    this._finder.requestMatchesCount(findString, this._limit);
-    return this._result;
-  },
-
-  /**
-   * Pass along the count results to FindInPageBar for display.
-   */
-  onMatchesCountResult: function(result) {
-    this._result = result;
-    this._result.limit = this._limit;
-  },
-
   doFind: function(searchString) {
     if (!this._finder) {
       this._init();
     }
 
     this._finder.fastFind(searchString, false);
+    return { searchString, findBackwards: false };
   },
 
+  /**
+   * Restart the same find-in-page operation as before via `doFind()`. If we
+   * haven't called `doFind()`, we simply kick off a regular find.
+   *
+   * @param  {String}  searchString  Word to search for in the current document
+   * @param  {Boolean} findBackwards Direction to search in
+   * @return {Object}                Echo of the current find action
+   */
   findAgain: function(searchString, findBackwards) {
     // This always happens if the user taps next/previous after re-opening the
     // search bar, and not only forces _init() but also an initial fastFind(STRING)
     // before any findAgain(DIRECTION).
     if (!this._finder) {
-      this.doFind(searchString);
-      return;
+      return this.doFind(searchString);
     }
 
     this._finder.findAgain(findBackwards, false, false);
+    return { searchString, findBackwards };
+  },
+
+  // Start of Finder.jsm listener implementation.
+
+  /**
+   * Pass along the count results to FindInPageBar for display. The result that
+   * is sent to the FindInPageBar is augmented with the current find-in-page count
+   * limit.
+   *
+   * @param {Object} result Result coming from the Finder instance that contains
+   *                        the following properties:
+   *                        - {Number} total   The total amount of matches found
+   *                        - {Number} current The index of current found range
+   *                                           in the document
+   */
+  onMatchesCountResult: function(result) {
+    this._result = result;
+
+    Messaging.sendRequest(Object.assign({
+      type: "FindInPage:MatchesCountResult"
+    }, this._result));
   },
 
+  /**
+   * When a find-in-page action finishes, this method is invoked. This is mainly
+   * used at the moment to detect if the current viewport has changed, which might
+   * be indicated by not finding a string in the current page.
+   *
+   * @param {Object} aData A dictionary, representing the find result, which
+   *                       contains the following properties:
+   *                       - {String}  searchString  Word that was searched for
+   *                                                 in the current document
+   *                       - {Number}  result        One of the following
+   *                                                 Ci.nsITypeAheadFind.* result
+   *                                                 indicators: FIND_FOUND,
+   *                                                 FIND_NOTFOUND, FIND_WRAPPED,
+   *                                                 FIND_PENDING
+   *                       - {Boolean} findBackwards Whether the search direction
+   *                                                 was backwards
+   *                       - {Boolean} findAgain     Whether the previous search
+   *                                                 was repeated
+   *                       - {Boolean} drawOutline   Whether we may (re-)draw the
+   *                                                 outline of a hyperlink
+   *                       - {Boolean} linksOnly     Whether links-only mode was
+   *                                                 active
+   */
   onFindResult: function(aData) {
     if (aData.result == Ci.nsITypeAheadFind.FIND_NOTFOUND) {
       if (this._viewportChanged) {
         if (this._targetTab != BrowserApp.selectedTab) {
           // this should never happen
           Cu.reportError("Warning: selected tab changed during find!");
           // fall through and restore viewport on the initial tab anyway
         }
--- a/toolkit/content/widgets/findbar.xml
+++ b/toolkit/content/widgets/findbar.xml
@@ -369,18 +369,16 @@
         this._foundURL = null;
 
         let prefsvc = this._prefsvc;
 
         this._quickFindTimeoutLength =
           prefsvc.getIntPref("accessibility.typeaheadfind.timeout");
         this._flashFindBar =
           prefsvc.getIntPref("accessibility.typeaheadfind.flashBar");
-        this._matchesCountLimit =
-          prefsvc.getIntPref("accessibility.typeaheadfind.matchesCountLimit");
         this._useModalHighlight = prefsvc.getBoolPref("findbar.modalHighlight");
 
         prefsvc.addObserver("accessibility.typeaheadfind",
                             this._observer, false);
         prefsvc.addObserver("accessibility.typeaheadfind.linksonly",
                             this._observer, false);
         prefsvc.addObserver("accessibility.typeaheadfind.casesensitive",
                             this._observer, false);
@@ -1304,19 +1302,19 @@
         -   of the current result.
         -->
       <method name="onMatchesCountResult">
         <parameter name="aResult"/>
         <body><![CDATA[
           if (aResult.total !== 0) {
             if (aResult.total == -1) {
               this._foundMatches.value = this.pluralForm.get(
-                this._matchesCountLimit,
+                aResult.limit,
                 this.strBundle.GetStringFromName("FoundMatchesCountLimit")
-              ).replace("#1", this._matchesCountLimit);
+              ).replace("#1", aResult.limit);
             } else {
               this._foundMatches.value = this.pluralForm.get(
                 aResult.total,
                 this.strBundle.GetStringFromName("FoundMatches")
               ).replace("#1", aResult.current)
                .replace("#2", aResult.total);
             }
             this._foundMatches.hidden = false;
--- a/toolkit/modules/Finder.jsm
+++ b/toolkit/modules/Finder.jsm
@@ -389,17 +389,18 @@ Finder.prototype = {
         controller.scrollLine(true);
         break;
     }
   },
 
   _notifyMatchesCount: function(result = this._currentMatchesCountResult) {
     // The `_currentFound` property is only used for internal bookkeeping.
     delete result._currentFound;
-    if (result.total == this.matchesCountLimit)
+    result.limit = this.matchesCountLimit;
+    if (result.total == result.limit)
       result.total = -1;
 
     for (let l of this._listeners) {
       try {
         l.onMatchesCountResult(result);
       } catch (ex) {}
     }