414287 - Share search result processing logic as AutoCompleteProcessSearch

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Ed Lee :Mardak]

All the searches need to provide the same information to the nsIAutoCompleteResult, so we should make the code sharable to help reduce bugs.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

The only user of aStyle right now is EmptyString(), but that'll be changed in a followup..

Comment 2

[Ed Lee :Mardak]

Attachment: v1.1

According to faaborg/beltzner, we should always favor the bookmark title over page title unless the search doesn't match the bookmark and does match the page title. Yay unit tests.

Comment 3

[Ed Lee :Mardak]

Attachment: v1.2

Don't use the bookmark title if it's empty (null in the db). That should be right sdwilsh? GetString( <null> ) will go through Variant magic and read out as EmptyString(), yeah?

I suppose you could unofficially review this refactoring change as well... :) ;)

(It passes the unit tests :))

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 299849 [details] [diff] [review]
v1.2

>+      PRBool useBookmarkTitle = itemId && !entryBookmarkTitle.IsEmpty() &&
So, if the column is null it will actually be a void string:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/storage/src/mozStorageStatement.cpp&rev=1.29#692

However, testing IsEmpty will still be true for a void string, so you are OK.

r=sdwilsh

Comment 5

[Ed Lee :Mardak]

Attachment: v2

Use tag search's process code because FullHistory filters results outside of SQL. Also, we can unescape like Fullhistory does.

Comment 6

[Ed Lee :Mardak]

Comment on attachment 301078 [details] [diff] [review]
v2

Refactoring to share this logic between tags and bug 395739 for adaptive.

Also adds the unescaping of the urls.

Comment 7

[Ed Lee :Mardak]

Comment on attachment 301078 [details] [diff] [review]
v2

New patch incoming that cleans up some stuff between the two search processing functions.

Comment 8

[Ed Lee :Mardak]

Attachment: v3

Restore the meaning of "aHasMoreResults" and decide if we should chunk outside of FullHistory.

Get some perf win if we have enough results from either tags, so don't bother querying for more fullhistory searches.

We aren't filtering duplicates because we're entering the escaped url and checking with unescaped :( oops.

Unescape urls for tags.

Use bookmark title if we have it.

Get rid of unsigned/signed warning for nsStringArray.Count()

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 301979 [details] [diff] [review]
v3

r=me for these changes.

re:

Mardak: dietrich: i was trying to see if i could/should share the logic between fullhistory and tags/adaptive.. but that seemed like it would either need a function pointer or extra parameters that conditionally decides to filter results, etc..
[8:40pm] Mardak: probably just better to leave them separate, so they're both faster?

should be easy enough to measure if adding a check for a bool param has a detectable perf impact.

Comment 10

[Ed Lee :Mardak]

Attachment: v4

I tried this out and there didn't seem to be any changes to the FullHistory processing time.

Basically v3 without the duplicate code for processing Tags. Also, there's an enum to choose which type of query processing to use. r? to make sure the style is acceptable.

Comment 11

[Ed Lee :Mardak]

Comment on attachment 302674 [details] [diff] [review]
v4

r=dietrich over irc

<dietrich> Mardak_: looks fine

This will help fix a couple bugs like bug 416211 and bug 416214 and helps implement bug 395739.

I'll land the testcase in bug 416214 with this patch.

Comment 12

[Ed Lee :Mardak]

Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.137; previous revision: 1.136
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.42; previous revision: 1.41
done