Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operations there are on a page. r?jaws draft
authorMike de Boer <mdeboer@mozilla.com>
Wed, 12 Oct 2016 15:46:44 +0200
changeset 424263 90a605da404918a77dbdb106e14502e2440eff33
parent 424216 aa8629a25ab5aeb2678a0187e50336659d368b44
child 533632 14b86cbdd73066e19baf458a73a5c0264e5f533c
push id32110
push usermdeboer@mozilla.com
push dateWed, 12 Oct 2016 13:49:06 +0000
reviewersjaws
bugs1305194
milestone52.0a1
Bug 1305194 - wait a little longer when the finder iterator is requested to find a query of only one or two characters, which improves usability due to less flickering of highlighter results and performance due to avoiding the most costly nsFind operations there are on a page. r?jaws Single and double character find operations cause a big hit on nsFind, because it usually yields many more occurrences than other queries. But most importantly, it needs to keep iterating each text node much longer than other queries, because there is much less escapes/ loop exits happening. This also fixes the FinderIterator infinite-depth call-stack due to unresolved promises that remain active during the lifetime of the FinderIterator. Unwinding the call-stack properly should save us bytes from the heap. MozReview-Commit-ID: 4K19X0yngC7
toolkit/content/tests/chrome/findbar_window.xul
toolkit/modules/FinderIterator.jsm
toolkit/modules/tests/browser/browser_FinderHighlighter.js
--- a/toolkit/content/tests/chrome/findbar_window.xul
+++ b/toolkit/content/tests/chrome/findbar_window.xul
@@ -26,17 +26,17 @@
     ContentTask.setTestScope(window.opener.wrappedJSObject);
 
     var gPrefsvc = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
 
     const SAMPLE_URL = "http://www.mozilla.org/";
     const SAMPLE_TEXT = "Some text in a text field.";
     const SEARCH_TEXT = "Text Test";
     const NOT_FOUND_TEXT = "This text is not on the page."
-    const ITERATOR_TIMEOUT = gPrefsvc.getIntPref("findbar.iteratorTimeout") + 20;
+    const ITERATOR_TIMEOUT = gPrefsvc.getIntPref("findbar.iteratorTimeout");
 
     var gFindBar = null;
     var gBrowser;
 
     var gClipboard = Cc["@mozilla.org/widget/clipboard;1"].getService(Ci.nsIClipboard);
     var gHasFindClipboard = gClipboard.supportsFindClipboard();
 
     var gStatusText;
@@ -250,17 +250,17 @@
         let listener = {
           onMatchesCountResult: function() {
             gFindBar.browser.finder.removeResultListener(listener);
             resolve();
           }
         };
         gFindBar.browser.finder.addResultListener(listener);
         // Make sure we resolve _at least_ after five times the find iterator timeout.
-        setTimeout(resolve, ITERATOR_TIMEOUT * 5);
+        setTimeout(resolve, (ITERATOR_TIMEOUT * 5) + 20);
       });
     }
 
     var enterStringIntoFindField = Task.async(function* (str, waitForResult = true) {
       for (let promise, i = 0; i < str.length; i++) {
         if (waitForResult) {
           promise = promiseFindResult();
         }
@@ -518,17 +518,17 @@
       ok(document.commandDispatcher.focusedElement == gFindBar._findField.inputField,
          "testFindCountUI: find field is not focused");
 
       let promise;
       let matchCase = gFindBar.getElement("find-case-sensitive");
       if (matchCase.checked) {
         promise = promiseFindResult();
         matchCase.click();
-        yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT));
+        yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT + 20));
         yield promise;
       }
 
       let foundMatches = gFindBar._foundMatches;
       let tests = [{
         text: "t",
         current: 5,
         total: 10,
@@ -553,26 +553,32 @@
         is(aMatches[2], String(aTest.total),
           `Total amount of matches should be ${aTest.total} for '${aTest.text}'`);
       }
 
       for (let test of tests) {
         gFindBar._findField.select();
         gFindBar._findField.focus();
 
-        yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT));
+        let timeout = ITERATOR_TIMEOUT;
+        if (test.text.length == 1)
+          timeout *= 4;
+        else if (test.text.length == 2)
+          timeout *= 2;
+        timeout += 20;
+        yield new Promise(resolve => setTimeout(resolve, timeout));
         yield enterStringIntoFindField(test.text, false);
         yield promiseMatchesCountResult();
         let matches = foundMatches.value.match(regex);
         if (!test.total) {
           ok(!matches, "No message should be shown when 0 matches are expected");
         } else {
           assertMatches(test, matches);
           for (let i = 1; i < test.total; i++) {
-            yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT));
+            yield new Promise(resolve => setTimeout(resolve, timeout));
             gFindBar.onFindAgainCommand();
             yield promiseMatchesCountResult();
             // test.current + 1, test.current + 2, ..., test.total, 1, ..., test.current
             let current = (test.current + i - 1) % test.total + 1;
             assertMatches({
               text: test.text,
               current: current,
               total: test.total
@@ -654,17 +660,17 @@
 
     function* testToggleEntireWord() {
       yield openFindbar();
       let promise = promiseFindResult();
       yield enterStringIntoFindField("Tex", false);
       let result = yield promise;
       is(result.result, Ci.nsITypeAheadFind.FIND_FOUND, "Text should be found");
 
-      yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT));
+      yield new Promise(resolve => setTimeout(resolve, ITERATOR_TIMEOUT + 20));
       promise = promiseFindResult();
       let check = gFindBar.getElement("find-entire-word");
       check.click();
       result = yield promise;
       is(result.result, Ci.nsITypeAheadFind.FIND_NOTFOUND, "Text should NOT be found");
 
       check.click();
       gFindBar.close(true);
--- a/toolkit/modules/FinderIterator.jsm
+++ b/toolkit/modules/FinderIterator.jsm
@@ -165,16 +165,20 @@ this.FinderIterator = {
   stop(cachePrevious = false) {
     if (!this.running)
       return;
 
     if (this._timer) {
       clearTimeout(this._timer);
       this._timer = null;
     }
+    if (this._runningFindResolver) {
+      this._runningFindResolver();
+      this._runningFindResolver = null;
+    }
 
     if (cachePrevious) {
       this._previousRanges = [].concat(this.ranges);
       this._previousParams = Object.assign({}, this._currentParams);
     } else {
       this._previousRanges = [];
       this._previousParams = null;
     }
@@ -215,16 +219,20 @@ this.FinderIterator = {
    * previous result invalid.
    * If the iterator is running, it will be stopped as soon as possible.
    */
   reset() {
     if (this._timer) {
       clearTimeout(this._timer);
       this._timer = null;
     }
+    if (this._runningFindResolver) {
+      this._runningFindResolver();
+      this._runningFindResolver = null;
+    }
 
     this._catchingUp.clear();
     this._currentParams = this._previousParams = null;
     this._previousRanges = [];
     this.ranges = [];
     this.running = false;
 
     this._notifyListeners("reset");
@@ -416,18 +424,32 @@ this.FinderIterator = {
    *                               is not, this identifier is used to learn if
    *                               it's supposed to still continue after a pause.
    * @yield {nsIDOMRange}
    */
   _findAllRanges: Task.async(function* (finder, spawnId) {
     if (this._timeout) {
       if (this._timer)
         clearTimeout(this._timer);
-      yield new Promise(resolve => this._timer = setTimeout(resolve, this._timeout));
-      this._timer = null;
+      if (this._runningFindResolver)
+        this._runningFindResolver();
+
+      let timeout = this._timeout;
+      let searchTerm = this._currentParams.word;
+      // Wait a little longer when the first or second character is typed into
+      // the findbar.
+      if (searchTerm.length == 1)
+        timeout *= 4;
+      else if (searchTerm.length == 2)
+        timeout *= 2;
+      yield new Promise(resolve => {
+        this._runningFindResolver = resolve;
+        this._timer = setTimeout(resolve, timeout);
+      });
+      this._timer = this._runningFindResolver = null;
       // During the timeout, we could have gotten the signal to stop iterating.
       // Make sure we do here.
       if (!this.running || spawnId !== this._spawnId)
         return;
     }
 
     this._notifyListeners("start", this.params);
 
--- a/toolkit/modules/tests/browser/browser_FinderHighlighter.js
+++ b/toolkit/modules/tests/browser/browser_FinderHighlighter.js
@@ -216,17 +216,22 @@ add_task(function* testModalResults() {
   let url = kFixtureBaseURL + "file_FinderSample.html";
   yield BrowserTestUtils.withNewTab(url, function* (browser) {
     let findbar = gBrowser.getFindBar();
 
     for (let [word, expectedResult] of tests) {
       yield promiseOpenFindbar(findbar);
       Assert.ok(!findbar.hidden, "Findbar should be open now.");
 
-      yield new Promise(resolve => setTimeout(resolve, kIteratorTimeout));
+      let timeout = kIteratorTimeout;
+      if (word.length == 1)
+        timeout *= 4;
+      else if (word.length == 2)
+        timeout *= 2;
+      yield new Promise(resolve => setTimeout(resolve, timeout));
       let promise = promiseTestHighlighterOutput(browser, word, expectedResult,
         expectedResult.extraTest);
       yield promiseEnterStringIntoFindField(findbar, word);
       yield promise;
 
       findbar.close(true);
     }
   });