512643 - FAYT should be smarter when there are no current matches

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(I swear I filed this before, but I can't find it)

Right now, FAYT will re-search the current document on every keypress -- even if there were previously no matches.  It does this for every character that you type, making searching for a phrase on long pages extremely painful.. if you type "/foobar" intending to search for foobar, and there were no matches after "fo", you'll go through a full slow search for every additional letter you already typed, locking up your browser as we go.

We should be smarter about this -- if there were no matches, there /still/ won't be any matches, *except* if the page maybe had new content added.  At that point, we should wait some amount of time before searching again, or until the page finishes loading, whichever is first.

We may need some way to indicate "no matches found, but page is still loading" in the find bar, instead of making it red, but that can be a followup bug.

STR:
- load http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp
- while it's loading, hit / and start banging on the keyboard.  note that the throbber will often lock up, your keys will fall behind, sometimes windows will gray out the app in a 'this app is unresponsive' way etc.
- once the page has finished loading, hit / and type a bunch of random keys, then g and get some coffee while your browser comes back to life

Comment 1

[John P Baker]

(In reply to comment #0)
> (I swear I filed this before, but I can't find it)

Probably in bug 477131

Not yet marking as duplicate of bug 293889 as you will probably want to move some flags.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: make FAYT suck less

Here's a patch.  It keeps track of the last string length that we had a match at, and if we've typed additional characters, we won't search again.  If we have network activity in the meantime though, and there is no current match, then we will repeat the search until the page stops loading.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Just checking the length would cause problems when the beginning of the search string is modified, wouldn't it?

I think restarting the search on a timer is weird - can't we just block more-specific searches when no results have been found, but undo that blocking if the page is loading (i.e. when we get progress events)?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: better

Here's a much better and simpler patch.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

nit: _findFailedAt is a confusing name, I thought it was an index. _findFailedString?

I think you actually want onStateChange rather than onStatusChange (and you should have an implementation of both). Unconditionally resetting _findFailedAt in both of those will probably mean that this won't have much of an effect at all during page load, since together they'll fire pretty frequently then, but maybe that isn't an issue.

You can pass a more specific filter to addProgressListener so that you avoid getting the callbacks you need.

That patch will have the dynamic-content problem I mentioned yesterday. Perhaps we should address that by resetting _findFailedAt on a timer as well? In fact, perhaps we could just replace the progress listener entirely with a short timeout, which should be sufficient to avoid the hanging-while-typing problem?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: more better

Ok, more better!

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 397169 [details] [diff] [review]
more better

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml

>+	  if (this._findResetTimeout)
>+            clearTimeout(this._findResetTimeout);

Nothing ever clears out _findResetTimeout once set, so this check isn't that useful. clearTimeout never throws, though, so you can just remove it I guess. Or reset _findResetTimeout to -1 in the callback (and in the constructor) and check for that here. This code block is also indented wrong.

>+	  this._findResetTimeout = setTimeout(function(self) { self._findFailedString = null; }, 1000, this);

Only need to set the timeout if _findFailedString is non-null, right?

A test that does something like:

enterText("f");
checkStatus("notfound");
enterText("o");
checkThatWeDidntSearch(); <- not sure if this is possible
document.write("foo");
findAgain(); (and/or toggle findbar open/closed)
checkStatus("found");

on a blank page would be useful. Timeout stuff is a bit harder to reliably test, I guess...

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/97994f5cc1c6

with fixes to the timeout to reset it to -1 in the callback.

Hmm, and I just now found the tests for it (thought we didn't have any); I'll see if I can modify to test for this.  I don't think there's a way to check that we didn't search though, not without explicitly adding code to support it in the findbar code.

Comment 9

[gert_p]

val.indexOf(this._findFailedString) != 0)

shouldn't this read
val.indexOf(this._findFailedString) == -1)

In case the search for "foo" failed, search for "barfoo" can not succeed (in general, search for anything that contains the failed string will also fail)

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

It could, though this patch preserves the redo-search-on-user-modify for when the page is loading.. that was the case that this was trying to handle, someone manually inserting something at the start of the string should cause us to search again.  It's not ideal, but without actually tracking page loading there isn't anything better we could do.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(relanded this)