After profiling the highlighter with the Gecko Profiler, I noticed that a large deal of time is spent running two iterators for the find counter AND the highlighter. This is _not_ cheap, neither optimal. Therefore it's a good idea, I think, to merge the two iterators into a FinderIterator module that both can use. It'd also provide a good place to further investigate performance and behavior tweaks.

Comment on attachment 8764922 [details] [diff] [review] Patch 2: adjust tests a bit to work with the new FindIterator I'm adding another patch soon with tests dedicated to the new module.

Review commit: https://reviewboard.mozilla.org/r/60520/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60520/

Review commit: https://reviewboard.mozilla.org/r/60522/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60522/

Comment on attachment 8768918 [details] Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60520/diff/1-2/

Comment on attachment 8768919 [details] Bug 1281421 - adjust tests a bit to work with the new FindIterator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60522/diff/1-2/

Comment on attachment 8768918 [details] Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton. https://reviewboard.mozilla.org/r/60520/#review60188 ::: toolkit/modules/Finder.jsm:189 (Diff revision 2) > let searchString = this._fastFind.searchString; > this._notify({ > searchString, > result: this._lastFindResult, > findBackwards: aFindBackwards, > - fidnAgain: true, > + findAgain: true, yikes lol ::: toolkit/modules/FinderIterator.jsm:299 (Diff revision 2) > + this._catchingUp.delete(onRange); > + }), > + > + /** > + * Internal; see the documentation of the start() method above. > + * nit, trailing whitespace

Comment on attachment 8768919 [details] Bug 1281421 - adjust tests a bit to work with the new FindIterator. https://reviewboard.mozilla.org/r/60522/#review60190

Review commit: https://reviewboard.mozilla.org/r/67426/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67426/

Comment on attachment 8768918 [details] Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60520/diff/2-3/

Comment on attachment 8768919 [details] Bug 1281421 - adjust tests a bit to work with the new FindIterator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60522/diff/2-3/

Comment on attachment 8775180 [details] Bug 1281421 - add new test to cover the new FinderIterator module code. https://reviewboard.mozilla.org/r/67426/#review64528 r=me with the following question answered and possibly test adjusted ::: toolkit/modules/tests/xpcshell/test_FinderIterator.js:147 (Diff revision 1) > + let rangeCount = 2143; > + prepareIterator(findText, rangeCount); > + > + // Start off the iterator. > + let count = 0; > + let whenDone = FinderIterator.start({ > + finder: gMockFinder, > + onRange: range => ++count, > + word: findText > + }); > + > + // Start again after a few milliseconds. > + yield new Promise(resolve => gMockWindow.setTimeout(resolve, 2)); > + Assert.ok(FinderIterator.running, "We ought to be running here"); > + > + let count2 = 0; > + let whenDone2 = FinderIterator.start({ > + finder: gMockFinder, > + onRange: range => ++count2, > + word: findText > + }); > + > + // Let the iterator run for a little while longer before we assert the world. > + yield new Promise(resolve => gMockWindow.setTimeout(resolve, 10)); > + FinderIterator.stop(); > + > + Assert.ok(!FinderIterator.running, "Stop means stop"); > + > + yield whenDone; > + yield whenDone2; > + > + Assert.greater(count, 0, "At least one range should've been found"); Just a note here because I had to go back and look. Correct me if I'm wrong: The FinderIterator looks for the first kIterationMax items before pausing by using a setTimeout(..., 0). After kIterationMax items have been found, each subsequent find will trigger another setTimeout(..., 0) call. If the caller yields on the promise returned by FinderIterator.start, then the full rangeCount will be found. But if the caller uses FinderIterator.stop(); before yielding on the promise, then the rangeCount will be equal to kIterationMax. If however, in this last test case, the caller also calls setTimeout(..., 10) and pauses for 10 milliseconds before resuming and subsequently calling FinderIterator.stop(), the rangeCount will be equal to kIterationMax plus however many iterations FinderIterator was able to run past kIterationMax. With all that being said and if my understanding is correct, can you update these last test cases to use Assert.greater(count, kIterationMax, ...) instead of 0?

Comment on attachment 8768918 [details] Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60520/diff/3-4/

Comment on attachment 8768919 [details] Bug 1281421 - adjust tests a bit to work with the new FindIterator. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60522/diff/3-4/

Comment on attachment 8775180 [details] Bug 1281421 - add new test to cover the new FinderIterator module code. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67426/diff/1-2/

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e525e1000aa7 Merge the find counter and highlighter iterators into a FinderIterator singleton. r=jaws https://hg.mozilla.org/integration/autoland/rev/276b214dde0d adjust tests a bit to work with the new FindIterator. r=jaws https://hg.mozilla.org/integration/autoland/rev/f962e7e8fb10 add new test to cover the new FinderIterator module code. r=jaws

r=eslint itself.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/705ce7870772 followup - fix ESLint error, even though the syntax was correct. r=me

https://hg.mozilla.org/mozilla-central/rev/e525e1000aa7 https://hg.mozilla.org/mozilla-central/rev/276b214dde0d https://hg.mozilla.org/mozilla-central/rev/f962e7e8fb10 https://hg.mozilla.org/mozilla-central/rev/705ce7870772