problems re-using autocomplete results due to bookmark titles from https://bugzilla.mozilla.org/show_bug.cgi?id=394038#c48 another problem I came across while testing: say you visit http://bar.com and the title is foox, but you bookmark it with the title foo. type "foo", and you'll get: foo http://bar.com keep typing to get "foox" and you should get: foox http://bar.com but you will get nothing. this is because of an optimization in the code to re-use previous search result if the old search term (foo) is a subset of the new search term (foox). when re-using the previous search result, the title is "foo", so we won't match. One solution it do not do this re-using optimization, but that would be a perf hit. Another solution is to include both results: foo http://bar.com foox http://bar.com (currently, we use the url to remove duplicates.) But this has problems because is the worst case, you'd see n+1 result for the same result (if there were n bookmarks), and the + 1 comes from the title.

If we removed the caching optimization, would it help if we varied the chunk sizes as we search for more results? Similar to mconnor's suggestion for the download manager.. quickly return a small chunk with a short timeout until the 2nd chunk. Then the chunks get bigger and timeouts longer. For the current patch in bug 394038, the chunk size is always 100 and timeout is 100ms. The first chunk would contain the most frecent pages that the user visits, and the user is probably more likely to pick a result out of these pages. E.g., 25, 50, 100, 200, 200, 200.. (double size max at 200) One thing to worry about is that results from earlier chunks will always appear before later results which might cause issues with adaptive or other rankings. Just curious.. what is the timeout for anyway? The comments say too big of a timeout (too small of a chunk) will have 'unresponsive UI'. So what if we make the timeout smaller? We would probably want a dynamic timeout anyway; if the first chunk returns no results, we would want to check the second chunk right away. Also, if we have more than enough results, we shouldn't bother looking for more. Side issue: Do we want the adaptive feedback to bump frecency values so that they're more likely to appear in the first chunk? This would probably be more needed if we have a really small first chunk.

dietrich, in case it was not clear from comment #0, this problem gets worse if you have a single place bookmarked multiple times.

> For the current patch in bug 394038, the chunk size is always 100 and timeout > is 100ms. the chunk size of 100 was just > Just curious.. what is the timeout for anyway? The comments say too big of a > timeout (too small of a chunk) will have 'unresponsive UI'. So what if we make > the timeout smaller? if the chunk size is too big, the ui will be unresponsive while we query the db and process the results. if the timeout is too small, we notice the delay while typing in the url bar, due to all the work. this is how I ended up with 100ms. > One thing to worry about is that results from earlier chunks will always > appear before later results which might cause issues with adaptive or other > rankings. as far as global frecency goes, this is not an issue. before global frecency, choosing a small chunksize (think 1), would have broken "poor man's frececny". but you are right, this will impact adaptive or other rankings that are per chunk. (again, think chunk size of 1) > Also, if we have more than enough results, we shouldn't bother looking for more. if we stop caching in order to fix this bug, then you are correct, we should stop after we have called AppendMatch() [including tag matches and non-tag matches] browser.urlbar.maxRichResults times, since searching for more does us no good (it does when we cache, though).

(In reply to comment #3) > the chunk size of 100 was just Was there something more? > if the chunk size is too big, the ui will be unresponsive while we query the db > and process the results. > if the timeout is too small, we notice the delay while typing in the url bar, > due to all the work. So both cases are because there would be too much background work? Does that come from the earlier queries or later ones finding overly-many results so that can be cached?

>> the chunk size of 100 was just > Was there something more? ...was just a guess, a usable value while I was working on frecency. Note, dietrich has since changed it to 10, see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#194 Upon idle (1 minute by default), we run two queries: 1) find the top 10 places with frecency = -1, see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#1021 for details 2) find the top 10 places that have a frecency, but it the last visit date is "old", so perhaps we need to recalculate. see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#1031

dietrich, we are no longer using cached results, right? problems re-using autocomplete results due to bookmark titles and cached results. http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#291 291 // XXX Re-use of previous search results is disabled due to bug 412730. 292 //if (aPreviousResult) { 293 if (0) { so, technically, this bug is fixed. to quote sayre: "Make it correct, make it clear, make it concise, make it fast. In that order." (https://bugzilla.mozilla.org/show_bug.cgi?id=408914#c16) as is, we are now "correct" and the expense of that optimization. as I wrote previously, this benefits edward lee as well, as reusing results impacts some of his work, too. my suggestion, remove this code: 286 // determine if we can start by searching through the previous search results. 287 // if we can't, we need to reset mCurrentChunkOffset 288 // if we can, we will search through our previous search results and then resume 289 // searching using the previous mCurrentOldestVisit and mLast values. 290 PRBool searchPrevious = PR_FALSE; 291 // XXX Re-use of previous search results is disabled due to bug 412730. 292 //if (aPreviousResult) { 293 if (0) { 294 nsAutoString prevSearchString; 295 aPreviousResult->GetSearchString(prevSearchString); 296 297 // if search string begins with the previous search string, it's a go. 298 // but don't search previous results if the previous search string was empty 299 // or if the current search string is empty. (an empty search string is a "typed only" 300 // search from when clicking on the drop down to the right of the url bar.) 301 searchPrevious = !prevSearchString.IsEmpty() && Substring(mCurrentSearchString, 0, 302 prevSearchString.Length()).Equals(prevSearchString); 303 } and replace it with: // we can not use aPreviousResult, see bug #412730 for details what do you think? I'll quickly slap the old r= on that, in case someone else wants to help out dietrich with this code cleanup so we can mark this bug as fixed.

Somewhat related.. should we continue to search the history if we somehow know there won't be any results? E.g., you have a really long url in the location bar and start to edit it.. every character you type will trigger a search even if you're typing in a new url. places in db: http://site/ current url: http://site/ user types: "typing_in_a_new_page" to get http://site/typing_in_a_new_page Every character typed will result in no results.

(In reply to comment #6) > my suggestion, remove this code: ... There's a ton of code below this part that checks searchPrevious and other things. What to do about that code? > and replace it with: > > // we can not use aPreviousResult, see bug #412730 for details

Removing check and dead code. Added a comment at the top of the function.

Comment on attachment 299443 [details] [diff] [review] v1 r=sspitzer, thanks edward for the code cleanup. Is mCurrentSearchString ever empty? if it were, we'd be doing AutoCompleteTypedSearch(), right? can you confirm and if so, remove the if () check? I think we are, and we always want to be, resetting mCurrentChunkOffset to 0 (if we are not re-searching the previous results.)

(In reply to comment #10) > Is mCurrentSearchString ever empty? > if it were, we'd be doing AutoCompleteTypedSearch(), right? > can you confirm and if so, remove the if () check? Right. It should be safe to always reset mCurrentChunkOffset to 0 when starting a new search for both "typed" and "full". Do we want the chunking aspect (and adaptive learning) for "typed"? Similar to bug 406359? I'm curious.. what happens when we're searching and the user types something. This causes the lists to get emptied and mCurrentSearchString changes to the new input.. but a previously started search timeout might come back and search again. It seems like the way the code is structured, it should be okay, but there potentially might be two or more PreformAutoComplete timer callbacks going off at the same time causing them to do the same search wasting cycles?

(In reply to comment #11) > > can you confirm and if so, remove the if () check? Removed and moved up with the other resetting code. > but a previously started search timeout might come back and search again Nevermind! mAutoCompleteTimer should make sure there's only one timer going off. If there happens to be two.. the later will override the former.

> Do we want the chunking aspect (and adaptive learning) for "typed"? Similar to > bug 406359? no, I don't think we do, but it all depends on bug #411293. as of what's checked in, "typed" is now by frecency, and we have a fast query for it, see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#459 but see faaborg's latest comment in bug #411293, where he thinks using frecency (and not date) is a bad idea. if we fix it, then we might have to do chunking to address bug #407429. I hate to dump this on dietrich , but I recommend including him in the conversation as he'll be the one to review the changes going forward.

Comment on attachment 299452 [details] [diff] [review] v1.1 r=sspitzer, thanks for the additional cleanup of mCurrentChunkOffset

(In reply to comment #13) > I hate to dump this on dietrich , but I recommend including him in the > conversation as he'll be the one to review the changes going forward. Should I r? dietrich and wait for that before landing?

> Should I r? dietrich and wait for that before landing? that would probably be best, thanks Edward. dietrich is now Mr. Frecency (he's also Mr. Places and Mr. Session Restore...he may soon unite the belts!). It would be best to get his r=+, so that at the very least he's aware of what's getting checked in.

Comment on attachment 299452 [details] [diff] [review] v1.1 thanks, r=me. thanks for doing first-review seth.

Comment on attachment 299452 [details] [diff] [review] v1.1 correct, clear, concise

Comment on attachment 299452 [details] [diff] [review] v1.1 a=beltzner

Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v <-- nsNavHistoryAutoComplete.cpp new revision: 1.33; previous revision: 1.32 done

*** VERIFIED Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv