Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar. r?nalexander draft
authorMike de Boer <mdeboer@mozilla.com>
Mon, 10 Oct 2016 11:53:25 +0200
changeset 423129 761afeed758a8d4b623b9166a99ce09177e64ec7
parent 423090 d72cf6ecebaf707c159f6697d9f5f6f6a6feb7e1
child 533368 6f8b8bddaa882f8a7fa298cb6aa4884b42c47b9d
push id31808
push usermdeboer@mozilla.com
push dateMon, 10 Oct 2016 09:53:55 +0000
reviewersnalexander
bugs1303008
milestone52.0a1
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) {}
     }