Simplify TagsSearch to be correct, clear, concise

RESOLVED FIXED in Firefox 3 beta4

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

Trunk
Firefox 3 beta4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 299651 [details] [diff] [review]
v1

Yay unit tests.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #299651 - Flags: review?(seth)
(Assignee)

Comment 2

11 years ago
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?
(Assignee)

Updated

11 years ago
Blocks: 414426
(Assignee)

Comment 3

11 years ago
Created attachment 299850 [details] [diff] [review]
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.
Attachment #299651 - Attachment is obsolete: true
Attachment #299850 - Flags: review?(sdwilsh)
Attachment #299651 - Flags: review?(seth)
Comment on attachment 299850 [details] [diff] [review]
v1.1

r=sdwilsh for storage stuff
Attachment #299850 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 5

11 years ago
Created attachment 301079 [details] [diff] [review]
v1.2

No more dummy to ProcessSearch which was already handled for bug 414287.
Attachment #299850 - Attachment is obsolete: true
Attachment #301079 - Flags: review?
(Assignee)

Comment 6

11 years ago
Created attachment 301974 [details] [diff] [review]
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()
Attachment #301079 - Attachment is obsolete: true
Attachment #301974 - Flags: review?(dietrich)
Attachment #301079 - Flags: review?
(Assignee)

Updated

11 years ago
Blocks: 414287
No longer depends on: 414287
(Assignee)

Updated

11 years ago
Blocks: 416211
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.
Attachment #301974 - Flags: review?(dietrich) → review+
(Assignee)

Comment 8

11 years ago
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. :)
Attachment #301974 - Flags: approval1.9?

Updated

11 years ago
Attachment #301974 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 9

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
You need to log in before you can comment on or make changes to this bug.