Closed Bug 414288 Opened 14 years ago Closed 14 years ago

Simplify TagsSearch to be correct, clear, concise

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 beta4

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
Yay unit tests.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #299651 - Flags: review?(seth)
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?
Blocks: 414426
Attached patch v1.1 (obsolete) — Splinter Review
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+
Attached patch v1.2 (obsolete) — Splinter Review
No more dummy to ProcessSearch which was already handled for bug 414287.
Attachment #299850 - Attachment is obsolete: true
Attachment #301079 - Flags: review?
Attached patch v1.3Splinter Review
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?
Blocks: 414287
No longer depends on: 414287
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+
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?
Attachment #301974 - Flags: approval1.9? → approval1.9+
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
Closed: 14 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.