Closed
Bug 1294392
Opened 8 years ago
Closed 8 years ago
Consolidate the highlight and counter timers into one iterator timer
Categories
(Toolkit :: Find Toolbar, defect)
Tracking
()
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
mozreview-review |
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+
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
(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.
Assignee | ||
Comment 8•8 years ago
|
||
(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
Assignee | ||
Comment 9•8 years ago
|
||
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•