Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar. r=nalexander, a=ritu
authorMike de Boer <mdeboer@mozilla.com>
Fri, 07 Oct 2016 18:23:05 +0200
changeset 356068 e57e85d7494c8e7ed02e55fd6f615ab161b5cc89
parent 356067 2da62d29c6a4826c874bc7f6b9e315b0293b2fc9
child 356069 1632aad24b80fdb2c77d5b416ca96490d7c7f993
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander, ritu
bugs1303008
milestone51.0a2
Bug 1303008 - refactor Fennec Finder.jsm integration to always show the correct match count results in the findbar. r=nalexander, a=ritu MozReview-Commit-ID: 8zTAedLSFlx
mobile/android/base/java/org/mozilla/gecko/FindInPageBar.java
mobile/android/chrome/content/FindHelper.js
--- 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;
-        EventDispatcher.getInstance().registerGeckoThreadListener(this, "TextSelection:Data");
+        EventDispatcher.getInstance().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;
         }
-        EventDispatcher.getInstance().unregisterGeckoThreadListener(this, "TextSelection:Data");
+        EventDispatcher.getInstance().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
@@ -6,16 +6,18 @@
 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;
       }
 
       case "Tab:Selected": {
@@ -26,44 +28,42 @@ 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 +73,127 @@ 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);
+    this._finder.requestMatchesCount(searchString, this._limit);
+    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);
+    this._finder.requestMatchesCount(searchString, this._limit);
+    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;
+    this._result.limit = this._limit;
+
+    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
         }