(Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be multiple existing bugs on this. Please make them block this bug, and we will de-dupe if they are indeed exactly the same. Thanks!) We stopped “speaking URL” when AwesomeBar came along, and it makes us seem slower when typing a URL. A large portion of people still "speak URLs" when navigating the internet, and we currently do a bad job at accommodating these people. If you enable "browser.urlbar.autoFill", you will get something that is very close to the behavior we want. However, it completes all the way to the end of a URL instead of respecting URL fragments, so if the only URL you have visited at cnn.com is: http://www.cnn.com/2010/TRAVEL/05/17/extreme.airfares/index.html and you type: "cnn[…]" …we currently complete the URL all the way, making it impossible to go to cnn.com without typing it manually. Since the full result will already be in the AwesomeBar results, there's no need for us to complete this all the way (since if you really want that result, you can use arrow down + enter or click it). What should happen: (The URL already in history in this example is: http://www.cnn.com/2010/TRAVEL/05/17/extreme.airfares/index.html) - Activate the URL bar - Type "cnn[…]" - We complete to the first URL fragment separator, ie. "cnn.com/" - The full URL to the existing article is always visible in the AwesomeBar pulldown. - If you arrow left (or click) and start typing the "2010" part of the URL, we complete to the next fragment separator, in this case: "cnn.com/2010/" - Whenever you hit enter, we complete the URL based on whatever prefix was already in the history (if we have one), ie. https or www gets added in. If there isn't any existing entry that matches, no such completion is done.

Yes, this is very annoying for URLs that you type a lot. I actually created a lot of keywords for this, so "cnn.com" would get a "cnn" keyword. However this does not work very nice, so I would be very happy if this papercut was fixed properly.

I agree that this is the right design; I withdraw previous arguments against this sort of feature given the design in comment 0. David or Marco: will this be OK for screen readers?

I'm pretty sure this is the right design as well, but we are planning on running a test pilot study on current beta users soon to get some details on how often users enter full URLs by hand (because the awesome bar is favoring a deep link). So for instance if the user hits "n" and they want nytimes.com, but the awesome bar produces "http://www.nytimes.com/2010/05/23/magazine/23FOB-medium-t.html" so they end up having to type "ytimes.com [enter]", we'll have that sequence of actions logged.

This bug has my interest, assuming I get the free time... I think this sort of autocomplete is implemented in C++ now, unlike in XBL in the XPFE days? http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp

Taking. Woohoo.

I have no idea what to do about tests, I've never written tests for the UI before. This is completely working from what I've tested and follows Limi's design. I'll likely have limited time to continue working on this patch since I'm not officially with Mozilla at the moment, though.

(In reply to comment #8) > Created attachment 476598 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=476598 > Patch Uh, note that the autocomplete controller is used by more than just the url bar. Have you tested that this doesn't break other things as well?

(In reply to comment #9) > Uh, note that the autocomplete controller is used by more than just the url > bar. Have you tested that this doesn't break other things as well? Ah, I forgot about the Open Location dialog, I'll test that soon, but what other places is this sort of autofill used in?

I believe Shawn was referring to other autocomplete users like the searchbox and not so much other history autocomplete.

(In reply to comment #11) > I believe Shawn was referring to other autocomplete users like the searchbox > and not so much other history autocomplete. But do any of them use the autofill? Via pref or anything? Looking at the existing code it does make provisions for "http://", so it does look like it's expecting URLs. If not, however, I think there is some extra code in that branch at the end I could add which will help avoid unnecessary use of the ">>" marking. It shouldn't break, because if the complete value is not a URL then it takes that branch of code at the end I just mentioned.

Drive-by comment from the guy who designed and implemented Chrome's inline autocomplete based on my prior interactions with Firefox' browser.urlbar.autofill. Inline autocomplete _must be_ synchronous. If it's not, where the user navigates is up to a race condition of "how fast you autofill" versus "how soon they press enter". This is frustrating enough to completely kill the feature. I highly doubt your infrastructure is set up to autocomplete synchronously; the entire address bar and history system for Chrome had to be purpose-built to make this happen. Also, inline autocompletion must be predictable, learnable, and fairly stable. The whole point is to train users to hit a few keys and then enter as an accelerator for common URLs. If your ranking algorithms change what those keys autocomplete too, your users will become extremely frustrated. In short, while this is incredibly useful, it is a giant minefield to get right. It took me probably nine man-months of effort to get most cases right in Chrome. IE9 is trying to do this and, from what I hear, getting it wrong; Safari has long tried to do this and gotten it very wrong.

Comment on attachment 476598 [details] [diff] [review] Patch The only other place autofill is used, according to MXR, is tags in the bookmark manager. That still seems to work fine. I have a new patch which I'll push to try server when it's back up.

Passes tryserver with flying colours.

note that vlad is pretty busy and likely won't be able to get to that review for a while. You might want to try a different toolkit peer, and probably one who is more familiar with the autocomplete code (I think I may have just singled gavin and myself out by saying that though): http://www.mozilla.org/projects/toolkit/review.html Also, we need to make sure that doesn't have a bad UX per pkasting's comment (comment 13). I know limi uses this in his day-to-day browsing, but it isn't currently "smarter". We should circulate try builds and make sure this works OK for people (I'm mostly concerned about slow hard drives and how this interacts given what comment 13 said).

Comment on attachment 476763 [details] [diff] [review] Patch 2 The try server builds are located here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mventnor@mozilla.com-7d71446f8292/ This patch doesn't change the nature of autocomplete, it only sometimes changes what gets filled in. Is autocomplete already asynchronous?

(In reply to comment #17) > This patch doesn't change the nature of autocomplete, it only sometimes changes > what gets filled in. Is autocomplete already asynchronous? For the location bar, yes.

If we want this feature for 4.0 I don't know how much behaviour we can change. If I'm reading the Chromium code right, you do two passes of searching, a synchronous pass on an in-memory database (ATTACHed to the history DB, copying only URLs that have been typed at least once, then DETACHed) and an asynchronous pass on the actual database backend.

(In reply to comment #17) > The try server builds are located here: Thanks! I'll run this over the next day with my full profile and see how it works out. Incredibly excited that we already have a patch for this!

Initial impressions are very positive, I'll keep running it for the next few days to make sure I get it exercised, though. The only thing that doesn't seem correct right now is that if I type a letter and the top entry doesn't start with that letter, we don't fill in anything. If the first match starting with Z is zedshaw.com as in the attached screenshot, we should fill that in, not leave it blank as we do now. The two ways to enter/select pages are independent (although it's fine for them to use the same ranking algorithm internally).

Comment on attachment 476763 [details] [diff] [review] Patch 2 Is there any way we can get it to trigger autocomplete for cases like I mentioned in comment 21, or is this hard to do with the existing code approach?

Is this going to miss the beta7 code freeze?

I'm hoping this will also fix bug 559878 (or at lest, make it easier to fix) - so it'd be great if that could be kept in mind when reviewing. On just a quick glance at the patch, it seems it might fix it.

Comment on attachment 476763 [details] [diff] [review] Patch 2 Good news everybody! This does indeed fix bug 559878 - tab-matches now auto-fill without the moz-action: prefix weirdness. Although it means hitting enter on a tab-match auto-fill navigates to that page, rather than switching to the tab - but I think that's correct in this case (limi should confirm that). And I'm not sure its possible to (sanely) fix it to be otherwise (both code-wise and interaction-wise). Will do a general feedback pass on the patch, to help with reviews. >+ if (StringBeginsWith(aFixedSpec, NS_LITERAL_STRING("moz-action:switchtab,"))) >+ aFixedSpec.Cut(0, 21); This is ok for now, but note that eventually there could be more actions than just "switchtab". Although other actions could potentially use data that isn't a URL, so that will be tricky. Best to burn that bridge when we come to it, I think. >+ if (NS_SUCCEEDED(ios->ExtractScheme(NS_ConvertUTF16toUTF8(aValue), scheme))) { >+ // We have a URI here. >+ // Remove common schemes and "www." so we can match more intelligently. I'm a bit worried about auto-filling autocomplete fields that can have any type of input here. AFAIK, there's nothing in the tree that would have a problem, but other consumers might. Though its just an extension of a limitation in the existing code. It might be nicer if the auto-filled value kept the original http:// (or whatever) prefix. Not sure what that would mean for a tab-match result though. Normally I'd suggest requiring opting-in to this behaviour with a flag added to the autocompletesearchparam attribute, but you can't really do that at the level of the autocomplete controller (because that's aimed more at the autocomplete provider, and they can use the param however they want). And I can't see an easy way of making it so the provider provides the auto-fill value, since you can have multiple providers.

Comment on attachment 476763 [details] [diff] [review] Patch 2 >+ mInput->SetTextValue(mSearchString + addString); This means it keeps the case (upper-case vs lower-case) of the characters of the input string, rather than using the case of the result's value. This will mess up URLs that are case-sensitive.

(In reply to comment #23) > Is this going to miss the beta7 code freeze? It's not a code freeze, it's a feature freeze. Since most of this is improving existing functionality and flipping a preference, it should be OK for later inclusion — at least as far as I understand it. When talking to Johnath/Beltzner, they are in favor of allowing this change as long as we ensure that it performs as expected. They obviously won't block the final release for it, but there is a good chance it can be taken as an opportunistic patch, especially since this is one of the largest "perceived performance" improvements we can make.

Yes, we'd let it through, but the bar will be high. As pkasting says, it'll need to perform with the async-like response of Chrome - the current race condition makes it really really twitchy, and I agree with him that's enough to kill the feature. Also, it will need to: - pass all tests, - no regressions on perf

Every change we make needs to pass all tests and not cause performance regressions :) Additional tests covering the auto-filling behavior specifically would be a big help in increasing confidence about the safety of the change. Changes to this code have a history of being problematic, so I think that we need to be careful about making "just a pref flip" characterizations.

Sorry, but I just cannot see us taking this for Firefox 4. These changes are just too scary to take at this point in the release cycle. I do, however, promise to get to this review once we get any blockers I can help out on cleared up so we can land it early in the Firefox next cycle to hammer out any regressions that might come up.

If this is not the correct bug for this information, please let me know. In addition to URLs starting with 'www' not working for inline autocomplete, it should be noted that 'https' URLs do not autocomplete unless you type the 'https' first. EXAMPLE: 1. Type 'm-a' and nothing is autocompleted inline (dropdown pops up with https://mail.google.com/) 2. Type 'h-t-t-p-s-:' and 'https://mail.google.com/' is autocompleted inline.

(In reply to comment #3) > I'm pretty sure this is the right design as well, but we are planning on > running a test pilot study on current beta users soon to get some details on > how often users enter full URLs by hand (because the awesome bar is favoring a > deep link). So for instance if the user hits "n" and they want nytimes.com, > but the awesome bar produces > "http://www.nytimes.com/2010/05/23/magazine/23FOB-medium-t.html" so they end up > having to type "ytimes.com [enter]", we'll have that sequence of actions > logged. Did a bug ever get filed about this test pilot study Alex? (In reply to comment #25) > Comment on attachment 476763 [details] [diff] [review] > Patch 2 > > Good news everybody! This does indeed fix bug 559878 - tab-matches now > auto-fill without the moz-action: prefix weirdness. Although it means hitting > enter on a tab-match auto-fill navigates to that page, rather than switching to > the tab - but I think that's correct in this case (limi should confirm that). > And I'm not sure its possible to (sanely) fix it to be otherwise (both > code-wise and interaction-wise). Need UX-input on this. (In reply to comment #28) > Yes, we'd let it through, but the bar will be high. As pkasting says, it'll > need to perform with the async-like response of Chrome - the current race > condition makes it really really twitchy, and I agree with him that's enough to > kill the feature. I want to clarify this; did you use this patch and see that it was twitchy, or is this a theoretical concern?

(holding off on review until comment 33 is answered by all three parties, but this review will happen soon)

(In reply to comment #33) > I want to clarify this; did you use this patch and see that it was twitchy, or > is this a theoretical concern? I don't think so; I was referring to how it was when one just flipped the pref, without this patch applied.

(In reply to comment #33) > (In reply to comment #3) > > I'm pretty sure this is the right design as well, but we are planning on > > running a test pilot study on current beta users soon to get some details on > > how often users enter full URLs by hand (because the awesome bar is favoring a > > deep link). So for instance if the user hits "n" and they want nytimes.com, > > but the awesome bar produces > > "http://www.nytimes.com/2010/05/23/magazine/23FOB-medium-t.html" so they end up > > having to type "ytimes.com [enter]", we'll have that sequence of actions > > logged. > Did a bug ever get filed about this test pilot study Alex? I don't think we need a study on this. Deep-links on autofill is bad, and the AwesomeBar search results will handle those cases better anyway. We should match to nearest separator when doing autofill. > (In reply to comment #25) > > Comment on attachment 476763 [details] [diff] [review] > > Patch 2 > > > > Good news everybody! This does indeed fix bug 559878 - tab-matches now > > auto-fill without the moz-action: prefix weirdness. Although it means hitting > > enter on a tab-match auto-fill navigates to that page, rather than switching to > > the tab - but I think that's correct in this case (limi should confirm that). > > And I'm not sure its possible to (sanely) fix it to be otherwise (both > > code-wise and interaction-wise). > Need UX-input on this. Yes, hitting enter on autofill should not switch to tab, but load the URL instead.

Comment on attachment 476763 [details] [diff] [review] Patch 2 (In reply to comment #12) > But do any of them use the autofill? Via pref or anything? Looking at the > existing code it does make provisions for "http://", so it does look like it's > expecting URLs. Correct; none of them do. > It shouldn't break, because if the complete value is not a URL then it takes > that branch of code at the end I just mentioned. Yeah, this looks to be OK. global-nit: please brace single line ifs per the style guide >+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp >+static void >+RemoveURIPrefixes(const nsString &aOrigSpec, >+ nsString& aFixedSpec) java-doc style comment here, please This method also does not belong in the nsIAutoCompleteController section of this file. Please add a new section for helper methods, and move it there. >+ aFixedSpec.Assign(aOrigSpec); Probably better to assign using a Substring at each place you cut below: https://developer.mozilla.org/en/XPCOM_string_guide#Substrings_.28string_fragments.29 >+ if (StringBeginsWith(aFixedSpec, NS_LITERAL_STRING("moz-action:switchtab,"))) >+ aFixedSpec.Cut(0, 21); Although you'll want to create a temporary string (const nsAString& with a Substring) here it looks like with this removed first and use it in the later checks. We certainly need to have a test for this case too. >+ nsresult rv; >+ nsCOMPtr<nsIIOService> ios = do_GetService(NS_IOSERVICE_CONTRACTID, &rv); This is going to end up getting expensive doing this do_GetService call every time we look at a result. Cache this service (might already be in mozilla/Services.h) >+ nsCAutoString scheme; >+ if (NS_SUCCEEDED(ios->ExtractScheme(NS_ConvertUTF16toUTF8(aValue), scheme))) { Doing this string conversion really sucks. We got rid of a bunch of string conversions in the location bar (high CPU usage), and this adds a bunch. We may want to implement ExtracScheme for wide strings so we don't have to do the conversion (it doesn't look terribly hard to do). jlebar's thought on this would be good (just cc'd him). >+ // We have a URI here. >+ // Remove common schemes and "www." so we can match more intelligently. >+ nsAutoString fixedSearchValue, fixedCompleteValue; >+ RemoveURIPrefixes(mSearchString, fixedSearchValue); >+ RemoveURIPrefixes(aValue, fixedCompleteValue); We really don't want to be allocating new strings here (even on the stack). We should have RemoveURIPrefixes return const nsAString& so we can just pass out the Substring output. >+ nsAutoString addString; >+ addString.Assign(Substring(fixedCompleteValue, fixedSearchValue.Length(), -1)); Avoid string allocation here too. >+ // We only want to autocomplete up to the next slash. This lets a user go to a toplevel >+ // domain, if a longer path in that domain is higher in the autocomplete. >+ // eg. if the user types "m" and "mozilla.org/credits" is the top hit, autocomplete >+ // only to "mozilla.org/" in case that's where they want to go. They're one keystroke >+ // away from "/credits", anyway. This isn't what comment 0 asks for though. Specifically: > - We complete to the first URL fragment separator, ie. "cnn.com/" However, there are other URL fragment separators. '.', '/', '?', and '#' are all ones that I think we should consider. This needs further clarification from limi, however. Additionally, we should pull this logic out into a helper since there will be multiple characters that we have to check for. >+ mInput->SetTextValue(mSearchString + addString); as noted in comment 26, this is not correct. We'll need tests for this too before it can land.

(In reply to comment #37) > >+ nsCAutoString scheme; > >+ if (NS_SUCCEEDED(ios->ExtractScheme(NS_ConvertUTF16toUTF8(aValue), scheme))) { > Doing this string conversion really sucks. We got rid of a bunch of string > conversions in the location bar (high CPU usage), and this adds a bunch. We > may want to implement ExtracScheme for wide strings so we don't have to do the > conversion (it doesn't look terribly hard to do). jlebar's thought on this > would be good (just cc'd him). Forgot to cc him first...

I won't have a chance to look at this properly for two weeks (finishing school), but how many conversions does this actually add? The code I messed with was being run over every entry in the Places db. Is this invoked for as many entries?

(In reply to comment #39) > The code I messed with was being run over every entry in the Places db. Is > this invoked for as many entries? Just every result, so not nearly as many. I forgot that was for every entry. We probably don't need to worry about the conversion at this point.

We'd like to see if this can land in time for the upcoming release. Michael, do you think you have time to address sdwilsh's review comments in the coming days?

Nice if that includes visited/unvisited local files like as has always been in Internet Explorer. That's the part I miss and I don't actually see local files mentioned here at all so just to be a little more explicit.

(In reply to comment #42) > Nice if that includes visited/unvisited local files like as has always been > in Internet Explorer. That's the part I miss and I don't actually see > local files mentioned here at all so just to be a little more explicit. File a follow-up bug for that. This bug should be tightly scoped and we should land the basic functionality first.

That's bug 77052 (and bug 97031).

OK, I've taken a completely new approach for this bug, which fixes limi's bug in comment 21 and even getting rid of the string conversions and service queries (thanks to assumptions we can now make!). I've hopefully addressed all review comments, with one exception: (In reply to comment #37) > This isn't what comment 0 asks for though. Specifically: > > - We complete to the first URL fragment separator, ie. "cnn.com/" > However, there are other URL fragment separators. '.', '/', '?', and '#' > are all ones that I think we should consider. This needs further > clarification from limi, however. Indeed, limi your words of wisdom are required. Also, tests are absent but I'm not sure what to do about those yet. If anyone with experience regarding places/autocomplete tests is watching, help is appreciated to speed things up.

(In reply to comment #37) >(From update of attachment 476763 [details] [diff] [review]) >>+ nsAutoString fixedSearchValue, fixedCompleteValue; >>+ RemoveURIPrefixes(mSearchString, fixedSearchValue); >>+ RemoveURIPrefixes(aValue, fixedCompleteValue); >We really don't want to be allocating new strings here (even on the stack). >We should have RemoveURIPrefixes return const nsAString& so we can just pass >out the Substring output. Another alternative is to use nsDependentString for these strings, and make RemoveURIPrefixes call Rebind to set them to the appropriate substring.

Comment on attachment 531855 [details] [diff] [review] Patch 3 >+ if (StringBeginsWith(Substring(aOrigSpec, amountToCut), NS_LITERAL_STRING("http://"))) >+ amountToCut += 7; >+ else if (StringBeginsWith(Substring(aOrigSpec, amountToCut), NS_LITERAL_STRING("https://"))) >+ amountToCut += 8; >+ else if (StringBeginsWith(Substring(aOrigSpec, amountToCut), NS_LITERAL_STRING("ftp://"))) >+ amountToCut += 6; >+ >+ if (StringBeginsWith(Substring(aOrigSpec, amountToCut), NS_LITERAL_STRING("www."))) >+ amountToCut += 4; FYI in case you're interested, XPFE's current algorithm only strips ftp:// from ftp host names, and only strips http:// from non-ftp host names. (We don't prefill https:// URLs any more.) And yes, I know a domain that uses both prefixes with both host names.

(In reply to comment #45) > (In reply to comment #37) > > This isn't what comment 0 asks for though. Specifically: > > > - We complete to the first URL fragment separator, ie. "cnn.com/" > > However, there are other URL fragment separators. '.', '/', '?', and '#' > > are all ones that I think we should consider. This needs further > > clarification from limi, however. > > Indeed, limi your words of wisdom are required. Should count as separators: / ? # Should not count as separator: . (reason: we do not want completion to stop at "index" for "index.html", etc.) Some sites depend on the values behind ?, but those sites are unlikely to be the ones where you type the manual URL anyway, as it's usually lots of query parameters involved instead of a human-readable URL.

OK, I've optimized and cleaned up the patch as much as I could think of, and this is ready to use. Try server builds will be here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mventnor@mozilla.com-081aafc7917d Only thing left is, of course, tests. But as I've said before, I would not be mad (in fact, the opposite) if someone were to come here providing their own tests before I got around to doing them.

Comment on attachment 532135 [details] [diff] [review] Patch 4 I want Marco to take a look at this too because he knows the front-end code far better than I here.

Comment on attachment 532135 [details] [diff] [review] Patch 4 Review of attachment 532135 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay on this. I had a number of large patches dumped into my queue. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +104,5 @@ > + * > + * @param aOrigSpec - The string to look for prefixes in. > + * @returns A substring, with any URL prefixes removed, that depends on the > + * same buffer as aOrigSpec (to save allocations). > + */ global-nit: format should be like https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#63 @@ +123,5 @@ > + else if (StringBeginsWith(result, NS_LITERAL_STRING("ftp://"))) > + result.Rebind(result, 6); > + > + if (StringBeginsWith(result, NS_LITERAL_STRING("www."))) > + result.Rebind(result, 4); global-nit: brace all ifs ::: toolkit/components/autocomplete/nsAutoCompleteController.h @@ +98,5 @@ > nsresult GetResultValueLabelAt(PRInt32 aIndex, PRBool aValueOnly, > PRBool aGetValue, nsAString & _retval); > + nsresult GetDefaultCompleteURLValue(nsIAutoCompleteResult *result, > + PRBool aPreserveCasing, > + nsAString &_retval); This needs javadoc style comments (yes, I realize that all the existing stuff doesn't have hit, but I'd rather improve code quality rather than let it stagnate). ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +739,5 @@ > result.setSearchResult(Ci.nsIAutoCompleteResult[resultCode]); > + > + // The magic number -2 tells the autocomplete controller to iterate over all > + // entries to find a suitable default, treating the values as URLs. > + result.setDefaultIndex(result.matchCount ? -2 : -1); I don't like using magic numbers here. I think we should adding an API to explicitly toggle this.

- Addressed review comments. - Figured out the tests now. So added some tests. - Removed some code which is no longer necessary in CompleteValue().

Comment on attachment 532890 [details] [diff] [review] Patch 5 Review of attachment 532890 [details] [diff] [review]: ----------------------------------------------------------------- Globally looks like an interesting experiment to put in Aurora and can be easily disabled, the code looks fine too and well contained. I don't like too much the idl setter though, see below. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1494,5 @@ > + // away from "/credits", anyway. > + PRInt32 separatorIndex = -1; > + FindIndexIfLower(appendValue, '/', separatorIndex); > + FindIndexIfLower(appendValue, '?', separatorIndex); > + FindIndexIfLower(appendValue, '#', separatorIndex); Is not this reimplementing .FindCharInSet("/?#") ? ::: toolkit/components/autocomplete/nsIAutoCompleteSimpleResult.idl @@ +77,5 @@ > /** > + * A writer for the readonly attribute 'isURLResult'. > + * Sets whether the values in this result are URLs. > + */ > + void setIsURLResult(in boolean aIsURLResult); This sounds like a hack. Having a readonly attribute and then a setter for it? Based on the only current usage, couldn't we somehow change setDefaultIndex to update mIsUrlResult as well? ::: toolkit/components/autocomplete/tests/unit/test_autofill.js @@ +33,5 @@ > + * and other provisions required by the GPL or the LGPL. If you do not delete > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ if you wish, in tests, you can use the short and nice pd license: http://www.mozilla.org/MPL/boilerplate-1.1/pd-c @@ +100,5 @@ > + if (iid.equals(Ci.nsISupports) || > + iid.equals(Ci.nsIAutoCompletePopup)) > + return this; > + > + throw Components.results.NS_ERROR_NO_INTERFACE; use XPCOMUtils.generateQI please @@ +110,5 @@ > + if (iid.equals(Ci.nsISupports) || > + iid.equals(Ci.nsIAutoCompleteInput)) > + return this; > + > + throw Components.results.NS_ERROR_NO_INTERFACE; XPCOMUtils.generateQI @@ +170,5 @@ > + if (iid.equals(Ci.nsISupports) || > + iid.equals(Ci.nsIAutoCompleteResult)) > + return this; > + > + throw Components.results.NS_ERROR_NO_INTERFACE; XPCOMUtils.generateQI @@ +213,5 @@ > + iid.equals(Ci.nsIFactory) || > + iid.equals(Ci.nsIAutoCompleteSearch)) > + return this; > + > + throw Components.results.NS_ERROR_NO_INTERFACE; XPCOMUtils.generateQI @@ +350,5 @@ > + > + default: > + do_test_finished(); > + return; > + } nit: rather than defining vars, tracking a test index, and distinguish through a switch, you could define an array of objects like: { searchValues: [...], inputString: "...", expectedAutocomplete: "...", expectedSelStart: ..., expectedSelEnd: ..., } and then just var test = tests.shift(); and check tests.length == 0 to finish Or use a test generator and yield. @@ +354,5 @@ > + } > + > + > + var search1 = new AutoCompleteSearch("test-autofill1", > + new AutoCompleteResult(searchValues, ["", ""], ["", ""])); why search1, if no search2 or search3 exist? @@ +360,5 @@ > + // Register search so AutoCompleteController can find them > + registerAutoCompleteSearch(search1); > + > + var controller = Components.classes["@mozilla.org/autocomplete/controller;1"]. > + getService(Components.interfaces.nsIAutoCompleteController); You have both Cc and Ci

(In reply to comment #53) > Comment on attachment 532890 [details] [diff] [review] [review] > Patch 5 > > Review of attachment 532890 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > Globally looks like an interesting experiment to put in Aurora and can be > easily disabled, the code looks fine too and well contained. > I don't like too much the idl setter though, see below. > > ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp > @@ +1494,5 @@ > > + // away from "/credits", anyway. > > + PRInt32 separatorIndex = -1; > > + FindIndexIfLower(appendValue, '/', separatorIndex); > > + FindIndexIfLower(appendValue, '?', separatorIndex); > > + FindIndexIfLower(appendValue, '#', separatorIndex); > > Is not this reimplementing .FindCharInSet("/?#") ? That sounds like a good idea. > ::: toolkit/components/autocomplete/nsIAutoCompleteSimpleResult.idl > @@ +77,5 @@ > > /** > > + * A writer for the readonly attribute 'isURLResult'. > > + * Sets whether the values in this result are URLs. > > + */ > > + void setIsURLResult(in boolean aIsURLResult); > > This sounds like a hack. Having a readonly attribute and then a setter for > it? > Based on the only current usage, couldn't we somehow change setDefaultIndex > to update mIsUrlResult as well? It's based on convention. Look at nsIAutoCompleteResult, with a lot of read-only properties. They get setter functions in nsIAutoCompleteSimpleResult. I assumed it was like that for a reason. Not sure what you mean by changing setDefaultIndex, sdwilsh doesn't like the idea of magic numbers, and URL bars are not the only user of autocomplete. > ::: toolkit/components/autocomplete/tests/unit/test_autofill.js > @@ +33,5 @@ > > + * and other provisions required by the GPL or the LGPL. If you do not delete > > + * the provisions above, a recipient may use your version of this file under > > + * the terms of any one of the MPL, the GPL or the LGPL. > > + * > > + * ***** END LICENSE BLOCK ***** */ > > if you wish, in tests, you can use the short and nice pd license: > http://www.mozilla.org/MPL/boilerplate-1.1/pd-c > > @@ +100,5 @@ > > + if (iid.equals(Ci.nsISupports) || > > + iid.equals(Ci.nsIAutoCompletePopup)) > > + return this; > > + > > + throw Components.results.NS_ERROR_NO_INTERFACE; > > use XPCOMUtils.generateQI please > > @@ +110,5 @@ > > + if (iid.equals(Ci.nsISupports) || > > + iid.equals(Ci.nsIAutoCompleteInput)) > > + return this; > > + > > + throw Components.results.NS_ERROR_NO_INTERFACE; > > XPCOMUtils.generateQI > > @@ +170,5 @@ > > + if (iid.equals(Ci.nsISupports) || > > + iid.equals(Ci.nsIAutoCompleteResult)) > > + return this; > > + > > + throw Components.results.NS_ERROR_NO_INTERFACE; > > XPCOMUtils.generateQI > > @@ +213,5 @@ > > + iid.equals(Ci.nsIFactory) || > > + iid.equals(Ci.nsIAutoCompleteSearch)) > > + return this; > > + > > + throw Components.results.NS_ERROR_NO_INTERFACE; > > XPCOMUtils.generateQI > > @@ +350,5 @@ > > + > > + default: > > + do_test_finished(); > > + return; > > + } > > nit: rather than defining vars, tracking a test index, and distinguish > through a switch, you could define an array of objects like: > { > searchValues: [...], > inputString: "...", > expectedAutocomplete: "...", > expectedSelStart: ..., > expectedSelEnd: ..., > } > and then just var test = tests.shift(); and check tests.length == 0 to finish > Or use a test generator and yield. > > @@ +354,5 @@ > > + } > > + > > + > > + var search1 = new AutoCompleteSearch("test-autofill1", > > + new AutoCompleteResult(searchValues, ["", ""], ["", ""])); > > why search1, if no search2 or search3 exist? > > @@ +360,5 @@ > > + // Register search so AutoCompleteController can find them > > + registerAutoCompleteSearch(search1); > > + > > + var controller = Components.classes["@mozilla.org/autocomplete/controller;1"]. > > + getService(Components.interfaces.nsIAutoCompleteController); > > You have both Cc and Ci Just so you know, I based this test code from another test file in the same folder. It took me a while to learn what I was doing! I'll try and fix these up tomorrow my time (hopefully sdwilsh gives his own review while I sleep).

(In reply to comment #54) > > Is not this reimplementing .FindCharInSet("/?#") ? > > That sounds like a good idea. Er, good idea to use that function I mean, not reimplement it...

(In reply to comment #54) > > ::: toolkit/components/autocomplete/nsIAutoCompleteSimpleResult.idl > It's based on convention. Look at nsIAutoCompleteResult, with a lot of > read-only properties. They get setter functions in > nsIAutoCompleteSimpleResult. I admit I didn't notice the existence of this common habit. Then it's probably fine, even if puzzling at first glance.

Fix Marco's comments.

Comment on attachment 533156 [details] [diff] [review] Patch 6 Review of attachment 533156 [details] [diff] [review]: ----------------------------------------------------------------- Are you testing both conditions of aPreserveCasing? ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1432,5 @@ > return NS_OK; > } > > nsresult > +nsAutoCompleteController::GetDefaultCompleteURLValue(nsIAutoCompleteResult *result, nit: aResult @@ +1435,5 @@ > nsresult > +nsAutoCompleteController::GetDefaultCompleteURLValue(nsIAutoCompleteResult *result, > + PRBool aPreserveCasing, > + nsAString &_retval) > +{ MOZ_ASSERT(aResult); @@ +1437,5 @@ > + PRBool aPreserveCasing, > + nsAString &_retval) > +{ > + PRUint32 rowCount; > + result->GetMatchCount(&rowCount); if you don't care about the result (void)result->GetMatchCount(...) @@ +1440,5 @@ > + PRUint32 rowCount; > + result->GetMatchCount(&rowCount); > + if (rowCount == 0) { > + return NS_ERROR_FAILURE; > + } What's the point of this? If you have zero, you fall out anyway with the same error because the for loop won't be executed. @@ +1442,5 @@ > + if (rowCount == 0) { > + return NS_ERROR_FAILURE; > + } > + > + const nsAString& fixedSearchTerm = RemoveURIPrefixes(mSearchString); Use the concrete class here: const nsDependentSubstring fixedSearchTerm = ... Note that using a reference like you are doing may actually be unsafe. Not 100% sure what is keeping that alive. @@ +1446,5 @@ > + const nsAString& fixedSearchTerm = RemoveURIPrefixes(mSearchString); > + for (PRUint32 i = 0; i < rowCount; ++i) { > + nsAutoString resultValue; > + result->GetValueAt(i, resultValue); > + const nsAString& fixedResult = RemoveURIPrefixes(resultValue); same here @@ +1454,5 @@ > + // Not a matching URL > + continue; > + } > + > + // Found a matching item! Figure out what needs to be assigned/appended global-nit: two spaces after punctuation @@ +1456,5 @@ > + } > + > + // Found a matching item! Figure out what needs to be assigned/appended > + if (aPreserveCasing) { > + const nsDependentString appendValue(Substring(fixedResult, fixedSearchTerm.Length())); This really needs a comment @@ +1464,5 @@ > + // eg. if the user types "m" and "mozilla.org/credits" is the top hit, autocomplete > + // only to "mozilla.org/" in case that's where they want to go. They're one keystroke > + // away from "/credits", anyway. > + PRInt32 separatorIndex = appendValue.FindCharInSet("/?#"); > + if (separatorIndex >= 0) { I would much rather see if (separatorIndex != kNotFound) { ::: toolkit/components/autocomplete/nsAutoCompleteController.h @@ +100,5 @@ > + /** > + * Searches for a suitable value to complete to, by comparing all values > + * as URLs, and completing only up to the next URL separator. > + * > + * @param result nit: aResult ::: toolkit/components/autocomplete/nsIAutoCompleteSimpleResult.idl @@ +77,5 @@ > /** > + * A writer for the readonly attribute 'isURLResult'. > + * Sets whether the values in this result are URLs. > + */ > + void setIsURLResult(in boolean aIsURLResult); I think that is starting down the path of making nsIAutoCompleteSimpleResult not so simple. Had you considered creating a new class that consumers could use that just has a different contract id? For example, instead of: let result = Cc["@mozilla.org/autocomplete/simple-result;1"]. createInstance(Ci.nsIAutoCompleteSimpleResult); have this: let result = Cc["@mozilla.org/autocomplete/url-result;1"]. createInstance(Ci.nsIAutoCompleteSimpleResult); Unclear if that's the right way to go though. You should talk to your sr (gavin) about the right way moving forward. ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +736,5 @@ > let resultCode = result.matchCount ? "RESULT_SUCCESS" : "RESULT_NOMATCH"; > if (aSearchOngoing) > resultCode += "_ONGOING"; > result.setSearchResult(Ci.nsIAutoCompleteResult[resultCode]); > + result.setIsURLResult(true); Doesn't this break the case when this pref is not set to true?

(In reply to comment #58) > @@ +1440,5 @@ > > + PRUint32 rowCount; > > + result->GetMatchCount(&rowCount); > > + if (rowCount == 0) { > > + return NS_ERROR_FAILURE; > > + } > > What's the point of this? If you have zero, you fall out anyway with the > same error because the for loop won't be executed. fwiw, I think he did to avoid the string assign that is between this code and the for loop.

(In reply to comment #59) > fwiw, I think he did to avoid the string assign that is between this code and > the for loop. It's a trivial operation since it's a dependent string, but if it's kept, this code needs a comment. I had to dig around to figure out why we would return early there.

(In reply to comment #58) > > + if (rowCount == 0) { > > + return NS_ERROR_FAILURE; > > + } > > + > > + const nsAString& fixedSearchTerm = RemoveURIPrefixes(mSearchString); > > Use the concrete class here: > const nsDependentSubstring fixedSearchTerm = ... > Note that using a reference like you are doing may actually be unsafe. Not > 100% sure what is keeping that alive. In 12.2 Temporary Objects: "A temporary holding the result of an initializer expression for a declarator that declares a reference persists until the end of the scope in which the reference declaration occurs. [...] In all these cases, the temporaries created during the evaluation of the expression initializing the reference, except the temporary to which the reference is bound, are destroyed at the end of the full-expression in which they are created and in the reverse order of the completion of their construction." I probably would have suggested "const nsDependentSubstring&" to make things clearer. But I assume copying nsDependentSubstring is cheap, so the copy Shawn suggests is probably tidier.

(In reply to comment #58) > ::: toolkit/components/autocomplete/nsIAutoCompleteSimpleResult.idl > @@ +77,5 @@ > > /** > > + * A writer for the readonly attribute 'isURLResult'. > > + * Sets whether the values in this result are URLs. > > + */ > > + void setIsURLResult(in boolean aIsURLResult); > > I think that is starting down the path of making nsIAutoCompleteSimpleResult > not so simple. Had you considered creating a new class that consumers could > use that just has a different contract id? For example, instead of: > let result = Cc["@mozilla.org/autocomplete/simple-result;1"]. > createInstance(Ci.nsIAutoCompleteSimpleResult); > have this: > let result = Cc["@mozilla.org/autocomplete/url-result;1"]. > createInstance(Ci.nsIAutoCompleteSimpleResult); > > Unclear if that's the right way to go though. You should talk to your sr > (gavin) about the right way moving forward. You suggested adding a new API in a previous review, and I think that is much simpler than adding a whole new result subclass, considering it is one new function. > ::: toolkit/components/places/nsPlacesAutoComplete.js > @@ +736,5 @@ > > let resultCode = result.matchCount ? "RESULT_SUCCESS" : "RESULT_NOMATCH"; > > if (aSearchOngoing) > > resultCode += "_ONGOING"; > > result.setSearchResult(Ci.nsIAutoCompleteResult[resultCode]); > > + result.setIsURLResult(true); > > Doesn't this break the case when this pref is not set to true? No, there's a separate boolean set based on the pref.

Comment on attachment 533516 [details] [diff] [review] Patch 7 >diff --git a/toolkit/components/autocomplete/nsIAutoCompleteResult.idl b/toolkit/components/autocomplete/nsIAutoCompleteResult.idl > /** >+ * Whether the values in this result are URLs. This will cause >+ * slightly different behaviour where defaultIndex is intelligently chosen >+ * for you, and searching is fine-tuned for URLs. You should probably mention explicitly that this attribute being set results in defaultIndex being ignored. Adding this here is fine - nsIAutoCompleteSimpleResult is kind of a bad name, it's probably more accurately named "nsIMutableAutoCompleteResult" or something. There are a bunch of JS nsIAutocompleteResult implementations that you're not updating. We should probably get a bug on file to make as many of them as possible use nsFormAutoCompleteResult.jsm.

This review should happen on Monday.

Comment on attachment 533516 [details] [diff] [review] Patch 7 Review of attachment 533516 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +104,5 @@ > + * comparisons. > + * > + * @param aOrigSpec > + * The string to look for prefixes in. > + * global-nit: no newline between these @@ +1451,5 @@ > + const nsDependentSubstring fixedSearchTerm = RemoveURIPrefixes(mSearchString); > + for (PRUint32 i = 0; i < rowCount; ++i) { > + nsAutoString resultValue; > + aResult->GetValueAt(i, resultValue); > + const nsDependentSubstring fixedResult = RemoveURIPrefixes(resultValue); Go ahead and use const nsDependentSubString& here (and elsewhere in the method) since comment 61 proves that it is safe. @@ +1461,5 @@ > + } > + > + // Found a matching item! Figure out what needs to be assigned/appended. > + if (aPreserveCasing) { > + // Use nsDependentString here so we have access to FindCharInSet nit: end sentences with punctuation please. @@ +1472,5 @@ > + // autocomplete only to "mozilla.org/" in case that's where they want to go. > + // They're one keystroke away from "/credits", anyway. > + PRInt32 separatorIndex = appendValue.FindCharInSet("/?#"); > + if (separatorIndex != kNotFound) { > + separatorIndex++; // Add 1 so we include the separator I wonder if we really always want to add the separator. It makes sense for /, but I'm less sure about ? or # ::: toolkit/components/autocomplete/nsAutoCompleteController.h @@ +107,5 @@ > + * Preserve the casing of what the user typed in. > + * @param [out] _retval > + * The value to complete to. > + * > + * @return A result, NS_OK if there is a value to complete to. Not usually worthwhile for nsresult return values. It's just used to propagate exceptions. ::: toolkit/components/autocomplete/nsAutoCompleteSimpleResult.cpp @@ +110,5 @@ > +nsAutoCompleteSimpleResult::GetIsURLResult(PRBool *aIsURLResult) > +{ > + *aIsURLResult = mIsURLResult; > + return NS_OK; > +} nit: need a newline after this ::: toolkit/components/autocomplete/nsAutoCompleteSimpleResult.h @@ +77,5 @@ > nsString mErrorDescription; > PRInt32 mDefaultIndex; > PRUint32 mSearchResult; > > + PRPackedBool mIsURLResult; I'd prefer it if you used bool ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +736,5 @@ > let resultCode = result.matchCount ? "RESULT_SUCCESS" : "RESULT_NOMATCH"; > if (aSearchOngoing) > resultCode += "_ONGOING"; > result.setSearchResult(Ci.nsIAutoCompleteResult[resultCode]); > + result.setIsURLResult(true); You should actually do this in startSearch. No reason to toggle this every time we notify (this._result is created in startSearch).

(In reply to comment #65) > I wonder if we really always want to add the separator. It makes sense for > /, but I'm less sure about ? or # ? or # should not be added automatically since they are meaningless on their own, whereas / has meaning. But OK to handle this in a follow-up, I think (they aren't actively bad, just unexpected).

(In reply to comment #65) > ::: toolkit/components/autocomplete/nsAutoCompleteController.h > @@ +107,5 @@ > > + * Preserve the casing of what the user typed in. > > + * @param [out] _retval > > + * The value to complete to. > > + * > > + * @return A result, NS_OK if there is a value to complete to. > > Not usually worthwhile for nsresult return values. It's just used to > propagate exceptions. We do need it in this case, though. Caller functions use the return value to determine whether to try to autocomplete, and always returning NS_OK will result in bugs. > ::: toolkit/components/autocomplete/nsAutoCompleteSimpleResult.cpp > @@ +110,5 @@ > > +nsAutoCompleteSimpleResult::GetIsURLResult(PRBool *aIsURLResult) > > +{ > > + *aIsURLResult = mIsURLResult; > > + return NS_OK; > > +} > > nit: need a newline after this OK, but other get/set functions also don't have a newline between them. > ::: toolkit/components/autocomplete/nsAutoCompleteSimpleResult.h > @@ +77,5 @@ > > nsString mErrorDescription; > > PRInt32 mDefaultIndex; > > PRUint32 mSearchResult; > > > > + PRPackedBool mIsURLResult; > > I'd prefer it if you used bool OK, but my understanding was PRPackedBool was a truly 1-bit bool, and it can save memory when used as a member.

Fixed comments.

(In reply to comment #67) > OK, but my understanding was PRPackedBool was a truly 1-bit bool, and it can > save memory when used as a member. AFAIK, that only applies if you're using many PRPackedBool members, or within structs where alignment allows.

(In reply to comment #69) > (In reply to comment #67) > > OK, but my understanding was PRPackedBool was a truly 1-bit bool, and it can > > save memory when used as a member. > > AFAIK, that only applies if you're using many PRPackedBool members, or > within structs where alignment allows. I don't think PRPackedBool is ever a 1-bit bool. From prtypes.h: /************************************************************************ ** TYPES: PRPackedBool ** DESCRIPTION: ** Use PRPackedBool within structs where bitfields are not desirable ** but minimum and consistant overhead matters. ************************************************************************/ typedef PRUint8 PRPackedBool; That said, Blair is right that PRPackedBool wouldn't save any space in this case, since it's squeezed between two word-aligned members.

The strategy for disabling this if we have to on aurora is to change the value of the preference browser.urlbar.autoFill back to false.

Hi.This feature landed in today's nightly and the url autocomplete is now not working as it should.For example when i type in the urlbar "arewe"(for arewefastyet.com)it autocompletes it like this:"arewe >> http://arewefastyet.com/" and when i hit enter instead of loading http://arewefastyet.com/ it makes a google search with "arewe >> http://arewefastyet.com/" as a search term.As it is now its like i have no autocomplete at all,since i still have to click the site from the dropdown list. Is it supposed to work like that or i am doing something wrong?

I now noticed that even if i type the full url and i hit enter i again make a google search with the "arewe >> http://arewefastyet.com/" as a serch term.I'm forced to disable this because i can't use the browser with it enabled.

I'm sorry this was caused by the Omnibar addon.When i disable the addon it works like before.

Can we figure out what the Omnibar addon is doing, and add it to the add-on compatibility scanner criteria for Fx6?

Nice feature! Thanks. But the Inline-Complete-feature shouldn't flicker while typing. Please compare the behavior with Chrome or Camino 2.1a1 (with the enabled "browser.urlbar.autofill" option).

Please add edge cases and issues found in bug 659437, so we can decide which ones need to be addressed. :)

Anyway this bug resolves following bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=649004 https://bugzilla.mozilla.org/show_bug.cgi?id=408133 https://bugzilla.mozilla.org/show_bug.cgi?id=333722 https://bugzilla.mozilla.org/show_bug.cgi?id=410837 They should be closed.

(In reply to comment #79) > They should be closed. Not quite. I believe this bug should be backed out, mainly because in order to fix bug 659445 and make this synchronous, I have to take a completely new approach within code. Plus, as it stands, this code is less shippable than I thought. sdwilsh, can I back this out please?

Verified Fixed after following the steps from description of the bug on Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110525 Firefox/6.0a2

(In reply to comment #77) > Nice feature! Thanks. But the Inline-Complete-feature shouldn't flicker > while typing. Please compare the behavior with Chrome or Camino 2.1a1 (with > the enabled "browser.urlbar.autofill" option). Agreed; for the record, I reported this as bug #648912 a month or so ago.

(In reply to comment #80) > sdwilsh, can I back this out please? I think it's fine to leave it on trunk for now, but as it currently stands it will need to be turned off on aurora I think. This is what our release channels are for :)

Can't we just pref this off everywhere? If we know this isn't good enough for Aurora, what's the benefit of leaving it on for nightly users?

(In reply to comment #85) > Can't we just pref this off everywhere? If we know this isn't good enough for > Aurora, what's the benefit of leaving it on for nightly users? This has been turned on for less than a week. I would prefer to see it stay on for a bit longer to see if any other issues shake out.

No, the reason why I want to back this out is a) There are issues that I've caused that occur even with the pref turned off, and b) I have an idea on how to make this synchronous, but it will take a new approach and won't build on this code at all.

FWIW, my proposal is to add a new search name (in addition to ";search=history" we have now) and make it search an in-memory DB of URLs. How we determine which URLs should be copied into memory is still up in the air and needs discussion in bug 659445.

Adding another search engine to the autocompletesearch attribute has issues with duplicate entries. I don't believe the autocomplete controller will assume same urls with potentially same "label" and "comment" are duplicates.

I will also add code that makes the results not added to the popup.

Backing this out (on trunk and aurora) seems like the best way forward, IMO. I don't think there's much value in keeping it on the trunk.

I've backed this out so I can start work on an approach that works.

Michael, can you attach a back-out patch to this bug and request Aurora approval please? Thanks.

(In reply to comment #93) > Michael, can you attach a back-out patch to this bug and request Aurora > approval please? Thanks. It's already been fixed by 659945.

(In reply to comment #94) > (In reply to comment #93) > > Michael, can you attach a back-out patch to this bug and request Aurora > > approval please? Thanks. > > It's already been fixed by 659945. Excellent. Thanks, Michael. Sorry if I confused anyone here. No need to track this for 6 since we've got the disable in in bug 659945.

(In reply to comment #90) > I will also add code that makes the results not added to the popup. Can you do that in a different bug please?

I'm going to add patches here that implement limi's new spec. From bug 659445 comment 8: > 1. We keep the top-level domains in an in-memory database (which should be a > much smaller set of data than all URLs), and complete using this for the > initial completion > > 2. Once we reach the fragment completion (ie. the rest of the URL after the > domain), we can either: > a) decide to use the existing approach, as it doesn't need to be as fast

This patch has three parts, however all three depend on the patches in bug 660592 and 660156. We need an SQL function so we can execute one SQL query to fill our domain table.

This will fill a temporary table at startup (on a separate thread of course), and autocomplete all domains synchronously.

After you type in a domain, this will autocomplete subdirectories using the same autocomplete algorithm as the awesomebar.

I forgot to add, part 3 may fix some of the review comments you might have for part 2.

(In reply to comment #100) > This will fill a temporary table at startup (on a separate thread of course), > and autocomplete all domains synchronously. Please don't do this at startup; we already do enough disk I/O at startup (much of it blocking the main thread) and this will compete with it. We should do it lazily, or shortly after startup.

(In reply to comment #103) > (In reply to comment #100) > > This will fill a temporary table at startup (on a separate thread of course), > > and autocomplete all domains synchronously. > Please don't do this at startup; we already do enough disk I/O at startup > (much of it blocking the main thread) and this will compete with it. We > should do it lazily, or shortly after startup. We can't do it lazily since the search must be synchronous and this operation to fill the data table takes time. The call is made under InitAdditionalDBItems(). Can you or mak think of a better place to put this operation?

The addon Speak Words , by Mozilla Labs , does a similar thin efficiently . It creates an array of the typed urls on startup. I have been using this addon for about 5 months now , and it merely delays the startup by 100ms at max. https://github.com/mozilla/prospector/tree/master/speakWords https://addons.mozilla.org/en-US/firefox/addon/242706/

How about loading it asynchronously when the address bar gains focus, and hoping the load has finished by the time I type a letter?

I've found the perfect place to put this initialisation: in delayedStartup() in browser.js. This is right after the browser window is loaded, so it shouldn't affect startup time (or at least perceived startup time). I've also folded parts 2 and 3 into one patch since they're basically for the same thing.

Any try build to test this patch ?

Comment on attachment 545138 [details] [diff] [review] Part 2 Patch 2 >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -1690,16 +1690,26 @@ function delayedStartup(isLoadingBlank, > if (scratchpadEnabled) { > document.getElementById("menu_scratchpad").hidden = false; > document.getElementById("Tools:Scratchpad").removeAttribute("disabled"); > #ifdef MENUBAR_CAN_AUTOHIDE > document.getElementById("appmenu_scratchpad").hidden = false; > #endif > } > >+ // Create the temporary table that stores all hostnames in Places. >+ // This will be used for inline autocomplete. >+ try { >+ Cc["@mozilla.org/autocomplete/search;1?name=urlinline"] >+ .getService(Ci.mozIURLInlineComplete) >+ .initDomainTable(); >+ } catch (ex) { >+ dump("URL domain table could not be initialized: " + ex + "\n"); >+ } This should be Cu.reportError (without \n) instead of dump

(In reply to comment #107) > I've found the perfect place to put this initialisation: in delayedStartup() in > browser.js. This is right after the browser window is loaded, so it shouldn't > affect startup time (or at least perceived startup time). Last I checked, we do a whole bunch of disk I/O stuff in delayedStartup ):

(In reply to comment #109) > This should be Cu.reportError (without \n) instead of dump Fixed in local patch. (In reply to comment #110) > Last I checked, we do a whole bunch of disk I/O stuff in delayedStartup ): I looked through delayedStartup again. With the possible exception of sessionStore, I couldn't find an example of disk I/O functions. A lot of stuff doesn't even run in delayedStartup, they have further timeouts set there. The new code is towards the end of the function anyway so any synchronous stuff would be done by then. I think delayedStartup remains the best place by far.

I've checked in all relevant patches into the ux branch for feedback. http://hg.mozilla.org/projects/ux/rev/5880f6da5c30 http://hg.mozilla.org/projects/ux/rev/9fafc59c9c6b http://hg.mozilla.org/projects/ux/rev/1816787bd415 http://hg.mozilla.org/projects/ux/rev/ba9167ccfd9c

Comment on attachment 544689 [details] [diff] [review] Part 1 - Add SQL function to get domain from URL Review of attachment 544689 [details] [diff] [review]: ----------------------------------------------------------------- The test may be better, some renaming to code would be welcome. ::: toolkit/components/places/SQLFunctions.cpp @@ +688,5 @@ > + nsCOMPtr<GetURLHostFunction> function = new GetURLHostFunction(); > + function->mParser = do_GetService(NS_STDURLPARSER_CONTRACTID, &rv); > + if (NS_FAILED(rv) || !function->mParser) { > + return NS_ERROR_FAILURE; > + } Could you avoid rv and just NS_ENSURE_STATE(function->mParser)? then just declare rv later at CreateFunction Otherwise, add a Init() method to the object and make mParser private, return rv from Init() and check it here. @@ +694,5 @@ > + rv = aDBConn->CreateFunction( > + NS_LITERAL_CSTRING("hostname"), 1, function > + ); > + > + return rv; please ns_ensure_success and return NS_OK per module consistency @@ +711,5 @@ > + nsIVariant **_result) > + { > + nsCAutoString url; > + (void)aArguments->GetUTF8String(0, url); > + const char* urlStr = url.get(); may we rather use SubString where needed and .get() ? I feel better using our string api and it should look cleaner being near usage @@ +713,5 @@ > + nsCAutoString url; > + (void)aArguments->GetUTF8String(0, url); > + const char* urlStr = url.get(); > + > + nsresult rv; declare at first use @@ +743,5 @@ > + &unusedInt); // port > + NS_ENSURE_SUCCESS(rv, rv); > + > + PRUint32 possibleSlashPos = authPos + hostPos + hostLen; > + if (hostLen != -1 && possibleSlashPos < url.Length() && When may hostLen be -1 if authLen is > 0? and do we care to return the result in such a case rather than just returning NULL as above? @@ +747,5 @@ > + if (hostLen != -1 && possibleSlashPos < url.Length() && > + url[possibleSlashPos] == '/') { > + // Make sure we include a trailing slash. > + hostLen++; > + } not sure why do you want this hostname method to return a trailing slash, you are reducing its functionality this way. I think it should return what its name says, and in SQL you can use the string concatenation operator "SELECT HOSTNAME(url) || '/'" would also simplify this checking code. @@ +749,5 @@ > + // Make sure we include a trailing slash. > + hostLen++; > + } > + > + nsDependentCSubstring host = Substring(url, authPos + hostPos, hostLen); so we may reach this point with hostLen = -1? @@ +751,5 @@ > + } > + > + nsDependentCSubstring host = Substring(url, authPos + hostPos, hostLen); > + if (StringBeginsWith(host, NS_LITERAL_CSTRING("www."))) { > + // Remove the www. add some reasoning to the comment please, this is not adding anything to the code itself, it's clear what it is doing. ::: toolkit/components/places/SQLFunctions.h @@ +246,5 @@ > > + > + > +//////////////////////////////////////////////////////////////////////////////// > +//// Get URL Host Function since the function is called HOSTNAME() I'd rename this to HostnameFunction, it's more consistent with what we did for the other functions @@ +249,5 @@ > +//////////////////////////////////////////////////////////////////////////////// > +//// Get URL Host Function > + > +/** > + * SQL function to get a hostname out of a URL. you should specify whatever the function does, for example it's stripping out www but you don't say that. ::: toolkit/components/places/tests/queries/test_hostname.js @@ +15,5 @@ > + hostname: "moz.port.com" }, > + > + { uri: "http://www.mozilla.org/i/love/directories", > + hostname: "mozilla.org/" }, > +]; may you also test with some idn names? does the url parser correctly handle those? and also add tests for other protocols (ftp and https) and for urls with full auth. and for urls without a auth (file) @@ +18,5 @@ > + hostname: "mozilla.org/" }, > +]; > + > + > +var conn = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection; no need for this, just use DBConn().whatever where you need it, it's in the Places test harness. @@ +26,5 @@ > + try { > + stmt.params.uri = uri; > + stmt.executeStep(); > + return stmt.row["host"]; > + } this is testing synchronous statements, you should also test an asynchronous statement

(In reply to comment #112) > I've checked in all relevant patches into the ux branch for feedback. > > http://hg.mozilla.org/projects/ux/rev/5880f6da5c30 > http://hg.mozilla.org/projects/ux/rev/9fafc59c9c6b > http://hg.mozilla.org/projects/ux/rev/1816787bd415 > http://hg.mozilla.org/projects/ux/rev/ba9167ccfd9c When landing on the UX branch, please follow the tree rules: https://wiki.mozilla.org/UX_Branch. You should list these changesets on the ux-branch etherpad.

Comment on attachment 545138 [details] [diff] [review] Part 2 Patch 2 Review of attachment 545138 [details] [diff] [review]: ----------------------------------------------------------------- So, I think I don't like some parts of the global approach here. Iterating through all places entries on startup will hurt, badly. We are talking about iterating more than one hundred thousands entries when the system is inited, and pass each one to the url parser. This blocks any other autocomplete work since everything is serialized, and will most likely move most of the database to the memory cache inflating it to the maximum as soon as you open the browser, this value won't decrease later. I think this is one case where we need a compromise rather than perfection, we should fetch in chunks, starting from the top 100 frecent entries, and we can eventually do a single larger chunk on idle. this means it will work most of the times but perfection in this case has a too high cost. We may even limit this to a limited amount of top frecent entries (some thousands?). Rather than polluting the addvisit methods, you may want to make your object observe history. This plus initing the table on service init should avoid the new interface and contain changes. To get the hostname you may even evaluate doing something cheaper than using the url parser, we already have a rev_host column, if you remove the last char and invert the string you get the host, just matter of removing www. at that point. ::: browser/app/profile/firefox.js @@ +273,5 @@ > pref("browser.urlbar.maxRichResults", 12); > // The amount of time (ms) to wait after the user has stopped typing > // before starting to perform autocomplete. 50 is the default set in > // autocomplete.xml. > +pref("browser.urlbar.delay", 0); is not this going to give us perf problems, like while I'm typing it may start spawning a bunch of async queries, when that's not really needed or wanted. It's possible we have to special case this so that sync queries use a smaller delay than async ones, but as it is it's not looking like a change we may take. ::: browser/base/content/browser.js @@ +1699,5 @@ > + // This will be used for inline autocomplete. > + try { > + Cc["@mozilla.org/autocomplete/search;1?name=urlinline"] > + .getService(Ci.mozIURLInlineComplete) > + .initDomainTable(); couldn't we rather init the table when the autocomplete service inits? this would remove one api need ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1100,5 @@ > + if (timeout == 0) { > + // The consumer wants to execute the search synchronously > + StartSearch(); > + return NS_OK; > + } but we don't want to start the async query immediately while the user is typing ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +1183,5 @@ > + "/* do not warn (bug 566489) */" > + + "SELECT hostname FROM moz_hostnames_temp " > + + "WHERE hostname LIKE :search_string ESCAPE '/'" > + + "ORDER BY frecency DESC " > + + "LIMIT 1" this query is going through each single entry in the table, executing a like on it, then it's going through them to order by frecency, and finally taking just one... for each char I type in the locationbar. Am I wrong thinking we are only matching the initial part of the hostname? then this should rather be hostname BETWEEN :search_string AND :search_string || X'FFFF' (provided we lowercase both). If you add an index on hostname this will be much more efficient than you currently have. @@ +1333,5 @@ > + this._db.executeSimpleSQL( > + "CREATE TEMP TABLE moz_hostnames_temp (" > + + " hostname TEXT PRIMARY KEY NOT NULL" > + + ", frecency INTEGER" > + + ")" give the table a real integer primary key id, you may think you are saving space but you are not really, since that exists regardless how you define your table hostname should be declared UNIQUE NOT NULL we actually don't need the real frecency value here, since you insert hostnames ordered by frecency, the lower the frecency the higher will be id, we can get advantage of the implicit index on id then. @@ +1344,5 @@ > + + "SELECT HOSTNAME(h.url) AS hostname, h.frecency " > + + "FROM moz_places h " > + + "WHERE h.hidden = 0 AND h.frecency <> 0 " > + + "ORDER BY h.frecency DESC" > + ); this should take advantage of the rev_host column. it already contains the hostname (inverted and with an added trailing period), so we may select top frecency distinct rev_host... we may even change HOSTNAME() to a INVERT_REVHOST function, may be cheaper. ::: toolkit/content/widgets/autocomplete.xml @@ +165,5 @@ > onget="return this.getAttribute('showimagecolumn') == 'true';"/> > > <property name="timeout" > onset="this.setAttribute('timeout', val); return val;" > + onget="var t = parseInt(this.getAttribute('timeout')); return isNaN(t) ? 50 : t;"/> what is this change supposed to do? NaN || 50 is 50, so the old code was fine

well I was wrong on the order the query executes things (it orders, then starts comparing and stops at first, in the worst case still iterating everything), but it's still largely unefficient.

there is another privacy hit, if I "forget about this page" or site, and there are no more pages for that hostname, it will keep appearing even if I wanted to get rid of it. So... we may add a count column to each hostname entry, that is number of times it appears in moz_places and remove it when it reaches 0, a temp trigger may keep these in sync easily.

(In reply to comment #115) > ::: browser/app/profile/firefox.js > @@ +273,5 @@ > > pref("browser.urlbar.maxRichResults", 12); > > // The amount of time (ms) to wait after the user has stopped typing > > // before starting to perform autocomplete. 50 is the default set in > > // autocomplete.xml. > > +pref("browser.urlbar.delay", 0); > > is not this going to give us perf problems, like while I'm typing it may > start spawning a bunch of async queries, when that's not really needed or > wanted. > It's possible we have to special case this so that sync queries use a > smaller delay than async ones, but as it is it's not looking like a change > we may take. What is the purpose of the delay anyway? It is important that, at least for the domain, this is all synchronous or else you'll have problems with fast typers. Why must the async query get a 50ms delay? > If you add an index > on hostname this will be much more efficient than you currently have. Can we create temp indexes? If the table resides in memory will the index reside in memory as well? If not, and you still want this, I'll have to stop declaring the DB connection read only, I did it for safety reasons. > ::: toolkit/content/widgets/autocomplete.xml > @@ +165,5 @@ > > onget="return this.getAttribute('showimagecolumn') == 'true';"/> > > > > <property name="timeout" > > onset="this.setAttribute('timeout', val); return val;" > > + onget="var t = parseInt(this.getAttribute('timeout')); return isNaN(t) ? 50 : t;"/> > > what is this change supposed to do? NaN || 50 is 50, so the old code was fine Then you can't declare the timeout as 0, otherwise you get 0 || 50. Also, regarding your suggestion of triggers, I tried that already but the triggers never occurred. I then assumed that triggers didn't work across connections, since plenty of other things in SQLite don't work across connections such as temp table access.

(In reply to comment #118) > What is the purpose of the delay anyway? It is important that, at least for > the domain, this is all synchronous or else you'll have problems with fast > typers. Why must the async query get a 50ms delay? The autocomplete queries are expensive, the delay is most likely trying to reduce the hit ratio firing them when there is a typing pause. > > If you add an index > > on hostname this will be much more efficient than you currently have. > > Can we create temp indexes? If the table resides in memory will the index > reside in memory as well? If not, and you still want this, I'll have to stop > declaring the DB connection read only, I did it for safety reasons. yes, but actually if you use the id primary key and declare hostname unique, you already have 2 automatic indexes! > > what is this change supposed to do? NaN || 50 is 50, so the old code was fine > > Then you can't declare the timeout as 0, otherwise you get 0 || 50. I see, so I don't get why the default is 50 and not 0 if that's an acceptable value. this is just a fallback for broken values. > Also, regarding your suggestion of triggers, I tried that already but the > triggers never occurred. I then assumed that triggers didn't work across > connections, since plenty of other things in SQLite don't work across > connections such as temp table access. triggers don't work across connections, but here you can use a single connection, you mean if you attach a temp trigger to moz_places in this connection it doesn't fire?

(In reply to comment #119) > (In reply to comment #118) > > > If you add an index > > > on hostname this will be much more efficient than you currently have. > > > > Can we create temp indexes? If the table resides in memory will the index > > reside in memory as well? If not, and you still want this, I'll have to stop > > declaring the DB connection read only, I did it for safety reasons. > > yes, but actually if you use the id primary key and declare hostname unique, > you already have 2 automatic indexes! Oh, so indexes are automatically done for unique keys? > > > what is this change supposed to do? NaN || 50 is 50, so the old code was fine > > > > Then you can't declare the timeout as 0, otherwise you get 0 || 50. > > I see, so I don't get why the default is 50 and not 0 if that's an > acceptable value. this is just a fallback for broken values. The default is 50 in other autocomplete users too, like forms. But I need this to be 0 for the sync query, for it to be a sync query. That said, I can probably enforce the delay on the search handler's side for the existing async query by using setTimeout 50. > > Also, regarding your suggestion of triggers, I tried that already but the > > triggers never occurred. I then assumed that triggers didn't work across > > connections, since plenty of other things in SQLite don't work across > > connections such as temp table access. > > triggers don't work across connections, but here you can use a single > connection, you mean if you attach a temp trigger to moz_places in this > connection it doesn't fire? I clone the connection, just like it is done in places autocomplete. So triggers won't fire on our connection when moz_places is modified. I can't use the actual database connection because that sometimes gets tied up doing large update queries.

(In reply to comment #120) > Oh, so indexes are automatically done for unique keys? yes, and temp indices and writes to temp tables should not be a problem even on readonly connections iirc. I don't know about triggers, I'll do some testing tomorrow.

Nightly-ux autocomplete crashhhh bp-265af365-a910-4afe-bf73-cbad02110721

(In reply to comment #122) > Nightly-ux autocomplete crashhhh > > bp-265af365-a910-4afe-bf73-cbad02110721 Do you remember what you did to cause this crash? I haven't had a crash on my PC at all.

(In reply to comment #123) > (In reply to comment #122) > > Nightly-ux autocomplete crashhhh > > > > bp-265af365-a910-4afe-bf73-cbad02110721 > > Do you remember what you did to cause this crash? I haven't had a crash on > my PC at all. I got that crash when i type on location bar.

the stack seems pretty strange, I'd first look at the add-ons. Could be a bad interaction with the Twitter Address Bar Search

with Twitter Address Bar Search addon, i get an error when i'm typing on location bar.

hmm... not sure, if the following behavior is intended or a bug: I typed "g" into the location bar and "groups.google.com/" is autofilled in the locationbar. But in the result list "groups.google.com/" is not on very top - it is at no.5. Please see the enclosed screenshot.

Recently I've found that editing URLs on the UX branch is noticeably slower, to the point where the URL bar becomes unresponsive as I try to enter text. It seems to be more of a problem with longer URLs. I'm assuming that this patch is at fault, since I haven't seen any other recent patches to the location bar. If this patch is causing this problem, maybe we should turn off the inline autocomplete behavior in this case, since autocomplete isn't very useful if you are editing the middle of a URL, since the half-edited URL likely wouldn't have any matches (or match what you want it to).

(In reply to comment #128) > Recently I've found that editing URLs on the UX branch is noticeably slower, > to the point where the URL bar becomes unresponsive as I try to enter text. > It seems to be more of a problem with longer URLs. I see this too. > autocomplete isn't very useful if > you are editing the middle of a URL, since the half-edited URL likely > wouldn't have any matches (or match what you want it to). Agreed.

(In reply to comment #127) > Created attachment 547478 [details] > Screenshot > > hmm... not sure, if the following behavior is intended or a bug: > > I typed "g" into the location bar and "groups.google.com/" is autofilled in > the locationbar. But in the result list "groups.google.com/" is not on very > top - it is at no.5. > > Please see the enclosed screenshot. Yes this is one of the many problems that occurs using the approach used in this patch. I still strongly recommend mak to look at the addon Speak Words from Mozilla Labs , and try to follow approach used there. It far better handles priorities of the user, and is relatively faster . see comment #105

(In reply to comment #130) > (In reply to comment #127) > > Created attachment 547478 [details] > > Screenshot > > > > hmm... not sure, if the following behavior is intended or a bug: > > > > I typed "g" into the location bar and "groups.google.com/" is autofilled in > > the locationbar. But in the result list "groups.google.com/" is not on very > > top - it is at no.5. > > > > Please see the enclosed screenshot. > > Yes this is one of the many problems that occurs using the approach used in > this patch. > I still strongly recommend mak to look at the addon Speak Words from Mozilla > Labs , and try to follow approach used there. It far better handles > priorities of the user, and is relatively faster . see comment #105 It's limi who is designing the user experience of this bug, and he intended it to only work with URLs in the classic way.

the performance problem is orthogonal to the designing part, we should probably backout it from UX till that issue is solved, it's pretty clear that's a blocking issue. We should try to have only the synchronous query hit with delay 0, and as I explained previously that query can be made far more performant.

(In reply to comment #132) > the performance problem is orthogonal to the designing part, we should > probably backout it from UX till that issue is solved, it's pretty clear > that's a blocking issue. > We should try to have only the synchronous query hit with delay 0, and as I > explained previously that query can be made far more performant. Totally agree with mak , backing out will be a good idea here. Also regarding comment #131 , even for URLs to work classic way , they must atleast be in sync with the results displayed at the awesomebar popup.

If anyone can back the patch out of UX they're welcome to, as I'm about to go to sleep here (and TBH am still not sure how to perform backouts with hg). Comment 112 lists the changesets.

(In reply to comment #134) > If anyone can back the patch out of UX they're welcome to, as I'm about to > go to sleep here (and TBH am still not sure how to perform backouts with hg). The UX branch isn't guaranteed to be non-busted, so you don't have to rush to back things out, but part of landing patches on the UX branch is taking responsibility for them. I've found that |hg backout changeset| works well, and there are docs on it here: https://developer.mozilla.org/en/Mercurial_FAQ#Backing_out_a_single_changeset. Feel free to ping me in #ux if you need help with this.

Also, hg backout will merge automatically if you add --merge. Works 99% of the time: hg backout --merge changeset hg commit -m "merge backout"

I backed it out of UX.

(In reply to Marco Bonardo [:mak] from comment #117) > there is another privacy hit, if I "forget about this page" or site, and > there are no more pages for that hostname, it will keep appearing even if I > wanted to get rid of it. > So... we may add a count column to each hostname entry, that is number of > times it appears in moz_places and remove it when it reaches 0, a temp > trigger may keep these in sync easily. I've been spending a lot of time figuring out a way to solve this, and am still having difficulty. Here's the SQL query I use to populate the domain table: "INSERT OR IGNORE INTO moz_hostnames_temp (host) " + "SELECT get_unreversed_host(lower(h.rev_host), 1) AS host " + "FROM moz_places h " + "WHERE h.hidden = 0 AND h.frecency <> 0 " + "ORDER BY h.frecency DESC, h.id DESC" I can't figure out a way to modify that query to increment a column if that tuple exists, do you know if its possible in an SQL statement? And regarding removal, we can't use triggers because the deletions will happen on a different connection. It seems only nsNavHistory handles removal of places rows and History.cpp doesn't, correct? This problem won't be hard to solve if I can solve my increment column problem above, otherwise I may have to do some expensive and complicated queries. Marco, any advice you could give?

(In reply to Michael Ventnor from comment #138) > I can't figure out a way to modify that query to increment a column if that > tuple exists, do you know if its possible in an SQL statement? you may use a subquery to count instances of that specific rev_host, or try with a group by, like. SELECT get_unreversed_host(lower(h.rev_host), 1) AS host, count(*) as count FROM moz_places h WHERE h.hidden = 0 AND h.frecency <> 0 GROUP BY rev_host ORDER BY h.frecency DESC, h.id DESC > And regarding removal, we can't use triggers because the deletions will > happen on a different connection. yeah, the problem is that the triggers are temporary, there are possible ways to handle this: - handle additions/removals manually, using a nsINavHistoryObserver - run the query (and create triggers) on the main db connection (nsPIPlacesDatabase.DBConnection), rather than on the cloned connection - add a permanent hostnames table to places.sqlite with permanent triggers The third one may be a long term solution, this is not the first case we hit the problem of ordering hosts by frecency (see about:permissions), but may require larger changes that are more pertinent to a full schema revise. The first approach is more prone to count errors, and adds more overhead to all add/remove page operations. The second approach has advantages in data integrity, simplicity and since we have to query synchronously it's not a big deal if it blocks. The disadvantage is the initial table population that may fight with other stuff on the main connection, but if we can limit it may be acceptable. I'd probably try the second one, unless there is specific need to join with the temp tables we create on the current cloned connection.

Typing "h" makes this go insane and doesn't let you complete your url for your history is rich with "http" pages...

(In reply to Marco Bonardo [:mak] from comment #139) > - run the query (and create triggers) on the main db connection > (nsPIPlacesDatabase.DBConnection), rather than on the cloned connection How is this one possible? Temp tables are only visible to the connection that created them (and we need to use a cloned connection because the main connection is often used for long cleanup queries).

(In reply to Michael Ventnor from comment #141) > (In reply to Marco Bonardo [:mak] from comment #139) > > - run the query (and create triggers) on the main db connection > > (nsPIPlacesDatabase.DBConnection), rather than on the cloned connection > > How is this one possible? Temp tables are only visible to the connection > that created them (and we need to use a cloned connection because the main > connection is often used for long cleanup queries). You should create everything on the main connection with this approach, included the temp table. Long cleanup queries on main thread will block the ui regardless, so it's hard to figure out how you could use inline autocomplete at that time. The only problem may be some added delay to async queries updating stuff into the table. If you think it's safer to go with the first solution we can try it, offhand is hard to guess which one of the two will work better :( If you prefer to spend more time and go for the third solution, we'd probably want a moz_hostnames table with host and frecency(max), maybe also favicon_id (there has been request in the past to figure out the best icon for a hostname), and triggers to keep it up-to-date.

(In reply to Marco Bonardo [:mak] from comment #142) > (In reply to Michael Ventnor from comment #141) > > (In reply to Marco Bonardo [:mak] from comment #139) > > > - run the query (and create triggers) on the main db connection > > > (nsPIPlacesDatabase.DBConnection), rather than on the cloned connection > > > > How is this one possible? Temp tables are only visible to the connection > > that created them (and we need to use a cloned connection because the main > > connection is often used for long cleanup queries). > > You should create everything on the main connection with this approach, > included the temp table. Long cleanup queries on main thread will block the > ui regardless, so it's hard to figure out how you could use inline > autocomplete at that time. The only problem may be some added delay to async > queries updating stuff into the table. They're not done on the main thread, they're done off thread on the main connection. The connection will block if asked to do a synchronous query (query temp table) while it's already in the middle of something.

(In reply to Michael Ventnor from comment #143) > They're not done on the main thread, they're done off thread on the main > connection. The connection will block if asked to do a synchronous query > (query temp table) while it's already in the middle of something. If you query the temp table in the background thread how do you satisfy your need of having a synchronous UI while the user types? i thought this was the target.

(In reply to Marco Bonardo [:mak] from comment #144) > (In reply to Michael Ventnor from comment #143) > > They're not done on the main thread, they're done off thread on the main > > connection. The connection will block if asked to do a synchronous query > > (query temp table) while it's already in the middle of something. > > If you query the temp table in the background thread how do you satisfy your > need of having a synchronous UI while the user types? i thought this was the > target. The temp table isn't done on the background thread, I'm talking about general background work done on places.sqlite. It doesn't block the UI but it does block the SQL connection. If we try to launch a synchronous query on it (the autocomplete query), then it will block the UI while we wait for that connection to free up.

So the idea to use GROUP BY seems to have ruined the idea of using the row id to represent frecency. I can get the maximum frecency from a group by using MAX(h.frecency), here's my query so far: let fillTableQuery = this._db.createAsyncStatement( "INSERT OR IGNORE INTO moz_hostnames_temp (host, page_count) " + "SELECT get_unreversed_host(LOWER(h.rev_host), 1) AS host, COUNT(*) as page_count " + "FROM moz_places h " + "WHERE h.hidden = 0 AND h.frecency <> 0 " + "GROUP BY host " + "ORDER BY MAX(h.frecency) DESC, MAX(h.id) DESC" ); But the ordering is still broken. I suspect that using the MAX(h.frecency) technique doesn't work in an ORDER BY clause. What I suspect is happening is that when a domain has a high frecency page and low frecency page, and another domain has one middle frecency page, then the middle frecency domain is added before the domain with two pages since it can be grouped fully earlier. I hope that makes sense.

(In reply to Michael Ventnor from comment #145) > If we try to launch a synchronous query on > it (the autocomplete query), then it will block the UI while we wait for > that connection to free up. It will block the ui regardless since it's a synchronous query, btw we are in wal mode, so writers don't block readers. (In reply to Michael Ventnor from comment #146) > + "GROUP BY host " this is wrong, I used GROUP BY rev_host for a reason, host cannot use an index > But the ordering is still broken. > > I suspect that using the MAX(h.frecency) > technique doesn't work in an ORDER BY clause. seems to do the right thing here, if I have it print the max(frecency) value it is correctly ordered, it's just slow. do you have an example where it fails ordering?

Hopefully this addresses all the comments. The sorting seems to work now that I use rev_host, I wonder why considering host is a valid option.

Comment on attachment 555982 [details] [diff] [review] Patch 3 Review of attachment 555982 [details] [diff] [review]: ----------------------------------------------------------------- this is just a quick look-through, I'll have more time later to dig further in code details ::: toolkit/components/places/nsNavHistory.cpp @@ +1116,5 @@ > + * @param revHost > + * A reversed hostname. This is the value from the rev_host column. > + * @param removeWWW > + * Whether to remove "www." if it exists at the start of the > + * unreversed string. in future we may decide to strip other prefixes (ftp, www2,...) so maybe we should keep this argument generic enough like a removePrefix or normalizeHostname. @@ +3463,5 @@ > "SELECT null, " > "'place:type=%ld&sort=%ld&domain='||host||'&domainIsHost=true'%s, " > "host, host, null, null, null, null, null, null, null " > "FROM ( " > + "SELECT get_unreversed_host(h.rev_host, 0) AS host " But ideally, since this is a change that may affect addons compatibility, if we may avoid it completely, would even be better. To avoid it we should modify code so that the query doesn't need to get the unreversed host, only the final user of the value needs it, then it can post-filter. if that's not feasible we'll take the compatibility hit, but we have to communicate it in the usual document for add-ons compatibility in whatever version this will land. ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +101,5 @@ > > const kBrowserUrlbarBranch = "browser.urlbar."; > > +const kTextURIService = Cc["@mozilla.org/intl/texttosuburi;1"] > + .getService(Ci["nsITextToSubURI"]); This was a lazy service getter, why did you make it a direct getService? @@ +163,5 @@ > + else if (uri.indexOf("ftp://") == 0) > + uri = uri.slice(6); > + > + if (uri.indexOf("www.") == 0) > + uri = uri.slice(4); nit: per this service code style, please brace oneline ifs @@ +452,4 @@ > > + // Unlike nsURLInlineComplete, we don't want this search to start > + // synchronously. Wait 50ms before launching the query. > + if (!("_startTimer" in this)) { you may just check !this._startTimer, shouldn't complain @@ +460,5 @@ > + let timerCallback = function() { > + self._doStartSearch(aSearchString, aSearchParam, > + aPreviousResult, aListener); > + }; > + this._startTimer.initWithCallback(timerCallback, 50, Ci.nsITimer.TYPE_ONE_SHOT); magic numbers should go into const at the top of the file @@ +466,5 @@ > > stopSearch: function PAC_stopSearch() > { > // We need to cancel our searches so we do not get any [more] results. > + if ("_pendingQuery" in this) { as well as here this._pendingQuery shouldn't complain @@ +474,2 @@ > > + if ("_startTimer" in this) { and this._startTimer @@ +474,3 @@ > > + if ("_startTimer" in this) { > + this._startTimer.cancel(); but if you don't set _startTimer to null (or delete it) the previous if will always resolve to true, even if there is no need to cancel it. @@ +610,2 @@ > { > + this._startTimer.cancel(); should delete or set it to null too @@ +619,5 @@ > > + this._currentSearchString = > + fixupSearchText(this._originalSearchString.toLowerCase()); > + > + var searchParamParts = aSearchParam.split(" "); nit: most of this svc uses let, this var is probably a leftover @@ +1216,5 @@ > + // Do not warn about the sorting, as id is a primary key so it > + // has an implicit index. > + return this._db.createStatement( > + "/* do not warn (bug 566489) */" > + + "SELECT host || '/' FROM moz_hostnames_temp " nit: FROM should stay in its own line please @@ +1239,5 @@ > + > + // register observers > + this._os = Cc["@mozilla.org/observer-service;1"]. > + getService(Ci.nsIObserverService); > + this._os.addObserver(this, kTopicShutdown, false); Services.obs @@ +1411,5 @@ > + } > + } > + > + if (Object.getOwnPropertyDescriptor(this, "_db").value !== undefined) { > + this._db.asyncClose(); your db connection is not a clone, it's the main connection, so it's not your duty to close it (indeed you should not) @@ +1452,5 @@ > + // Trigger for adding a new page to moz_places. > + this._db.executeSimpleSQL( > + "CREATE TEMP TRIGGER moz_hostnames_temp_afterinsert_trigger " > + + "AFTER INSERT ON moz_places FOR EACH ROW " > + + "WHEN NEW.hidden = 0 AND NEW.frecency <> 0 " maybe you may want to rather filter on nonempty rev_host (thus skipping STRLEN(NEW.rev_host) < 2) rather than on frecency, should be more effective and you don't care about those @@ +1458,5 @@ > + + "INSERT OR IGNORE INTO moz_hostnames_temp (host, page_count) " > + + "VALUES (get_unreversed_host(LOWER(NEW.rev_host), 1), 0); " > + + "UPDATE moz_hostnames_temp " > + + "SET page_count = page_count + 1 " > + + "WHERE host = get_unreversed_host(LOWER(NEW.rev_host), 1); " you may try to do something like an INSERT OR UPDATE INTO moz_hostnames_temp (id, host, page_count) VALUES (OLD.id, get_unreversed_host(LOWER(NEW.rev_host), 1), IFNULL(page_count + 1, 0)) we used a similar trick in the past, the idea is that you enforce reusing the same old id I was thinking of a possible alternative to changing get_unreversed_host signature, maybe we could cook up something with WHEN CASE substr(rev_host) = "www" THEN substr(rev_host) ELSE rev_host END @@ +1483,5 @@ > + + "SELECT get_unreversed_host(LOWER(h.rev_host), 1) AS host, COUNT(*) as page_count " > + + "FROM moz_places h " > + + "WHERE h.hidden = 0 AND h.frecency <> 0 " > + + "GROUP BY h.rev_host " > + + "ORDER BY MAX(h.frecency) DESC, MAX(h.id) DESC" I'd probably ignore collisions here, dropping the MAX(h.id) ordering, for perf reasons This query is unlimited, I'm worried by how it may behave on 100 000 entries... I'd probably sacrifice some perfect match for perf, by limiting it to a certain number of more frecent hosts.

(In reply to Marco Bonardo [:mak] from comment #149) > Comment on attachment 555982 [details] [diff] [review] > Patch 3 > > Review of attachment 555982 [details] [diff] [review]: > ----------------------------------------------------------------- > > this is just a quick look-through, I'll have more time later to dig further > in code details > > ::: toolkit/components/places/nsNavHistory.cpp > @@ +1116,5 @@ > > + * @param revHost > > + * A reversed hostname. This is the value from the rev_host column. > > + * @param removeWWW > > + * Whether to remove "www." if it exists at the start of the > > + * unreversed string. > > in future we may decide to strip other prefixes (ftp, www2,...) so maybe we > should keep this argument generic enough like a removePrefix or > normalizeHostname. > > @@ +3463,5 @@ > > "SELECT null, " > > "'place:type=%ld&sort=%ld&domain='||host||'&domainIsHost=true'%s, " > > "host, host, null, null, null, null, null, null, null " > > "FROM ( " > > + "SELECT get_unreversed_host(h.rev_host, 0) AS host " > > But ideally, since this is a change that may affect addons compatibility, if > we may avoid it completely, would even be better. To avoid it we should > modify code so that the query doesn't need to get the unreversed host, only > the final user of the value needs it, then it can post-filter. What do you mean by this? Implement a new different SQL function? > ::: toolkit/components/places/nsPlacesAutoComplete.js > @@ +101,5 @@ > > > > const kBrowserUrlbarBranch = "browser.urlbar."; > > > > +const kTextURIService = Cc["@mozilla.org/intl/texttosuburi;1"] > > + .getService(Ci["nsITextToSubURI"]); > > This was a lazy service getter, why did you make it a direct getService? In order to pull out the code which stripped the uri prefixes, so it can be shared between the two services. > @@ +474,3 @@ > > > > + if ("_startTimer" in this) { > > + this._startTimer.cancel(); > > but if you don't set _startTimer to null (or delete it) the previous if will > always resolve to true, even if there is no need to cancel it. > > @@ +610,2 @@ > > { > > + this._startTimer.cancel(); > > should delete or set it to null too I was hoping to reuse the timer object. I'll see if there's something else I can check to make that possible. > @@ +1452,5 @@ > > + // Trigger for adding a new page to moz_places. > > + this._db.executeSimpleSQL( > > + "CREATE TEMP TRIGGER moz_hostnames_temp_afterinsert_trigger " > > + + "AFTER INSERT ON moz_places FOR EACH ROW " > > + + "WHEN NEW.hidden = 0 AND NEW.frecency <> 0 " > > maybe you may want to rather filter on nonempty rev_host (thus skipping > STRLEN(NEW.rev_host) < 2) rather than on frecency, should be more effective > and you don't care about those Does an empty rev_host represent an item of 0 frecency? > I was thinking of a possible alternative to changing get_unreversed_host > signature, maybe we could cook up something with WHEN CASE substr(rev_host) > = "www" THEN substr(rev_host) ELSE rev_host END This might help, but it sounds messy given how many queries in this service use get_unreversed_host > @@ +1483,5 @@ > > + + "SELECT get_unreversed_host(LOWER(h.rev_host), 1) AS host, COUNT(*) as page_count " > > + + "FROM moz_places h " > > + + "WHERE h.hidden = 0 AND h.frecency <> 0 " > > + + "GROUP BY h.rev_host " > > + + "ORDER BY MAX(h.frecency) DESC, MAX(h.id) DESC" > > I'd probably ignore collisions here, dropping the MAX(h.id) ordering, for > perf reasons > > This query is unlimited, I'm worried by how it may behave on 100 000 > entries... I'd probably sacrifice some perfect match for perf, by limiting > it to a certain number of more frecent hosts. Is the perf hit really that bad? I tried removing it and ran into situations where the autocompleted value wasn't the highest matching row in the popup.

(In reply to Michael Ventnor from comment #150) > > But ideally, since this is a change that may affect addons compatibility, if > > we may avoid it completely, would even be better. To avoid it we should > > modify code so that the query doesn't need to get the unreversed host, only > > the final user of the value needs it, then it can post-filter. > > What do you mean by this? Implement a new different SQL function? nope, or we figure out a way to not need to do the www removal in the function (for example by using CASE WHEN as I suggested above), or we should rememeber to notify the addon-compat. the former is preferrable since won't break add-ons, but may even be harder to do. > > ::: toolkit/components/places/nsPlacesAutoComplete.js > > @@ +101,5 @@ > > > > > > const kBrowserUrlbarBranch = "browser.urlbar."; > > > > > > +const kTextURIService = Cc["@mozilla.org/intl/texttosuburi;1"] > > > + .getService(Ci["nsITextToSubURI"]); > > > > This was a lazy service getter, why did you make it a direct getService? > > In order to pull out the code which stripped the uri prefixes, so it can be > shared between the two services. I still don't understand why it can't be a global service lazy getter then, just moving it out of the constructor will be a service getter for the global object. XPCOMUtils.defineLazyServiceGetter(this, "gTextURIService", "@mozilla.org/intl/texttosuburi;1", "nsITextToSubURI"); > I was hoping to reuse the timer object. I'll see if there's something else I > can check to make that possible. shouldn't be that much expensive, btw nsITimer doesn't allow to know if it's currently running afaict. > > @@ +1452,5 @@ > > > + // Trigger for adding a new page to moz_places. > > > + this._db.executeSimpleSQL( > > > + "CREATE TEMP TRIGGER moz_hostnames_temp_afterinsert_trigger " > > > + + "AFTER INSERT ON moz_places FOR EACH ROW " > > > + + "WHEN NEW.hidden = 0 AND NEW.frecency <> 0 " > > > > maybe you may want to rather filter on nonempty rev_host (thus skipping > > STRLEN(NEW.rev_host) < 2) rather than on frecency, should be more effective > > and you don't care about those > > Does an empty rev_host represent an item of 0 frecency? not completely, it represents a place: query (that indeed has zero frecency), and iirc a single dot "." represents local files (that you don't care about). The only thing with frecency 0 that would be left out of this filter are unvisited children of livemarks, we may ignore this issue, there is long term plan to remove livemark children from this table btw. > > I was thinking of a possible alternative to changing get_unreversed_host > > signature, maybe we could cook up something with WHEN CASE substr(rev_host) > > = "www" THEN substr(rev_host) ELSE rev_host END > > This might help, but it sounds messy given how many queries in this service > use get_unreversed_host it would probably be a bit more verbose I admit, but I didn't try to implement it. > > @@ +1483,5 @@ > > > + + "SELECT get_unreversed_host(LOWER(h.rev_host), 1) AS host, COUNT(*) as page_count " > > > + + "FROM moz_places h " > > > + + "WHERE h.hidden = 0 AND h.frecency <> 0 " > > > + + "GROUP BY h.rev_host " > > > + + "ORDER BY MAX(h.frecency) DESC, MAX(h.id) DESC" > > > > I'd probably ignore collisions here, dropping the MAX(h.id) ordering, for > > perf reasons > > > > This query is unlimited, I'm worried by how it may behave on 100 000 > > entries... I'd probably sacrifice some perfect match for perf, by limiting > > it to a certain number of more frecent hosts. > > Is the perf hit really that bad? I tried removing it and ran into situations > where the autocompleted value wasn't the highest matching row in the popup. you will often hit these cases regardless, since to really get the top matching row in the popup you should take into account also adaptive history and reproduce completely the original autocomplete mix. Not limiting the query makes so that SQLite will copy all entries from moz_places in the memory cache, that is a lot of input and a lot of memory, my database has 154 000 entries, for example.

(In reply to Marco Bonardo [:mak] from comment #151) > Not limiting the query makes so that SQLite will copy all entries from > moz_places in the memory cache, that is a lot of input and a lot of memory, > my database has 154 000 entries, for example. By this, do you mean adding a LIMIT clause, or adding an extra condition to WHERE with a minimum frecency? Because I don't think either of these solves the problem of limiting the number of entries copied to a memory cache. ...Or am I not understanding this correctly?

(In reply to Marco Bonardo [:mak] from comment #149) > you may try to do something like an > INSERT OR UPDATE INTO moz_hostnames_temp (id, host, page_count) > VALUES (OLD.id, get_unreversed_host(LOWER(NEW.rev_host), 1), > IFNULL(page_count + 1, 0)) > we used a similar trick in the past, the idea is that you enforce reusing > the same old id Really? This is an INSERT trigger, so there is no OLD row to go by. I tried this and, indeed, it doesn't work.

(In reply to Michael Ventnor from comment #153) > Really? This is an INSERT trigger, so there is no OLD row to go by. I tried > this and, indeed, it doesn't work. ehr, sorry too much context switching, I meant VALUES((SELECT id FROM moz_hostnames_temp WHERE host = get_unreversed_host....), whatever)

I think I've addressed your comments, but there are some clarifications I hope you'll provide before you go away on the 7th: - How can I limit queries in a way that lowers the memory cache usage? Thinking about how the LIMIT word works, that doesn't seem like that's what you meant. - I still believe an SQL function is the best approach for removing URL prefixes. Not removing them will result in more unnecessary rows in memory, and I believe the CASE approach is far too verbose, because a) some queries are complicated enough, and b) it is used very frequently. If SQLite supported variables, or you can find a way to optimize my queries, I'll look again into it.

Feel free to land this on the UX branch for testing on a good subset of people that care about these things. :) Instructions: https://wiki.mozilla.org/UX_Branch Let us know in #ux on irc.mozilla.org if you need any assistance.

Pushed to UX and added to etherpad.

(In reply to Margaret Leibovic [:margaret] from comment #128) > Recently I've found that editing URLs on the UX branch is noticeably slower, > to the point where the URL bar becomes unresponsive as I try to enter text. > It seems to be more of a problem with longer URLs. I'm assuming that this > patch is at fault, since I haven't seen any other recent patches to the > location bar. I'm still experiencing this problem with the new version of the patch. I really think this needs to be addressed before we'd ship this.

(In reply to Margaret Leibovic [:margaret] from comment #158) > (In reply to Margaret Leibovic [:margaret] from comment #128) > > Recently I've found that editing URLs on the UX branch is noticeably slower, > > to the point where the URL bar becomes unresponsive as I try to enter text. > > It seems to be more of a problem with longer URLs. I'm assuming that this > > patch is at fault, since I haven't seen any other recent patches to the > > location bar. > > I'm still experiencing this problem with the new version of the patch. I > really think this needs to be addressed before we'd ship this. Yes, seeing this too. But I just wanted to state for the record that it's OK to have this kind of minor bustage and slowdowns on the UX branch until it gets fixed, so don't feel like you have to back it out before investigating a fix.

I've found some problem with the site that not working without 'www'. I bookmarked http://www.set.or.th/. if i tend to skip typing 'www' before the domain (i typed 'set'), the autocomplete shows set.or.th/ and when i press enter, firefox go to http://set.or.th/ and displays as 'Server not found' page. On Google Chrome, I didn't get this problem. when i typed 'set' then press enter, google chrome take me to http://www.set.or.th/

(In reply to Ek from comment #160) > I've found some problem with the site that not working without 'www'. I > bookmarked http://www.set.or.th/. if i tend to skip typing 'www' before the > domain (i typed 'set'), the autocomplete shows set.or.th/ and when i press > enter, firefox go to http://set.or.th/ and displays as 'Server not found' If the site is bookmarked or in the history with the "www" it might be ok to assume the "www," but it can't just be automatically assumed because http://set.or.th/ is a valid URL and if Firefox automatically prepended "www" users would have no way to get to that server.

Comment on attachment 558237 [details] [diff] [review] Patch 4 Review of attachment 558237 [details] [diff] [review]: ----------------------------------------------------------------- I'm looking into a debug build with the patches applied to check where we are. One thing I noticed is that SQLite pages cache seem to immediately increase to the top value, 50MB in my case, that is somehow what I was expecting from reading the entire table in one chunk. This is probably an expense we can't afford, while SQlite cache memory hardly grows once it reaches a stable value, an increase at the first search would be extremely visible on memory benchmarks. Either we do the initial read in steps accepting that in the first minutes of app life the results may just not exist, or we finally decide to change the schema and add a real moz_hostnames table to places.sqlite. On a related note I think it's possible to implement fast reverse string searching in SQLite by implementing a REVERSE collation, building an index with it and doing comparison specifying COLLATE REVERSE. I was evaluating this for a new schema where we'd have a hostnames table with a column where we may search both forward and backward. I'm not sure if it's going to satisfy our needs yet, but is worth investigating apart. Doing the read in steps doesn't seem trivial though, due to the group by. At this point moving on requires going different ways: 1. finally change the schema, add moz_hostname (id, hostname, frecency), index on frecency and triggers to keep it in sync. With WAL writes should be cheap enough. 2. add back frecency to the temp table, asynchronously query moz_places in chunks of 500 as an insert or ignore of "SELECT h.rev_host, MAX(frecency) FROM moz_places h GROUP BY (h.rev_host) HAVING LENGTH(h.rev_host) > 1 LIMIT 0,500". At this point we have to drop the idea to use id as a sorting column, and add a further index on frecency. I suppose at this point, rather than spending time implementing again something that may cause contention, we should go for option 1, then we can again use the special autocomplete connection and regain concurrency. In the hypothetical future schema we may regain the additional required space by having a host_id in the pages table, rather than repeating the host string over and over (in my DB that would save about 3MB). Regarding editing slowness, I don't think we can improve the query, what may be possible would be to do caching, if the new string is a subset of the previous one we may skip the database query. The only issue with that may be a privacy concern, we may probably invalidate this cache on a timer then. Thoughts? I'm sorry if this is looking like a neverending story, it's not a trivial task and it's prone to error-and-retry. ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +1454,5 @@ > + + "AFTER INSERT ON moz_places FOR EACH ROW " > + + "WHEN NEW.hidden = 0 AND NEW.rev_host != '' " > + + "BEGIN " > + + "INSERT OR REPLACE INTO moz_hostnames_temp (id, host, page_count) " > + + "VALUES ((SELECT id FROM moz_hostnames_temp WHERE host = fixup_url(get_unreversed_host(LOWER(NEW.rev_host)))), " why do we need LOWER in these queries, I think we already lowercase all urls and hosts we put in the database @@ +1479,5 @@ > + let fillTableQuery = this._db.createAsyncStatement( > + "INSERT OR IGNORE INTO moz_hostnames_temp (host, page_count) " > + + "SELECT fixup_url(get_unreversed_host(LOWER(h.rev_host))) AS host, COUNT(*) as page_count " > + + "FROM moz_places h " > + + "WHERE h.hidden = 0 AND h.rev_host != '' " should probably be LENGTH(h.rev_host) > 1 to also skip ".", most likely it's faster as a GROUP BY h.rev_host HAVING LENGTH(h.rev_host) > 1 @@ +1482,5 @@ > + + "FROM moz_places h " > + + "WHERE h.hidden = 0 AND h.rev_host != '' " > + + "GROUP BY h.rev_host " > + + "ORDER BY MAX(h.frecency) DESC, MAX(h.id) DESC" > + ); this takes more than 1 second on my profile (on a pretty fast i7 too), it's far too much to add to the start path :(

(In reply to Marco Bonardo [:mak] from comment #162) > Comment on attachment 558237 [details] [diff] [review] [diff] [details] [review] > Patch 4 > > Review of attachment 558237 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > I'm looking into a debug build with the patches applied to check where we > are. > One thing I noticed is that SQLite pages cache seem to immediately increase > to the top value, 50MB in my case, that is somehow what I was expecting from > reading the entire table in one chunk. > This is probably an expense we can't afford, while SQlite cache memory > hardly grows once it reaches a stable value, an increase at the first search > would be extremely visible on memory benchmarks. > Either we do the initial read in steps accepting that in the first minutes > of app life the results may just not exist, or we finally decide to change > the schema and add a real moz_hostnames table to places.sqlite. On a related > note I think it's possible to implement fast reverse string searching in > SQLite by implementing a REVERSE collation, building an index with it and > doing comparison specifying COLLATE REVERSE. I was evaluating this for a new > schema where we'd have a hostnames table with a column where we may search > both forward and backward. I'm not sure if it's going to satisfy our needs > yet, but is worth investigating apart. > Doing the read in steps doesn't seem trivial though, due to the group by. At > this point moving on requires going different ways: > 1. finally change the schema, add moz_hostname (id, hostname, frecency), > index on frecency and triggers to keep it in sync. With WAL writes should be > cheap enough. > 2. add back frecency to the temp table, asynchronously query moz_places in > chunks of 500 as an insert or ignore of "SELECT h.rev_host, MAX(frecency) > FROM moz_places h GROUP BY (h.rev_host) HAVING LENGTH(h.rev_host) > 1 LIMIT > 0,500". At this point we have to drop the idea to use id as a sorting > column, and add a further index on frecency. > > I suppose at this point, rather than spending time implementing again > something that may cause contention, we should go for option 1, then we can > again use the special autocomplete connection and regain concurrency. In the > hypothetical future schema we may regain the additional required space by > having a host_id in the pages table, rather than repeating the host string > over and over (in my DB that would save about 3MB). > > Regarding editing slowness, I don't think we can improve the query, what may > be possible would be to do caching, if the new string is a subset of the > previous one we may skip the database query. The only issue with that may be > a privacy concern, we may probably invalidate this cache on a timer then. > > Thoughts? > I'm sorry if this is looking like a neverending story, it's not a trivial > task and it's prone to error-and-retry. The idea for a new permanent schema looks good and I've been trying to get something going. Here are some potential issues: 1) For the feature to work as expected, the domain search must be synchronous and block the main thread. If the table is now on disk instead of memory, would that be too slow? 2) We have a frecency column in the table, but how do we determine the frecency and how do we update it? Do we simply take the maximum frecency amount and only update it when a place receives an even higher frecency? Or may we need a foreign key pointing to the place we get the frecency from, and update the frecency on that since the frecency may even decrease? 3) We'd also need a page_count column to keep track of how many pages have that hostname and when the row should be deleted if a place is deleted? > ::: toolkit/components/places/nsPlacesAutoComplete.js > @@ +1454,5 @@ > > + + "AFTER INSERT ON moz_places FOR EACH ROW " > > + + "WHEN NEW.hidden = 0 AND NEW.rev_host != '' " > > + + "BEGIN " > > + + "INSERT OR REPLACE INTO moz_hostnames_temp (id, host, page_count) " > > + + "VALUES ((SELECT id FROM moz_hostnames_temp WHERE host = fixup_url(get_unreversed_host(LOWER(NEW.rev_host)))), " > > why do we need LOWER in these queries, I think we already lowercase all urls > and hosts we put in the database I don't see any code where we lowercase URLs or reversed hostnames. > @@ +1479,5 @@ > > + let fillTableQuery = this._db.createAsyncStatement( > > + "INSERT OR IGNORE INTO moz_hostnames_temp (host, page_count) " > > + + "SELECT fixup_url(get_unreversed_host(LOWER(h.rev_host))) AS host, COUNT(*) as page_count " > > + + "FROM moz_places h " > > + + "WHERE h.hidden = 0 AND h.rev_host != '' " > > should probably be LENGTH(h.rev_host) > 1 to also skip ".", most likely it's > faster as a GROUP BY h.rev_host HAVING LENGTH(h.rev_host) > 1 > > @@ +1482,5 @@ > > + + "FROM moz_places h " > > + + "WHERE h.hidden = 0 AND h.rev_host != '' " > > + + "GROUP BY h.rev_host " > > + + "ORDER BY MAX(h.frecency) DESC, MAX(h.id) DESC" > > + ); > > this takes more than 1 second on my profile (on a pretty fast i7 too), it's > far too much to add to the start path :( I'm guessing this won't be a problem with the disk schema, since we'll only take this hit once? Because this is something that really needs to be done before the user uses the address bar. I suggested delayedStartup() in browser.js because that is after the browser window is displayed.

Sorry if I take some time to answer, but there are issues with Milan connectivity with the rest of the Internet, so I get my bugmail with some delay. (In reply to Michael Ventnor from comment #163) > 1) For the feature to work as expected, the domain search must be > synchronous and block the main thread. If the table is now on disk instead > of memory, would that be too slow? It may be a bit slower, but if that's the case we may copy it to memory, that would be much faster since the slow path in creating the temp table was doing all the groupings and frecency calculations, that now we'd get for "free". That may be a follow-up. > 2) We have a frecency column in the table, but how do we determine the > frecency and how do we update it? Do we simply take the maximum frecency > amount and only update it when a place receives an even higher frecency? Or > may we need a foreign key pointing to the place we get the frecency from, > and update the frecency on that since the frecency may even decrease? yes, this is something to figure out, what's the best frecency for a domain? SUM? AVG? At first glance I'd suggest taking the max and updating when a page gets an higher value. on page deletion we may have to recalculate the max if the removed page had the max value. > 3) We'd also need a page_count column to keep track of how many pages have > that hostname and when the row should be deleted if a place is deleted? Yes, may be more efficient than checking if moz_places has still an entry using it. in future we may use foreign keys > I don't see any code where we lowercase URLs or reversed hostnames. Should be one of the advantages of using nsIURI: Components.utils.import("resource://gre/modules/NetUtil.jsm"); var uri = NetUtil.newURI("hTtP://wwW.tEst.COm"); uri.spec; > I'm guessing this won't be a problem with the disk schema, since we'll only > take this hit once? Because this is something that really needs to be done > before the user uses the address bar. I suggested delayedStartup() in > browser.js because that is after the browser window is displayed. Yes, the point is that this price will be paid on schema migration rather than on each startup. Actually this requires a places.sqlite schema version bump, and should be done with a MigrateVXX method, as well as adding triggers and all the related stuff. See for example http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1661

I pushed the new patch to the ux branch, it creates a permanent moz_hostnames schema. Changesets are in the etherpad. If it sticks, and everyone says it's working alright, I'll attach the new patch tomorrow, and we can discuss perf.

(In reply to Michael Ventnor from comment #165) > I pushed the new patch to the ux branch, it creates a permanent > moz_hostnames schema. Changesets are in the etherpad. > If it sticks, and everyone says it's working alright, I'll attach the new > patch tomorrow, and we can discuss perf. It's failing XPCShell tests: https://tbpl.mozilla.org/?tree=UX&rev=345b6db0d4ca

that test has to be bumped with the schema version, rnewman did that in bug 675996 so you may inherit that code, you both have schema changes pending and will depend on who lands first.

I've pushed to ux again with the xpcshell tests fixed. I took the relevant parts of rnewman's patch. All tests seem to pass now on my machine.

I hope ux branch users are using sepaarate profiles from their default, since the schema bump can likely corrupt their databases when they move back to the official channel (they may end having a not-final hostnames table in the db, and when we create the final one it could clash with it)... I wonder if we should backout to reduce the impact.

btw, I didn't see interesting comments on the performances impact, that by itself may be good or bad news, either nobody noticed issues or nobody cared to report them...

The ux landing seemed alright. I dont see increased memory usage or any noticeably UI performance lag, dunno about benchmarks. Pretty satisfied

Feedback seems to be relatively few, and what feedback I've received is good. So it looks like this is ready to go.

These tests come from some of the patch from the other bug you mentioned, Marco.

Comment on attachment 566435 [details] [diff] [review] Patch 5 tests I haven't updated my tree in a while and didn't notice you bumped the schema. I'll have to update it now, but other than that the patch should be ready for checkin.

This updates to tip, and includes the test changes since they are very small now.

ok, sorry for late, I can now dedicate time to this, let's proceed! A thing I'm going to ask you is to split the patch into 2 parts. Part 1 will only implement the schema bump, we'll have to fix eventual upgrade issues from UX branch by removing the stuff we created and using a different name for the table and the trigger(s). Need to check indexes here, since we will need an additional index on the count column we should check if having that index and querying on it is more/less expensive than directly querying moz_places. This part will have to be checked on Try to avoid talos regressions, since this is a schema bump it won't be backed out (fixing databases of all users is a bad task). I think may be healthier to move this part to a separate bug too, and make this bug depend on that one. Part 2 will be the actual implementation using part 1, it may be backed out in case there are big issues. The only requirement I have is that when the .autofill pref is set to false we should not do any additional work in the added code, it should behave almost as it is today, no additional observers or queries should run. Now, I'm going to do a review pass on the latest version, and then will investigate the index thing.

Comment on attachment 566481 [details] [diff] [review] Patch 5 Review of attachment 566481 [details] [diff] [review]: ----------------------------------------------------------------- First thing, please backout from UX. The experiment was fine, we won't reland there, any testing should be done on Try builds with copied test profiles while we finalize the feature. ::: toolkit/components/places/nsNavHistory.cpp @@ +926,5 @@ > + > + if (currentSchemaVersion < 13) { > + rv = MigrateV13Up(mDBConn); > + NS_ENSURE_SUCCESS(rv, rv); > + } after the migration step we usually indicate the version that will use it, so here there should be comment like // Firefox XY uses schema version 13. where XY may vary depending on when we land. @@ +1246,5 @@ > NS_ADDREF(*_retval = result); > return NS_OK; > } > > +class mozStorageFunctionFixupUrl: public mozIStorageFunction this should go into SQLFunctions.cpp, using the same structure of other functions, in a separate bug I changed also mozStorageFunctionGetUnreversedHost to that. We are bitrotting each other but we'll figure this out later depending on landing order. @@ +1261,5 @@ > + mozIStorageValueArray* aFunctionArguments, > + nsIVariant** _retval) > +{ > + NS_ASSERTION(aFunctionArguments, "Must have non-null function args"); > + NS_ASSERTION(_retval, "Must have non-null return pointer"); Use MOZ_ASSERT() please, put the comment as a common cpp one line comment before it like // Must have non-null function args. MOZ_ASSERT(aFunctionArguments); @@ +1269,5 @@ > + > + nsresult rv; > + nsCOMPtr<nsIWritableVariant> result(do_CreateInstance( > + "@mozilla.org/variant;1", &rv)); > + NS_ENSURE_SUCCESS(rv, rv); I think you can avoid rv by using NS_ENSURE_STATE(result) here @@ +1303,2 @@ > rv = MatchAutoCompleteFunction::create(mDBConn); > NS_ENSURE_SUCCESS(rv, rv); once you move to SQLFunction.cpp style your added function should look like this @@ +1324,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mDBConn->ExecuteSimpleSQL(CREATE_HOSTNAMES_AFTERDELETE_TRIGGER); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mDBConn->ExecuteSimpleSQL(CREATE_HOSTNAMES_AFTERUPDATE_FRECENCY_TRIGGER); > + NS_ENSURE_SUCCESS(rv, rv); keeping this structure we'll need an index on page_count too, since otherwise the trigger would be too slow in finding orphans. As I previously said due to this I have to check if it's worth it since we have one more column and one more index to avoid a moz_places query... Most likely we may want physical triggers, not temp ones, the historyvisits ones are temp for "historical reasons". thus triggers creation should be moved to the MigrateV13Up function. @@ +1837,5 @@ > +nsresult > +nsNavHistory::MigrateV13Up(mozIStorageConnection *aDBConn) > +{ > + // Add the moz_hostnames table so we can get hostnames for URL > + // autocomplete. As said, if we change schema of the table, to avoid conflicts that may create headaches in future this should DROP TABLE IF EXISTS moz_hostnames DROP INDEX IF EXISTS moz_hostnames_frecencyindex and then use different names, most_hosts and moz_hosts_frecencyindex may be fine for now... but first let's see if we have to change the schema. @@ +1842,5 @@ > + nsresult rv = aDBConn->ExecuteSimpleSQL(CREATE_MOZ_HOSTNAMES); > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = aDBConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_HOSTNAMES_FRECENCY); > + NS_ENSURE_SUCCESS(rv, rv); So, migration works differently than this, when you downgrade the schema is set to the previous version, but the database stays the same. when you upgrade we run again the migration task. This happens because you may have temp triggers that on previous version may no exist, so on upgrade we guarantee schema coherence. For this reason you should check if the table exists, and create the table and the index only if they don't exist. Then you should ensure the table contents are up-to-date in both cases (if the table exists and if it does not exist). So, if the table exists your insert or ignore is going to fail updating page_count, should probably be an insert or replace. Or, if we don't add page_count, it may be fine (provided we should delete orphans) @@ +1862,5 @@ > + fillHostnameTable > + }; > + nsCOMPtr<mozIStoragePendingStatement> ps; > + rv = aDBConn->ExecuteAsync(stmts, NS_ARRAY_LENGTH(stmts), nsnull, > + getter_AddRefs(ps)); since you have only one statement you should use fillHostnameTable->ExecuteAsync, DBConn->ExecuteAsync is for a vector of statements. but it's possible we need more statements, let's figure out the final schema change first. ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +479,5 @@ > + let self = this; > + let timerCallback = function() { > + self._doStartSearch(aSearchString, aSearchParam, > + aPreviousResult, aListener); > + }; I think you can do let timerCallback = (function() {...}).bind(this); instead of using self @@ +494,5 @@ > > + if (this._startTimer) { > + this._startTimer.cancel(); > + delete this._startTimer; > + } I don't remember it but I'm pretty sure there was a reason to invoke _finishSearch even if this_pendingQuery was false, and you are changing that path, by will? I see that the query may have not started yet due to the timer, but you may restore the old behavior by leaving this._finishSearch(false); at the end of the method, and early returning in the this._startTimer if. Unless you figured out that call was completely pointless. @@ +1252,5 @@ > + XPCOMUtils.defineLazyGetter(this, "_syncQuery", function() { > + // Add a trailing slash at the end of the hostname, since we always > + // want to complete up to and including a URL separator. > + // Do not warn about the sorting, as id is a primary key so it > + // has an implicit index. this comment should be updated, we are sorting on frecency now. Does this still warn? it's possible we have to add a compound (host, frecency) index to improve perfs. To be verified. @@ +1283,5 @@ > + > + this._db = Cc["@mozilla.org/browser/nav-history-service;1"]. > + getService(Ci.nsPIPlacesDatabase). > + DBConnection. > + clone(true); Why is this not a lazygetter? I wonder if we may reuse the async autocomplete clone here, by sharing it globally... follow-up probably, this will increase memory usage as it is (each new clone spawns a cache). ::: toolkit/components/places/nsPlacesTriggers.h @@ +131,5 @@ > +) > + > +/** > + * This update trigger changes the frecency of a hostname entry if the moz_places > + * entry it was based from gets a higher frecency. This is missing reasoning on why we only bump up and don't bump down. I don't recall if this was discussed.

Ok, I've done some first check, I have not found big differences between the 2 possibilities perf side, so most likely we'll stick to the easiest one. Please move patch implementing this to a separate blocking bug, as previously pointed out. 1. drop old ux version table and index (use IF EXISTS condition) 2. Make a simple moz_hosts table with a frecency index CREATE TABLE moz_hosts ( id INTEGER PRIMARY KEY , host TEXT UNIQUE NOT NULL , frecency INTEGER ); CREATE INDEX moz_hosts_frecencyindex ON moz_hosts(frecency); 3. Use physical triggers. Note these are wrong, since in the shell I did not have your special SQLite functions, so I just used rev_host. Note the trigger names, they act on places changes so are prefixed moz_places (per internal consistency), they should live in MigrateV13Up. CREATE TRIGGER moz_places_afterinsert_trigger AFTER INSERT ON moz_places FOR EACH ROW WHEN NEW.hidden = 0 AND LENGTH(NEW.rev_host) > 1 BEGIN INSERT OR REPLACE INTO moz_hosts (id, host, frecency) VALUES ( (SELECT id FROM moz_hosts WHERE host = NEW.rev_host) NEW.rev_host, MAX((SELECT frecency FROM moz_hosts WHERE host = NEW.rev_host), NEW.frecency) ); END; CREATE TRIGGER moz_places_afterdelete_trigger AFTER DELETE ON moz_places FOR EACH ROW BEGIN DELETE FROM moz_hosts WHERE host = OLD.rev_host AND NOT EXISTS(SELECT 1 FROM moz_places WHERE rev_host = OLD.rev_host); END; CREATE TRIGGER moz_places_afterupdate_frecency_trigger AFTER UPDATE OF frecency ON moz_places FOR EACH ROW WHEN NEW.frecency >= 0 BEGIN UPDATE moz_hosts SET frecency = (SELECT MAX(frecency) FROM moz_places WHERE rev_host = NEW.rev_host) WHERE host = NEW.rev_host; END; 4. Since triggers are physical, there should be no risk to go out of sync, so on you can avoid reupdating all the table on each schema upgrade, once the table exists we consider it coherent (we'll rather file a bug to add a maintenance task to clean it up on idle).

The patches here have bitrotted. After merging mozilla-central to ux, it failed to compile. I just pushed a change to try to fix the bustage: https://hg.mozilla.org/projects/ux/rev/2511cb93bea2 This should be backed out first along with the other patches when pushing a new iteration of this feature to the ux branch. Also, IIRC, all the PR_BOOL references should be regular booleans now.

(In reply to Frank Yan (:fryn) from comment #179) > The patches here have bitrotted. > After merging mozilla-central to ux, it failed to compile. this should just be backed out, we are going to finalize it and won't hit UX anymore. I'll take care of that.

(In reply to Marco Bonardo [:mak] from comment #180) > (In reply to Frank Yan (:fryn) from comment #179) > > The patches here have bitrotted. > > After merging mozilla-central to ux, it failed to compile. > > this should just be backed out, we are going to finalize it and won't hit UX > anymore. I'll take care of that. Sorry for not being responsive today, I was busy with other things mostly to do with @page in CSS. If you could back this out for me that would be great. I have already started addressing your comments.

(In reply to Michael Ventnor from comment #181) > Sorry for not being responsive today, I was busy with other things mostly to > do with @page in CSS. no problem, I've been lagging as well. I'm now pulling ux and will proceed with the removal in minutes.

(In reply to Marco Bonardo [:mak] from comment #177) > this should go into SQLFunctions.cpp, using the same structure of other > functions, in a separate bug I changed also > mozStorageFunctionGetUnreversedHost to that. We are bitrotting each other > but we'll figure this out later depending on landing order. Marco, which bug is this? I want to change my patch to include your code, so whoever lands first it will be easy for the other to adapt their patch.

bug 695554, it's a pretty large refactoring, I'm just waiting for approval to land since it will cause a fake Ts shutdown regression on all platforms.

the bug is currently in inbound, so you can build on top of it. I filed bug 697695 for some additional code move but I can wait on that, since it's just cleanup.

I still need to keep the "do not warn" in the domain query, and I'm not sure why since I've created an index on the frecency column (and verified it with SQLite Manager). I think I've addressed all your comments, let me know if I missed anything.

This does not split the patch as I requested though, may you move the schema changes to a separate bug and make this one depend on it? (In reply to Michael Ventnor from comment #186) > I still need to keep the "do not warn" in the domain query, and I'm not sure > why since I've created an index on the frecency column (and verified it with > SQLite Manager). It's possible it can't use both host and frecency and maybe wants a unified index on (host, frecency)? What does EXPLAIN QUERY PLAN say? The other possibility is that it thinks there are a few entries and decides to ignore the index.

(In reply to Marco Bonardo [:mak] from comment #187) > What does EXPLAIN QUERY PLAN say? SEARCH TABLE moz_hosts USING INDEX sqlite_autoindex_moz_hosts_1 (host>? AND host<?) (~30000 rows) USE TEMP B-TREE FOR ORDER BY I'm guessing this "autoindex" is the index created by setting the host column as unique? If so, should I create a compound index or just leave the do not warn?

A compound index works.

(In reply to Michael Ventnor from comment #188) > I'm guessing this "autoindex" is the index created by setting the host > column as unique? Yes. Autoindex is a poor naming choice, it's just the index created by the table definition (unique in this case). (In reply to Michael Ventnor from comment #189) > A compound index works. Interesting, I had that suspect. Now the problem is that we are going to index that column twice, that will double the disk size taken by the host index and increase time for an insert. We need maximum responsiveness here, so makes sense to have the compound index. The only alternative I see is to drop the UNIQUE constraint on host and change the OR IGNORE and OR UPDATE. We should condition those on the primary key, so SELECT the primary key (is) for the host we are inserting and try to insert it as a VALUE, it should give us a similar effect as a unique index on host, with a minor perf penalty. To clarify, INSERT OR REPLACE INTO moz_hosts (host, frecency) becomes INSERT OR REPLACE INTO moz_hosts (id, host, frecency), id will be our conflict.

s/(is)/(id)

If we get an index for free with the UNIQUE constraint, might it be simpler to simply keep the do not warn comment? I'm nervous about dropping the UNIQUE constraint on the host column.

Now without the schema changes put out into a new bug.

Comment on attachment 571930 [details] [diff] [review] Patch 7 Review of attachment 571930 [details] [diff] [review]: ----------------------------------------------------------------- please file a follow-up to implement result re-use in inline autocomplete, it's a case we can likely optimize, if I type "mush" and we autocomplete to "mushroom" we shouldn't search again when I type the "r", that should reduce typing lag. ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +1281,5 @@ > + > + this._db = Cc["@mozilla.org/browser/nav-history-service;1"]. > + getService(Ci.nsPIPlacesDatabase). > + DBConnection. > + clone(true); I thought I said to make this._db a getter, and actually, I think this should listen for changes to the autoFill pref and close the connection when it's flipped off (setting stuff so that it can be restarted on another flip. That means you may have to do statements getters different too, using a local object to cache them). Really, when browser.urlbar.autoFill is set to false this should act like if it wouldn't exist and don't take any resource, to allow easy disable in Aurora in case of problems. That also means invoked methods should bail out. As it is now looks like even if autofill is disabled all of this runs searches? @@ +1314,5 @@ > + this._finishSearch(); > + return; > + } > + > + // Do a synchronous search on the in-memory table of domains. not in-memory

Comment on attachment 571930 [details] [diff] [review] Patch 7 Review of attachment 571930 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +102,5 @@ > const kBrowserUrlbarBranch = "browser.urlbar."; > > +XPCOMUtils.defineLazyServiceGetter(this, "gTextURIService", > + "@mozilla.org/intl/texttosuburi;1", > + "nsITextToSubURI"); This should be under a newly created "Globals" section as it is not a "Constant" @@ +468,5 @@ > startSearch: function PAC_startSearch(aSearchString, aSearchParam, > aPreviousResult, aListener) > { > // If a previous query is running and the controller has not taken care > // of stopping it, kill it. This comment is no longer accurate. @@ +679,5 @@ > + // Reset our search behavior to the default. > + if (this._currentSearchString) > + this._behavior = this._defaultBehavior; > + else > + this._behavior = this._emptySearchDefaultBehavior; nit: please brace all `if`s in places code @@ +1241,5 @@ > }; > > + > + > + nit: way more whitespace than needed here. @@ +1243,5 @@ > + > + > + > +//////////////////////////////////////////////////////////////////////////////// > +//// nsURLInlineComplete class nit: newline between this and class definition like rest of file @@ +1244,5 @@ > + > + > +//////////////////////////////////////////////////////////////////////////////// > +//// nsURLInlineComplete class > +function nsURLInlineComplete() Please don't use the `ns` prefix anymore in places. @@ +1291,5 @@ > + > + startSearch: function UIC_startSearch(aSearchString, aSearchParam, > + aPreviousResult, aListener) > + { > + if (this._pendingQuery) { We should include the comment from `nsPlacesAutoComplete` here as well about why we do this. @@ +1314,5 @@ > + this._finishSearch(); > + return; > + } > + > + // Do a synchronous search on the in-memory table of domains. I'm really not a fan of doing synchronous disk I/O for this still. @@ +1369,5 @@ > + let wrapper = new AutoCompleteStatementCallbackWrapper(this, this._db); > + this._pendingQuery = wrapper.executeAsync([query]); > + }, > + > + stopSearch: function UIC_stopSearch() It seems like `nsPlacesAutoComplete` and this have a lot in common and could benefit from a common base object that they both inherit from, no? @@ +1444,5 @@ > + > + ////////////////////////////////////////////////////////////////////////////// > + //// nsURLInlineComplete > + > + _db: null, You assign to this, so you don't need this here.

(In reply to Shawn Wilsher :sdwilsh from comment #195) > I'm really not a fan of doing synchronous disk I/O for this still. We absolutely must have this feature be synchronous. At least with the latest patch, there haven't been complaints about performance.

(In reply to Marco Bonardo [:mak] from comment #194) > I think this > should listen for changes to the autoFill pref and close the connection when > it's flipped off (setting stuff so that it can be restarted on another flip. > That means you may have to do statements getters different too, using a > local object to cache them). What do you mean by the last part of this comment? The lazy getter caches the result of the function, so it can't return a reference to a local object, correct? Well, it can, if the lazy getter returns a function pointer, which is the only way I can think of solving this problem. Is that what you had in mind?

(In reply to Michael Ventnor from comment #197) > What do you mean by the last part of this comment? The lazy getter caches > the result of the function, so it can't return a reference to a local > object, correct? > Well, it can, if the lazy getter returns a function pointer, which is the > only way I can think of solving this problem. Is that what you had in mind? I was thinking to the good old: __db: null, get db() { if (!this.__db) {} return this.__db; } that allows you to close and restart it.

Regarding the synchronous part, it's true we need this to be synchronous, there are a couple things we can do to improve things though, that I'd left to follow-ups: - better results reuse, to avoid queries. - use a temp table that is asynchronously kept in sync with the disk table.

(In reply to Marco Bonardo [:mak] from comment #198) > (In reply to Michael Ventnor from comment #197) > > What do you mean by the last part of this comment? The lazy getter caches > > the result of the function, so it can't return a reference to a local > > object, correct? > > Well, it can, if the lazy getter returns a function pointer, which is the > > only way I can think of solving this problem. Is that what you had in mind? > > I was thinking to the good old: > > __db: null, > get db() { > if (!this.__db) {} > return this.__db; > } > > that allows you to close and restart it. Oh, I see. I was too busy figuring out how to do this with the lazygetter helpers that I forgot about this simple trick...

I'm going to start work on something else soon. If anyone else wants to finish this up before I come back, that's fine. Order of landing: 1) Bug 660592 2) Bug 660156 3) Bug 699751 4) This bug.