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

Status: RESOLVED WORKSFORME

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

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).

Comment 1

[Ed Lee :Mardak]

And the autocomplete controller should be giving previous results, so we can just hand those right back.

Comment 2

[David Dahl :ddahl]

Should we cache the search string that yield zero results for N minutes or something like that? Will this bug delve into native code?

Comment 3

[Shawn Wilsher :sdwilsh]

We do not need to cache the search string at all.  The previous results should contain it.

Comment 4

[Ed Lee :Mardak]

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.)

Comment 5

[Shawn Wilsher :sdwilsh]

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).

Comment 6

[Andreas Wagner [:TheOne] [use NI]]

Attachment: Patch 1.0

First bug, first patch.

I hope it's not too bad.

Comment 7

[Shawn Wilsher :sdwilsh]

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?

Comment 8

[Ed Lee :Mardak]

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.

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

Attachment: Patch 1.1

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.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Andreas, are you still interested in working on this bug?

Comment 15

[Andreas Wagner [:TheOne] [use NI]]

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.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Part is done, no reason to continue unless a perf profile shows current status is not enough