414288 - Simplify TagsSearch to be correct, clear, concise

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Ed Lee :Mardak]

Using the refactored code from bug 414285 and bug 414287.. we can eliminate dead code and reuse the search processing. We'll be dynamically generating tag queries for single term searches, but creating queries is supposed to be fast and it's only one term plus it makes the code clearer and more concise.

Additionally, because we're using the shared search processing code, we will be able to correctly show bookmark titles instead of page titles on tag matches.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Yay unit tests.

Comment 2

[Ed Lee :Mardak]

Comment on attachment 299651 [details] [diff] [review]
v1

nsNavHistory::AutoCompleteTagsSearch()
> nsNavHistory::AutoCompleteTagsSearch()
> {
>   nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>   NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>   nsresult rv;
What's this for? We don't use "bookmarks" anywhere. This is separate from the favicon service for later. Should this be removed?

Comment 3

[Ed Lee :Mardak]

Attachment: v1.1

sdwilsh: For.. mozStorageStatementScoper scope(tagAutoCompleteQuery);
Just making sure it's safe to remove because the mozStorageStatement is created locally and doesn't get assigned from a class member variable anymore.

Passes unit tests. Yay code reuse.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 299850 [details] [diff] [review]
v1.1

r=sdwilsh for storage stuff

Comment 5

[Ed Lee :Mardak]

Attachment: v1.2

No more dummy to ProcessSearch which was already handled for bug 414287.

Comment 6

[Ed Lee :Mardak]

Attachment: v1.3

I'm putting this blocking bug 414287 because bug 414287 will correctly handle bookmark titles, so this bug needs to first provide the title in the SELECT query.

Additionally, I got rid of the unsigned/signed warnings for nsStringArray.Count()

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 301974 [details] [diff] [review]
v1.3

statement creation is fast enough that i'm not worried about adding statements on startup, but slow enough that you don't want to create them inside loops, for example (yes, this actually happened). so just need to be aware this does carry minimal perf risk.

other than that concern, the change looks good. r=me.

Comment 8

[Ed Lee :Mardak]

Comment on attachment 301974 [details] [diff] [review]
v1.3

Correct, clear, concise!

This is needed for the patch in bug 414426 which will help perf. :)

Comment 9

[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.135; previous revision: 1.134
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.41; previous revision: 1.40
done