I'm proposing to do this in 3 patches, unless reviewers ask me to combine them!

OK, so the tough thing here was that history stores titles in (endian-dependent) UTF-16, but everything else in UTF-8. (There's even code that was designed to convert titles to UTF-8, but I didn't want to resurrect that because it would break existing profiles.) Unfortunately it turns out to be pretty useful to be able to retrieve the URL in UTF-16, so I added a helper method for it. I toyed with the idea of returning a Substring but couldn't work out how to do that.

The code to filter existing rows wasn't working so I resolved it by making the global history service its own autocomplete result. This will however make it slightly less efficient to autocomplete in two windows at once.

so... this makes it impossible to have two concurrent autocomplete searches, no?

(In reply to comment #3) >so... this makes it impossible to have two concurrent autocomplete searches, no? History autocomplete is synchronous. Are you referring to the optimization of searching previous results?

ah, I didn't realize it was sync.

Comment on attachment 227215 [details] [diff] [review] Part 1a: support toolkit autocomplete datasources +nsGlobalHistory::AutoCompleteSortComparison(nsIMdbRow* row1, nsIMdbRow* row2, + void* closureVoid) wrong indentation in the second line here +nsGlobalHistory::GetSearchResult(PRUint16 *aSearchResult) +{ + NS_ENSURE_ARG_POINTER(aSearchResult); oh c'mon, you shouldn't have to check this parameter. only c++ can pass null here and if someone does that they deserve what they get :) same for some of the other functions here + NS_ENSURE_TRUE(aIndex >= 0 && aIndex < mResults.Count(), NS_ERROR_INVALID_ARG); I'd have used NS_ENSURE_ARG, but sure :) +nsGlobalHistory::RemoveValueAt(PRInt32 aIndex, PRBool aRemoveFromDb) +{ + NS_ENSURE_TRUE(aIndex >= 0 && aIndex < mResults.Count(), NS_ERROR_INVALID_ARG); + mResults.RemoveObjectAt(aIndex); + return NS_OK; should you really be ignoring aRemoveFromDb here? Hm... in StartSearch... why not use AutoCompleteEnumerator? this is either r+ or r- from me, depending on the answers :)

(In reply to comment #6) >(From update of attachment 227215 [details] [diff] [review]) >>+nsGlobalHistory::AutoCompleteSortComparison(nsIMdbRow* row1, nsIMdbRow* row2, >>+ void* closureVoid) >wrong indentation in the second line here Fixed. >>+nsGlobalHistory::GetSearchResult(PRUint16 *aSearchResult) >>+{ >>+ NS_ENSURE_ARG_POINTER(aSearchResult); >oh c'mon, you shouldn't have to check this parameter. only c++ can pass null >here and if someone does that they deserve what they get :) I did this for consistency with existing functions that do this. >>+ NS_ENSURE_TRUE(aIndex >= 0 && aIndex < mResults.Count(), NS_ERROR_INVALID_ARG); >I'd have used NS_ENSURE_ARG, but sure :) I was not aware of that usage, but I see there are other examples. Fixed. >>+nsGlobalHistory::RemoveValueAt(PRInt32 aIndex, PRBool aRemoveFromDb) >>+{ >>+ NS_ENSURE_TRUE(aIndex >= 0 && aIndex < mResults.Count(), NS_ERROR_INVALID_ARG); >>+ mResults.RemoveObjectAt(aIndex); >>+ return NS_OK; >should you really be ignoring aRemoveFromDb here? We don't use it (yet...) but I can fairly easily implement it. >Hm... in StartSearch... why not use AutoCompleteEnumerator? Because it enumerates xpfe autocomplete results, not MDB rows.

Comment on attachment 227215 [details] [diff] [review] Part 1a: support toolkit autocomplete datasources ok, then r=biesi. but please either implement aRemoveFromDb or file a bug about it :)

Comment on attachment 232376 [details] [diff] [review] Part 1b: support toolkit autocomplete datasources How about: NS_ENSURE_ARG_RANGE(aIndex, 0, mResults.Count() - 1); And |const nsAFlatCString&| for RemovePageInternal? Or is that archaic these days?

(In reply to comment #10) > And |const nsAFlatCString&| for RemovePageInternal? Or is that archaic these > days? It's archaic - all strings are flat now.

Notes: 1. I haven't checked in attachment 232376 [details] [diff] [review] so you will need to apply that if you want to really test this out. 2. The "file" autocomplete search is only useful on Linux where I added it for good measure but I can remove it again if you prefer. It allows you to type a full path name if you begin with a / but I only used it in the three places that understand absolute path names.

Substrings aren't, iirc, so we still have nsAC?String for that. Is there a plan to remove nsAFlatC?String, or is it still used for documentation purposes?

darin, see comment 13 ;)

(In reply to comment #13) >Substrings aren't, iirc, so we still have nsAC?String for that. My understanding was that we now use an automatic conversion operator so for instance my code in RebuildDocumentFromSource in nsHTMLEditor.cpp that does things like res = LoadHTML(body + aSourceString); now causes a copy where the old string classes used to efficiently pass a dependent concatenation.

This is the previous patch but I additionally removed the old-style autocomplete interfaces. Strangely enough this makes the nsModule.cpp changes disappear ;-)

Comment on attachment 232448 [details] [diff] [review] XUL changes >Index: extensions/inspector/resources/content/toolboxOverlay.xul >=================================================================== >@@ -49,7 +49,7 @@ > <hbox align="center" flex="1"> > <textbox id="tfURLBar" type="autocomplete" flex="1" > searchSessions="history" timeout="50" maxrows="6" >- observes="cmdGotoURL"> >+ autocompletesearch="history file" observes="cmdGotoURL"> > <image id="imgURLBarIcon"/> > > </textbox> Maybe worth adding a comment as to why searchSessions is being kept in this case.

Comment on attachment 232544 [details] [diff] [review] Complete patch r=biesi assuming you only moved and deleted functions since the patch I reviewed. interdiff is useless here.

Fix checked in and doesn't seem to have caused any bustage... yet ;-)

This busted Camino: http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1158587940.19370.gz

Neil: This bug here might have caused Bug 354025, can you take a look?