show tag results in url bar autocomplete As an intermediate step between where I am with it right now (https://bugzilla.mozilla.org/show_bug.cgi?id=395161) and faaborg's mockup (http://people.mozilla.com/~faaborg/files/granParadisoUI/places_UnifyingSearch_i1.png), here's what I'd like to propose for m8 (meaning, something I think I can finish and get reviewed today.) Upon typing in the url bar: * prioritize exact (case insensitive) tag matches to the top, and use the tag icon instead of the star. among the tag matches, sort by frecency. The reasoning here is to promote tagging, to make finding tagged items easier, and now users can use tags for the additional purpose of affecting search results. Say I want g to be google. I can tag google.com with g and if that is the only page tagged with g, it will always be first. I'd rather users use tags to do this, than just bookmarking. * after the tag matches, the frecency results for bookmarks + the first chunk of history. the reason for this is so that unvisited bookmarks will be "near the top", yet not completely outweighing unbookmarked visits. * after all that, continue on with the rest of history (in the current asynchronous fashion). Note, this should be as fast as things currently are, as tag searching and bookmark searching fast as those are typicallly several orders of magnitude less than history.

taking, patch in hand.

Comment on attachment 279981 [details] [diff] [review] patch, v1 note, some of the changes to nsNavHistory::StartSearch() are not part of this fix, but are problems I spotted while working on this code. for example, we were searching previous results when doing a "typed only" search, getting the match count of the previous results (for no reason), a warning fix PRInt32 -> PRUint32, and save calls to PR_Now() and avoid a query to determine the earliest visit if we are doing a "typed only" search

Comment on attachment 279989 [details] [diff] [review] patch, v2. my bookmark query is flooding results and is too slow, I'll take showing unvisited bookmarks off to another bug >-// The auto complete query we use depends on options, so we have it in >+// The auto complete queries we use depends on options, so we have it in nits: s/depends/depend/ s/it/them/ >+ >+ sql = NS_LITERAL_CSTRING( >+ "SELECT h.url, h.title, f.url, b.id, b.parent " >+ "FROM moz_places h " >+ "JOIN moz_bookmarks b ON b.fk = h.id " >+ "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id " >+ "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id " >+ "WHERE " >+ "(b.parent = (SELECT t.id FROM moz_bookmarks t WHERE t.parent = ?1 and t.title LIKE ?2 ESCAPE '/')) " >+ "GROUP BY h.id ORDER BY h.visit_count DESC, MAX(v.visit_date) DESC;"); >+ rv = mDBConn->CreateStatement(sql, getter_AddRefs(mDBTagAutoCompleteQuery)); >+ NS_ENSURE_SUCCESS(rv, rv); > >- return mDBConn->CreateStatement(sql, getter_AddRefs(mDBAutoCompleteQuery)); >+ return NS_OK; > } > >@@ -234,47 +259,25 @@ nsNavHistory::StartSearch(const nsAStrin > nsresult rv; > > // determine if we can start by searching through the previous search results. > // if we can't, we need to reset mCurrentChunkEndTime and mCurrentOldestVisit. > // if we can, we will search through our previous search results and then resume > // searching using the previous mCurrentChunkEndTime and mCurrentOldestVisit values. > PRBool searchPrevious = PR_FALSE; > if (aPreviousResult) { >+ // if search string begins with the previous search string, it's a go. >+ // but don't search previous results if the previous search string was empty >+ // or if the current serach string is empty. (an empty search string is a "typed only" s/serach/search/ > > PRBool isMatch = CaseInsensitiveFindInReadable(mCurrentSearchString, url); > if (!isMatch) > isMatch = CaseInsensitiveFindInReadable(mCurrentSearchString, title); > > if (isMatch) { >- nsAutoString image, style; >- aPreviousResult->GetImageAt(i, image); >+ nsAutoString style; > aPreviousResult->GetStyleAt(i, style); >- >- mCurrentResultURLs.Put(url, PR_TRUE); >+ >+ // if the previous result item was a tagged item, and it matches >+ // the current search term (due to url or title) don't include it. >+ // find it "naturally" with a history or bookmark search. >+ // >+ // note, if the previous result item is tagged with the current search term, >+ // our call to AutoCompleteTagsSearch() above will find it. >+ if (!style.Equals(NS_LITERAL_STRING("tag"))) { could you do this check before the matching? don't you want to throw out all previous tag entries in the previous result? >+ // Determine the result of the search >+ while (NS_SUCCEEDED(mDBTagAutoCompleteQuery->ExecuteStep(&hasMore)) && hasMore) { >+ nsAutoString entryURL, entryTitle, entryFavicon; >+ mDBTagAutoCompleteQuery->GetString(kAutoCompleteIndex_URL, entryURL); >+ mDBTagAutoCompleteQuery->GetString(kAutoCompleteIndex_Title, entryTitle); >+ mDBTagAutoCompleteQuery->GetString(kAutoCompleteIndex_FaviconURL, entryFavicon); >+ PRInt64 itemId = 0; >+ mDBTagAutoCompleteQuery->GetInt64(kAutoCompleteIndex_ItemId, &itemId); >+ PRInt64 parentId = 0; >+ mDBTagAutoCompleteQuery->GetInt64(kAutoCompleteIndex_ParentId, &parentId); it's pretty standard in the rest of places code to check the rv of mozStorage's Get* value accessors, would that be overkill here?

1) nits: s/depends/depend/ s/it/them/ fixed, thanks. 2) s/serach/search/ fixed, thanks. 3) could you do this check before the matching? don't you want to throw out all previous tag entries in the previous result? yes, good catch! fixed, thanks. 4) it's pretty standard in the rest of places code to check the rv of mozStorage's Get* value accessors, would that be overkill here? fixed, thanks.

Comment on attachment 280011 [details] [diff] [review] patch v3, address dietrich's review comments r=me, thanks.

Comment on attachment 280011 [details] [diff] [review] patch v3, address dietrich's review comments seeking approval for m8

fixed. Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHis tory.cpp new revision: 1.168; previous revision: 1.167 done Checking in toolkit/components/places/src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHisto ry.h new revision: 1.97; previous revision: 1.96 done Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v < -- nsNavHistoryAutoComplete.cpp new revision: 1.18; previous revision: 1.17 done Checking in browser/themes/pinstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v <-- browser.cs s new revision: 1.76; previous revision: 1.75 done Checking in browser/themes/pinstripe/browser/jar.mn; /cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v <-- jar.mn new revision: 1.53; previous revision: 1.52 done RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/places/tag.png,v done Checking in browser/themes/pinstripe/browser/places/tag.png; /cvsroot/mozilla/browser/themes/pinstripe/browser/places/tag.png,v <-- tag.png initial revision: 1.1 done Checking in browser/themes/winstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v <-- browser.cs s new revision: 1.91; previous revision: 1.90 done Checking in browser/themes/winstripe/browser/jar.mn; /cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v <-- jar.mn new revision: 1.49; previous revision: 1.48 done RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/places/tag.png,v done Checking in browser/themes/winstripe/browser/places/tag.png; /cvsroot/mozilla/browser/themes/winstripe/browser/places/tag.png,v <-- tag.png initial revision: 1.1 done

Seth: Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090704 Minefield/3.0a8pre, I don't see my locations chunked out like I do in your screenshot (Locations, Tags, Bookmarks): https://bugzilla.mozilla.org/attachment.cgi?id=279966. Another issue I noticed: I get two instances of the site that I tagged in the URL bar - one that has a star and one that has the tag icon. I can grab a screenshot if you need it.

ignore Comment 11 - sorry, it looks as if this landed late in the cycle and did not make the first build out the door, which is the one I am testing.

sorry marcia, my original screen shot was for https://bugzilla.mozilla.org/show_bug.cgi?id=395161, not this bug. I've replaced the screen shot.

> Another issue I noticed: I get two instances of the site that I tagged in the > URL bar - one that has a star and one that has the tag icon. I can grab a > screenshot if you need it. I have code in there that is supposed to prevent that from happening. can you log a spin off bug with a screen shot? I'll try to reproduce that bug here as well.

Bug 395445 has been filed to cover the spin off issue. (In reply to comment #15) > > Another issue I noticed: I get two instances of the site that I tagged in the > > URL bar - one that has a star and one that has the tag icon. I can grab a > > screenshot if you need it. > > I have code in there that is supposed to prevent that from happening. can you > log a spin off bug with a screen shot? I'll try to reproduce that bug here as > well. >

Litmus Triage Team: stephend, will you cover this one?

(In reply to comment #17) > Litmus Triage Team: stephend, will you cover this one? Sure; http://litmus.mozilla.org/show_test.cgi?id=4680 in-litmus+

Verified FIXED using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre