Closed Bug 504853 Opened 11 years ago Closed 4 years ago

Don't perform a new search if the previous search is the same as this one

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: sdwilsh, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

We shouldn't fire off a new search if the new search has the same trimmed search string as the old search's trimmed search string (so, ignore whitespace differences).
And the autocomplete controller should be giving previous results, so we can just hand those right back.
Whiteboard: [good first bug]
Should we cache the search string that yield zero results for N minutes or something like that? Will this bug delve into native code?
We do not need to cache the search string at all.  The previous results should contain it.
Trunk has an optimization right now where if we search through all of history, we remember that we found 0 results and if the user types more to the end of the query, we can immediately say we have 0 results.

(This is a specific case of searching through history and having less than 12 results, and typing more will just search through those 12 results.)
We can't really know that now with the new code though (unless we get notified on completion, but I'm not sure that that is the common case).
Assignee: nobody → andreaswagner
Status: NEW → ASSIGNED
Attached patch Patch 1.0 (obsolete) — Splinter Review
First bug, first patch.

I hope it's not too bad.
Attachment #467525 - Flags: review?(mak77)
Hmm, I don't think this is quite right.  Shouldn't we be setting search results here to be the previous results?

Maybe we should also add a test that makes sure that we get results if the only change is whitespace?
It's possible that returning will just result in the old results staying visible.. assuming they're visible to begin with. But if you dismiss the results and hit space, the old results should be made to reappear.
Attached patch Patch 1.1Splinter Review
Ok, here is another try. I'm not sure that this will work either, because _finishSearch() will clear _originalSearchString so I can't compare it with the current search term aSearchString. Could anyone please point me to the right direction?

Sorry I first have to get familiar with all the coding rules.
Attachment #467525 - Attachment is obsolete: true
Attachment #467525 - Flags: review?(mak77)
Attachment #468172 - Attachment is patch: true
Attachment #468172 - Flags: review?(mak77)
Comment on attachment 468172 [details] [diff] [review]
Patch 1.1

As you said, this._originalSearchString is deleted on finishSearch, I don't see a problem in not deleting it. So I suggest removing the deletion from finishSearch and rename the property to this._lastOriginalSearchString so it's clearer that it persists.

>diff -r b6a15d2bdb14 toolkit/components/places/src/nsPlacesAutoComplete.js

>+    // If the current search string is the same as the previous
>+    // (after trimming), we don't need to search again,
>+    // just return aPreviousResult
>+    var trimmedSearchString = aSearchString.trim();
>+    if (this._originalSearchString != null) {

this should be if (this._originalSearchString), we delete it so it would be undefined, the result does not change since this is not a strict type check but it's conceptually different

>+      if (trimmedSearchString == this._originalSearchString) {

the two ifs should be unified into one since there is no other code path

the condition should also check aPreviousResult, since if the popup has been dismissed aPreviousResult is null, and we can't reuse it.

>+        this._result = aPreviousResult;
>+        this._finishSearch(true);

finishSearch will use this._listener that has been deleted, but you can move the code "this._listener = aListener" that is just some row below, to run before your code, and that will solve the problem.

this will need a simple test as stated in comment 7, you can check other unit tests in tests/autocomplete/ folder. it should do a search for a word, and a search for the same word with added spaces in front of after it. I can't find such a simple test. Ideally it should also check that if the popup is dismissed and we do the same search, the search is run again but this could be harder to do.
This test will pass regardless your change, but it's just to ensure we don't break there.
Attachment #468172 - Flags: review?(mak77) → review-
notice this could be problematic for open pages, we should add a local var to track wether any open page has been registered or unregistered from the last search, and reuse results only if nothing changed.
mak, is this still relevant? Should we look back into it, or do you think it should still be in the mix for good first bugs? I'm asking you because it looks like you had reviewed the patch. We could also double check with Andreas if he wants to work on it again. Thanks!
Flags: needinfo?(mak77)
yes I think it's still relevant since I don't remember us adding such a mechanism yet. not exactly a good first bug though, more like a good second bug, so better removing it from the list of easy things.
Flags: needinfo?(mak77)
Whiteboard: [good first bug]
Hi Andreas, are you still interested in working on this bug?
Flags: needinfo?(mail)
If you want to take it, feel free to do so! I can't work on this before next week or the week after that.

If you don't, it's also fine with me, I'll try to come up with another patch in a couple of weeks then.
Flags: needinfo?(mail)
Assignee: mail → nobody
Status: ASSIGNED → NEW
Part is done, no reason to continue unless a perf profile shows current status is not enough
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.