1294392 - Consolidate the highlight and counter timers into one iterator timer

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Mike de Boer [:mikedeboer]]

Since we merged the iterators used for Highlight All and the find occurrence counter, we can iron out some of the disparate logic that applies to both features.
One of those things is something very important to (perceived) performance: the timers that control the delay between a find action and the follow-up iterator boot kick.
We can generalize the two timers into the FinderIterator so that we have one single place to control this behavior.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1294392 - consolidate the highlight and counter timers into one iterator timer.

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

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8781535 [details]
Bug 1294392 - consolidate the highlight and counter timers into one iterator timer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71950/diff/1-2/

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8781535 [details]
Bug 1294392 - consolidate the highlight and counter timers into one iterator timer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71950/diff/2-3/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781535 [details]
Bug 1294392 - consolidate the highlight and counter timers into one iterator timer.

https://reviewboard.mozilla.org/r/71950/#review71124

::: toolkit/modules/FinderIterator.jsm:66
(Diff revision 3)
> -   * @param {Boolean}  [options.linksOnly]   Only yield ranges that are inside a
> -   *                                         hyperlink (used by QuickFind).
> -   *                                         Optional, defaults to `false`.
> +   * @param {Number}  [options.limit]         Limit the amount of results to be
> +   *                                          passed back. Optional, defaults to no
> +   *                                          limit.

I think its good to require some limit here to make this API less of a footgun if people forget to pass a limit.

::: toolkit/modules/FinderIterator.jsm:616
(Diff revision 3)
> +  /**
> +   * Calculate the Levenshtein distance between two words.
> +   * The implementation of this method was heavily inspired by
> +   * http://locutus.io/php/strings/levenshtein/index.html
> +   * License: MIT.

Can you pull this out to a separate JSM and then call it from the _distance method? I'm not sure how licenses within files is supposed to work, but a separate file should make this clearer.

::: toolkit/modules/tests/xpcshell/test_FinderIterator.js:193
(Diff revision 3)
>      finder: gMockFinder,
>      listener: { onIteratorRangeFound(range) { ++count; } },
>      word: findText
>    });
>  
>    // Start again after a few milliseconds.

This comment is out of date now.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8781535 [details]
Bug 1294392 - consolidate the highlight and counter timers into one iterator timer.

https://reviewboard.mozilla.org/r/71950/#review71130

This is cool that it introduces Levenshtein distance in to the find bar. When is this going to be enabled or disabled?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I should note that there was no mention or reason as to why the distance metric was part of this patch. Seems like it could be separated out to a different bug, but if there was a related reason here then please include that as a comment in the bug and you don't need to create a new bug.

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> I should note that there was no mention or reason as to why the distance
> metric was part of this patch. Seems like it could be separated out to a
> different bug, but if there was a related reason here then please include
> that as a comment in the bug and you don't need to create a new bug.

I added the distance metric to have the highlighter iterator running as a background process and opt-in to currently running searches that are +1 edit distance off from the word it's suggesting to start with.
It's not part of the nsFind algorithm to allow for finding words that may almost match what you're looking for (thus accounting for typos).
What could be done in the future is to have a mechanism built-in to do a find-in-page with a word that _is_ present in the document and within the right edit distance, like we have with `./mach colbber`, for instance.

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> It's not part of the nsFind algorithm to allow for finding words that may
> almost match what you're looking for (thus accounting for typos).

This is fuzzy matching as described in https://en.wikipedia.org/wiki/Approximate_string_matching

Comment 9

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/3434d7b58e308b7bef65933802f38ffbc234d460
Bug 1294392 - consolidate the highlight and counter timers into one iterator timer. r=jaws

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3434d7b58e30