Closed Bug 1294392 Opened 3 years ago Closed 3 years ago

Consolidate the highlight and counter timers into one iterator timer

Categories

(Toolkit :: Find Toolbar, defect)

50 Branch
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
mozilla51
Iteration:
51.1 - Aug 15
Tracking Status
firefox51 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Flags: qe-verify-
Flags: firefox-backlog+
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 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?
Attachment #8781535 - Flags: review?(jaws) → review+
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.
(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.
(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
https://hg.mozilla.org/integration/fx-team/rev/3434d7b58e308b7bef65933802f38ffbc234d460
Bug 1294392 - consolidate the highlight and counter timers into one iterator timer. r=jaws
https://hg.mozilla.org/mozilla-central/rev/3434d7b58e30
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1306320
No longer blocks: 1306320
You need to log in before you can comment on or make changes to this bug.