Closed Bug 566489 Opened 14 years ago Closed 12 years ago

Enable inline autocomplete again, but make it smarter (perceived performance)

Categories

(Firefox :: Address Bar, defect)

Product:
Firefox
Component:
Address Bar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 12
Tracking Flags:
Tracking Status
firefox12 + disabled
firefox13 --- disabled
firefox14 --- verified
blocking2.0 --- -
status2.0 --- wontfix

People

(Reporter: limi, Assigned: ddahl)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: ux-efficiency, Whiteboard: [snappy:p1][qa!][release tracking in bug 746572])

Attachments

(4 files, 26 obsolete files)

Example of autocomplete not matching Z since the top match doesn't start with Z
25.02 KB, image/png
Details
error when typing on locationbar
481.17 KB, application/octet-stream
Details
Screenshot
64.45 KB, image/png
Details
patch 14
33.00 KB, patch
Details | Diff | Splinter Review 
(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.
Component: General → Location Bar
QA Contact: general → location.bar
Summary: Enable autocomplete again, but make it smarter (perceived performance) → Enable inline autocomplete again, but make it smarter (perceived performance)
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.
Blocks: 577235
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.
No longer blocks: 577235
Depends on: 597009
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.
Assignee: nobody → ventnor.bugzilla
Attached patch Patch (obsolete) — — Splinter Review 
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.
Attachment #476598 - Flags: review?(vladimir)
(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.
Attachment #476598 - Flags: review?(vladimir)
Blocks: 579716
Attached patch Patch 2 (obsolete) — — Splinter Review 
Passes tryserver with flying colours.
Attachment #476598 - Attachment is obsolete: true
Attachment #476763 - Flags: review?(vladimir)
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?
Attachment #476763 - Flags: review?(vladimir) → review?(sdwilsh)
(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.
Attachment #476763 - Flags: feedback+
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.
blocking2.0: --- → -
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.
Attachment #476763 - Flags: review?(sdwilsh) → review-
(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).
Attached patch Patch 3 (obsolete) — — Splinter Review 
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.
Attachment #476763 - Attachment is obsolete: true
Attachment #531855 - Flags: feedback?(sdwilsh)
(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.
Attached patch Patch 4 (obsolete) — — Splinter Review 
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.
Attachment #531855 - Attachment is obsolete: true
Attachment #532135 - Flags: review?(sdwilsh)
Attachment #531855 - Flags: feedback?(sdwilsh)
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.
Attachment #532135 - Flags: review?(mak77)
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.
Attachment #532135 - Flags: review?(sdwilsh) → review-
Attached patch Patch 5 (obsolete) — — Splinter Review 
- Addressed review comments.
- Figured out the tests now. So added some tests.
- Removed some code which is no longer necessary in CompleteValue().
Attachment #532135 - Attachment is obsolete: true
Attachment #532890 - Flags: review?(sdwilsh)
Attachment #532890 - Flags: review?(mak77)
Attachment #532135 - Flags: review?(mak77)
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
Attachment #532890 - Flags: review?(mak77) → feedback+
(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.
Attached patch Patch 6 (obsolete) — — Splinter Review 
Fix Marco's comments.
Attachment #532890 - Attachment is obsolete: true
Attachment #533156 - Flags: review?(sdwilsh)
Attachment #532890 - Flags: review?(sdwilsh)
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?
Attachment #533156 - Flags: review?(sdwilsh) → review-
(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.
Attached patch Patch 7 (obsolete) — — Splinter Review 
(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.
Attachment #533156 - Attachment is obsolete: true
Attachment #533516 - Flags: superreview?(gavin.sharp)
Attachment #533516 - Flags: review?(sdwilsh)
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.
Attachment #533516 - Flags: superreview?(gavin.sharp) → superreview+
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).
Attachment #533516 - Flags: review?(sdwilsh) → review+
(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.
Attached patch Patch 8 (obsolete) — — Splinter Review 
Fixed comments.
Attachment #533516 - Attachment is obsolete: true
(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.
http://hg.mozilla.org/mozilla-central/rev/518d679ef81f
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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.
tracking-firefox6: --- → ?
Flags: in-testsuite+
Target Milestone: --- → Firefox 6
Depends on: 659332
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. :)
Depends on: 659445
Depends on: 659451
Keywords: ux-efficiency
Depends on: 659672
(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
Status: RESOLVED → VERIFIED
(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?
Depends on: 659945
(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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
tracking-firefox6: + → ---
Depends on: 660156
(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?
tracking-firefox6: --- → +
Depends on: 660592
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.
Attachment #534664 - Attachment is obsolete: true
Attachment #544689 - Flags: review?(mak77)
Attached patch Part 2 - Add domains to a temporary table (obsolete) — — Splinter Review 
This will fill a temporary table at startup (on a separate thread of course), and autocomplete all domains synchronously.
Attachment #544717 - Flags: review?(mak77)
Attached patch Part 3 - Autocomplete subdirectories (obsolete) — — Splinter Review 
After you type in a domain, this will autocomplete subdirectories using the same autocomplete algorithm as the awesomebar.
Attachment #544718 - Flags: review?(mak77)
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?
Attached patch Part 2 Patch 2 (obsolete) — — Splinter Review 
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.
Attachment #544717 - Attachment is obsolete: true
Attachment #544718 - Attachment is obsolete: true
Attachment #545138 - Flags: review?(mak77)
Attachment #544717 - Flags: review?(mak77)
Attachment #544718 - Flags: review?(mak77)
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.
Whiteboard: [needs-sr]
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
Attachment #544689 - Flags: review?(mak77) → review-
(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
Attachment #545138 - Flags: review?(mak77) → review-
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.
Attached image 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.
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?
Attached patch Patch 3 (obsolete) — — Splinter Review 
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.
Attachment #544689 - Attachment is obsolete: true
Attachment #545138 - Attachment is obsolete: true
Attachment #555982 - Flags: review?(mak77)
Target Milestone: Firefox 6 → ---
Whiteboard: [needs-sr]
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)
Attached patch Patch 4 (obsolete) — — Splinter Review 
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.
Attachment #555982 - Attachment is obsolete: true
Attachment #558237 - Flags: review?(mak77)
Attachment #555982 - Flags: review?(mak77)
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 :(
Attachment #558237 - Flags: review?(mak77)
(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
Attached patch Patch 5 (obsolete) — — Splinter Review 
Feedback seems to be relatively few, and what feedback I've received is good. So it looks like this is ready to go.
Attachment #558237 - Attachment is obsolete: true
Attachment #566431 - Flags: review?(mak77)
Attached patch Patch 5 tests (obsolete) — — Splinter Review 
These tests come from some of the patch from the other bug you mentioned, Marco.
Attachment #566435 - Flags: review?(mak77)
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.
Attachment #566435 - Attachment is obsolete: true
Attachment #566435 - Flags: review?(mak77)
Attached patch Patch 5 (obsolete) — — Splinter Review 
This updates to tip, and includes the test changes since they are very small now.
Attachment #566431 - Attachment is obsolete: true
Attachment #566481 - Flags: review?(mak77)
Attachment #566431 - Flags: review?(mak77)
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.
Attachment #566481 - Flags: review?(mak77)
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.
Blocks: 678352
(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.
Attached patch Patch 6 (obsolete) — — Splinter Review 
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.
Attachment #566481 - Attachment is obsolete: true
Attachment #571584 - Flags: review?(mak77)
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.
Depends on: 699751
Attached patch Patch 7 (obsolete) — — Splinter Review 
Now without the schema changes put out into a new bug.
Attachment #571584 - Attachment is obsolete: true
Attachment #571930 - Flags: review?(mak77)
Attachment #571584 - Flags: review?(mak77)
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
Attachment #571930 - Flags: review?(mak77) → review-
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.
Attachment #571930 - Flags: review-
(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.
Attached patch Patch 8 (obsolete) — — Splinter Review 
Addressed comments by sdwilsh and mak I hope:)
Attachment #571930 - Attachment is obsolete: true
Attachment #578095 - Flags: review?(mak77)
I'll wait for bug 699751 before going through this, in the meanwhile if you have available time for that, this needs some kind of test, similar to the tests we have in toolkit/components/places/tests/autocomplete (maybe not that many, at least the basic functionality should be covered and we can improve in future).
It should go into a separate patch please.
This is one of the most obvious things to fix to improve perceived performance, and it shows up time and again in user interviews as something people expect — all other browsers have it now. It makes us looks slower than we are by requiring more keystrokes to get you to where you're going.
Whiteboard: [snappy]
Whiteboard: [snappy] → [snappy:p1]
Comment on attachment 578095 [details] [diff] [review]
Patch 8

Review of attachment 578095 [details] [diff] [review]:
-----------------------------------------------------------------

Not that far from the target, still the pref-ability to fix (thus the r-), but you handled most of the previous comments, good work David!

Now, this needs tests (please please not in this patch, patch or file apart), but if we can do some manual testing it may land and we could build automated tests a bit more lazily. Having the possibility to disable this easily helps, as well as the fact old autofill is pretty much bogus.
Did you have a chance to do some testing of the feature?

::: toolkit/components/places/Database.cpp
@@ +1366,2 @@
>                    "DROP TABLE IF EXISTS moz_hostnames"));
>    NS_ENSURE_SUCCESS(rv, rv);

this change is in the wrong patch, should be in bug 699751, if it's still applicable.

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +471,5 @@
>    startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>                                          aPreviousResult, aListener)
>    {
> +    // TODO: add an accurate comment about why we call this.stopSearch()
> +    this.stopSearch();

use "// Stop the search in case the controller has not taken care of it.". Btw the controller is rocket science :)

@@ +481,5 @@
> +    let timerCallback = (function() {
> +      this._doStartSearch(aSearchString, aSearchParam,
> +                          aPreviousResult, aListener);
> +    }).bind(this);
> +    this._startTimer.initWithCallback(timerCallback, 50, Ci.nsITimer.TYPE_ONE_SHOT);

move the magic 50 number to a const at the top please, use the const name both here and in the comment.

@@ +1303,5 @@
> +    );
> +  });
> +
> +  // register observers
> +  Services.obs.addObserver(this, kTopicShutdown, false);

We should read and start observing the browser.urlbar.autofill pref here.

When the pref changes to false we should cancel an eventual pendingStmt, finalize statements and close the db connection, then null the internal getters value so that they can be reused (for example setting this.__db = null will allow reuse of the this._db getter).
This means we can't rely on the above lazy getters for statements, we need old fashioned getters that we can restart, similar to the _db one.

@@ +1314,5 @@
> +  startSearch: function UIC_startSearch(aSearchString, aSearchParam,
> +                                        aPreviousResult, aListener)
> +  {
> +    if (this._pendingQuery) {
> +      this.stopSearch();

copy here the comment from the other startSearch in the component.

@@ +1315,5 @@
> +                                        aPreviousResult, aListener)
> +  {
> +    if (this._pendingQuery) {
> +      this.stopSearch();
> +    }

If browser.urlbar.autofill is false should bail out now (we handle an eventual pending query before)

@@ +1460,5 @@
> +        if (Object.getOwnPropertyDescriptor(this, stmts[i]).value !== undefined) {
> +          this[stmts[i]].finalize();
> +        }
> +      }
> +    }

this should first check this.__db since could not have been inited, and if it's non null it should .asyncClose() it and set it to null.
you won't be able to use getOwnPropertyDescriptor once you move to old style getters, but you can use the internal property instead of the getter.

@@ +1468,5 @@
> +  //// urlInlineComplete
> +
> +  __db: null,
> +
> +  get _db() {

remove newline
methods (And getters) brace goes on new line
Attachment #578095 - Flags: review?(mak77) → review-
(In reply to Marco Bonardo [:mak] from comment #205)
> Comment on attachment 578095 [details] [diff] [review]
> @@ +1303,5 @@
> > +    );
> > +  });
> > +
> > +  // register observers
> > +  Services.obs.addObserver(this, kTopicShutdown, false);
> 
> We should read and start observing the browser.urlbar.autofill pref here.
> 

it looks like you have some extra fancy pref handling here. After reading the code a few times I am unsure how to use it. Can you provide an example?
Attached patch Patch 9 (obsolete) — — Splinter Review 
Mak: let me know if I am on the right track here.
Attachment #578095 - Attachment is obsolete: true
Attachment #581815 - Flags: feedback?(mak77)
Comment on attachment 581815 [details] [diff] [review]
Patch 9

Review of attachment 581815 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +66,5 @@
>  
>  // observer topics
>  const kTopicShutdown = "places-shutdown";
>  const kPrefChanged = "nsPref:changed";
> +const kTopicAutofill = "browser.urlbar.autofill";

this is not a topic, it's a preference. in the component there is already
const kBrowserUrlbarBranch = "browser.urlbar.";
so below it you may add a 
const kBrowserUrlbarAutofillPref = "browser.urlbar.autofill";
Since it's a single pref we won't observe the whole branch.

@@ +95,5 @@
>  const kQueryTypeKeyword = 0;
>  const kQueryTypeFiltered = 1;
>  
> +// Autocomplete minimum time before query is executed
> +const kQueryWaitTime = 50;

kAsyncQueriesWaitTime is less generic. The synchronous inline query does not wait.

@@ +800,4 @@
>      this._listener.onSearchResult(this, result);
>      if (this._telemetryStartTime) {
>        let elapsed = Date.now() - this._telemetryStartTime;
> +      if (elapsed > kQueryWaitTime) {

This should stay 50, it's not related to the other const (another good reason to not use magic numbers :( )

@@ +841,5 @@
>          return aDefault;
>        }
>      }
>  
> +    this._autofill = safeGetter("browser.urlbar.autofill", true);

you are on the good way, but in the wrong component.
We are working on urlInlineAutocomplete, not on nsPlacesAutocomplete.

So you actually hade to duplicate (but in a largely simpler way since you want a single pref and not the whole branch) the prefs handling code to that component, and remove these changes from here.

@@ +1309,5 @@
> +  });
> +
> +  // register observers
> +  Services.obs.addObserver(this, kTopicShutdown, false);
> +  Services.obs.addObserver(this, kTopicAutofill, false);

this doesn't work, you have to add an observer through Services.prefs.addObserver, see what nsPlacesAutocomplete does, but instead of observing the whole branch observe only the single pref you care about.

@@ +1468,5 @@
> +          this.__db.asyncClose();
> +          // XXX: not sure how to null it if it is doing an "asyncClose"
> +        }
> +        if (Object.getOwnPropertyDescriptor(this, stmts[i]).value !== undefined) {
> +          this[stmts[i]].finalize();

you cannot close the database before finalizing its statements and surely not inside this for loop. move it after.
once you have invoked asyncClose() you can assign null to __db, there should be no problem in doing that.

@@ +1472,5 @@
> +          this[stmts[i]].finalize();
> +        }
> +      }
> +    }
> +    else if (aTopic == "nsPref:changed" && aData == kTopicAutofill) {

we already have a const kPrefChanged = "nsPref:changed"
and if you correctly add the observer, there's no need to check aData

@@ +1474,5 @@
> +      }
> +    }
> +    else if (aTopic == "nsPref:changed" && aData == kTopicAutofill) {
> +      if (!this._autofill) {
> +        this.stopSearch();

you should first read the new pref value.

@@ +1488,5 @@
> +            this[stmts[i]].finalize();
> +          }
> +        }
> +        // null out the __db object
> +        this.__db = null;

since we do this both here and on shutdown, is surely better to have an helper method like _closeDatabase.
This is missing .asyncClose() as well.
Attachment #581815 - Flags: feedback?(mak77)
Attached patch Patch 10 (obsolete) — — Splinter Review 
Mak: please test this - i thin kit is pretty snappy with my copied over places.sqlite
Attachment #581815 - Attachment is obsolete: true
Attachment #582033 - Flags: review?(mak77)
Comment on attachment 582033 [details] [diff] [review]
Patch 10

Review of attachment 582033 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +102,5 @@
>  // "comment" back into the title and the tag.
>  const kTitleTagsSeparator = " \u2013 ";
>  
>  const kBrowserUrlbarBranch = "browser.urlbar.";
> +const kBrowserUrlbarAutofillPref = "browser.urlbar.autofill";

casing is messed up, the pref is browser.urlbar.autoFill (uppercase F)

@@ +475,5 @@
>    startSearch: function PAC_startSearch(aSearchString, aSearchParam,
>                                          aPreviousResult, aListener)
>    {
> +    // Stop the search in case the controller has not taken care of it.
> +    this.stopSearch();

This should still bail out if (!this._autofill)

@@ +1312,5 @@
> +  // load preferences
> +  this._prefs = Cc["@mozilla.org/preferences-service;1"].
> +                getService(Ci.nsIPrefService).
> +                getBranch(kBrowserUrlbarBranch);
> +  this._loadPrefs(true);

So, I'd like to simplify this a bit, since we only care about a single pref for now.
Don't cache _prefs, we'll just use the global Services.prefs

@@ +1444,5 @@
> +      }
> +      catch (e) {
> +        return aDefault;
> +      }
> +    }

Since this exists in the other _loadPrefs too, may be worth to just move it to a global function and pass the prefbranch to it:
function safePrefGetter(aPrefBranch, aName, aDefault)

@@ +1446,5 @@
> +        return aDefault;
> +      }
> +    }
> +
> +    this._autofill = safeGetter("autofill", true);

and here you'll safePrefGetter(Services.prefs, kBrowserUrlbarAutofillPref, true);

(please also fix the old component to use it, the difference is that it will instead pass this._prefs as the branch)

@@ +1451,5 @@
> +
> +    // register observer
> +    if (aRegisterObserver) {
> +      let pb = this._prefs.QueryInterface(Ci.nsIPrefBranch2);
> +      pb.addObserver("", this, false);

Services.prefs.addObserver(kBrowserUrlbarAutofillPref, this, true);
To use the weak reference you'll have to add Ci.nsISupportsWeakReference) to generateQI

@@ +1529,5 @@
> +          if (Object.getOwnPropertyDescriptor(this, stmts[i]).value !== undefined) {
> +            this[stmts[i]].finalize();
> +          }
> +        }
> +        this._closeDatabase();

closeDatabase should also include the code to finalize statements, since it's the same exact code...
Attachment #582033 - Flags: review?(mak77) → review-
Attached patch Patch 10 (obsolete) — — Splinter Review 
It is exhilarating working on patches that get review right away!
Attachment #582033 - Attachment is obsolete: true
Attachment #582075 - Flags: review?(mak77)
Comment on attachment 582075 [details] [diff] [review]
Patch 10

Review of attachment 582075 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +178,5 @@
> +  return gTextURIService.unEscapeURIForUI("UTF-8", uri);
> +}
> +
> +/**
> + * safePrefGetter get the pref with type "saftey"

typo saftey.
Btw would prefer if you'd explain what safety means, thus that if the pref does not exist the provided default value is returned instead.

@@ +181,5 @@
> +/**
> + * safePrefGetter get the pref with type "saftey"
> + *
> + * @param aPrefBranch
> + *        nsIPrefBranch

The nsIPrefBranch2 containing the required preference.

@@ +186,5 @@
> + * @param aName
> + *        A preference name
> + * @param aDefault
> + *        The preference's default value
> + * @returns (a pref value)

@return the preference value or provided default
(@returns is wrong, afaik)

@@ +201,5 @@
> +    throw "Unknown type!";
> +  }
> +  // If the pref isn't set, we want to use the default.
> +  try {
> +    return self._prefs["get" + type + "Pref"](aName);

This can't clearly work since self does not exist, s/self._prefs/aPrefBranch/

@@ +512,1 @@
>      }

Hm, here you should not check _autofill, it's still the wrong component, the only component that cares about _autofill is urlInlineComplete. Maybe I commented the wrong line before?
Please restore this.

@@ +883,2 @@
>                   Ci.mozIPlacesAutoComplete.BEHAVIOR_HISTORY |
>                   Ci.mozIPlacesAutoComplete.BEHAVIOR_TYPED);

bogus indentation

@@ +1327,5 @@
> +    +                          ":matchBehavior, :searchBehavior) "
> +    + "ORDER BY h.frecency DESC, h.id DESC "
> +    + "LIMIT 1"
> +    );
> +  });

change these to old style getters (__syncQuery and get _syncQuery()), like _db to be clear.

@@ +1329,5 @@
> +    + "LIMIT 1"
> +    );
> +  });
> +
> +  // register observers

this._loadPrefs(true); before

@@ +1340,5 @@
> +
> +  startSearch: function UIC_startSearch(aSearchString, aSearchParam,
> +                                        aPreviousResult, aListener)
> +  {
> +    // Stop the search in case the controller has not taken care of it.

before anything here
if (!this._autofill) {
  return;
}

@@ +1446,5 @@
> +                                    kBrowserUrlbarAutofillPref,
> +                                    true);
> +
> +    // register observer
> +    if (aRegisterObserver) {

I think the comment is pretty much useless considered the immediately following line.

@@ +1448,5 @@
> +
> +    // register observer
> +    if (aRegisterObserver) {
> +      let pb = this._prefs.QueryInterface(Ci.nsIPrefBranch2);
> +      pb.addObserver("", this, false);

nope, Services.prefs.addObserver(kBrowserUrlbarAutofillPref, this, true);

@@ +1513,5 @@
> +          if (Object.getOwnPropertyDescriptor(this, stmts[i]).value !== undefined) {
> +            this[stmts[i]].finalize();
> +          }
> +        }
> +        this._closeDatabase();

why are you still finalizing statements here, is not _closeDatabase already doing that?

@@ +1522,5 @@
> +  /**
> +   *
> +   * Finalize and close the database safely
> +   *
> +   **/

/**
 * Finalize statements and close the database safely.
 */

@@ +1536,5 @@
> +      // see if it is a getter first.
> +      if (Object.getOwnPropertyDescriptor(this, stmts[i]).value !== undefined) {
> +        this[stmts[i]].finalize();
> +      }
> +    }

ok, you can't do this anymore, instead will have to check __syncQuery and __asyncQuery, finalize() and null them. So we can restart them.

@@ +1545,5 @@
> +  },
> +
> +  //////////////////////////////////////////////////////////////////////////////
> +  //// urlInlineComplete
> +  __db: null,

add newline after the "comment header"
Attachment #582075 - Flags: review?(mak77) → review-
Attached patch Patch 11 (obsolete) — — Splinter Review 

        Attachment #582117 -
        Flags: review?(mak77)

    
  

        Attachment #582075 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 582117 [details] [diff] [review]
Patch 11

Review of attachment 582117 [details] [diff] [review]:
-----------------------------------------------------------------

So, this is a 99%r+, in the sense it's an r+ but I want to do some final testing and checks with all the patches, before a final r+, good work!

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +513,2 @@
>  
> +    // Unlike urlInlineComplete, we don't want this search to start

too many newlines before? (I see 2)

@@ +1505,5 @@
> +    // handleCompletion() will cause the result listener to be called, and
> +    // will display the result in the UI.
> +  }
> +,
> +  handleError: function UIC_handleError(aError)

looks like you touched that comma

@@ +1550,5 @@
> +    for (let i = 0; i < stmts.length; i++) {
> +      // We do not want to create any query we haven't already created, so
> +      // see if it is a getter first.
> +      if (this[stmts[i]]) {
> +        this[stmts[i]].finalize();

you forgot to null the statement after finalize(), so the getter will now return a finalized statement instead of a new one :)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 12 (obsolete)
        — 
          — Splinter Review
      

    


  
Ok, fixed those little gaffs. Let me know if you need some testing help. Do we need a bug filed for any additional automated tests? If so, can you describe the what the tests should do?

        Attachment #582117 -
        Attachment is obsolete: true

        Attachment #582172 -
        Flags: review+

        Attachment #582117 -
        Flags: review?(mak77)

    
    

    
  
A couple issues to fix still, not related to this patch but to the global functionality:
- trimURL conflicts with autocomplete, typing http://, as soon as the second / is typed, replaces text with some unpredictable result (the most frecent host selected from the 8th char on)
- Autocomplete controller asserts in processResults, at oldMatchCount = mMatchCounts[aSearchIndex]; the mMatchCounts has length 1, but aSearchIndex is 1.

    
  
Depends on: 711503

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 13 (obsolete)
        — 
          — Splinter Review
      

    


  
solves a couple minor bugs like kPrefChanged inside a string, may also solve the assertion by properly handling the disabling bailout.

sending the whole thing to Try.

        Attachment #582172 -
        Attachment is obsolete: true

        Attachment #582307 -
        Flags: review+

    
    

    
  
try builds.
BEWARE: DO NOT USE YOUR DEFAULT PROFILE, copy places.sqlite to a new test profile.
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-d2ee9250eb7e/

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch 14
        — 
          — Splinter Review
      

    


  
minor change relative to empty queries, found by limi while testing interaction on the trybuild

        Attachment #582307 -
        Attachment is obsolete: true

    
    

    
  
I tried the try build and it seems to work ok and better than how it worked before.I hope you land this soon :) Good work.

    
    

    
  
This is done by Internet Explorer, Safari, Chrome, and Opera.
Firefox is the only one not doing it, even with browser.urlbar.autoFill=true (as of Firefox 9.0.1)

    
  
Assignee: ventnor.bugzilla → ddahl

    
    

    
  
Current status here... this bug's patch is r+ - just waiting on the review for bug 711503

    
    

    
  
David, could you please file a bug for the automated tests, and eventually start looking at those?

    
  
Blocks: 715133

    
    

    
  
(In reply to Marco Bonardo [:mak] from comment #224)
> David, could you please file a bug for the automated tests, and eventually
> start looking at those?

Filed bug 715133

    
    

    
  
Drive-by security comment: please make sure this doesn't modify *pasted* URLs.

    
    

    
  
(In reply to David Dahl :ddahl from comment #225)
> (In reply to Marco Bonardo [:mak] from comment #224)
> > David, could you please file a bug for the automated tests, and eventually
> > start looking at those?
> 
> Filed bug 715133

Are these required for landing? If so, why are these being split out?

    
    

    
  
(In reply to Dietrich Ayala (:dietrich) from comment #227)
> Are these required for landing? If so, why are these being split out?

They are required for the feature to stay enabled, filed apart since the idea was to reach central with manual testing and have them ready before taking a decision. These tests are not critical since the backend part providing data already has tests, so the missing ones are mostly checking we don't return empty or random stuff (mostly regressions safety).
Actually, I hope David has some news regarding those.

    
  
Depends on: 717772

    
    

    
  
(In reply to Marco Bonardo [:mak] from comment #228)
> Actually, I hope David has some news regarding those.
I plan on starting these tests soon.

    
    

    
  
Thanks everyone for the cooperation, this now hits inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/953bde82b7a7
Target Milestone: --- → Firefox 12

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/953bde82b7a7
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED

    
  
Blocks: 597009, 659332, 659445, 659451, 659672
No longer depends on: 597009, 659332, 659445, 659451, 659672
Depends on: 719890

    
  
Depends on: 719888

    
    

    
  
Requiring tracking since this is a new feature in 12 that we may eventually disable in Aurora through a pref if it should not meet our quality criteria.

          tracking-firefox12:
          --- → ?

    
    

    
  
Not sure if it's a bug but let me show you..
take a look in this pic:
http://img171.imageshack.us/img171/4296/exampleed.jpg

this patch throw me to the second option on awesome bar popup..is this what should happen?

I'm using nightly (2012-01-20) x86 in windows 7 x64
I'd set my awesome bar to suggest only bookmarks and not history
Let me know if you need anything else...

    
    

    
  
My awesome bar suggests both, bookmarks and history. And the new behaviour does not make sense for me... Visit count for woltlab.com/forum: 1699, visit count for woltlab.com/: 21. Why does the autocomplete selects woltlab.com/ ?

http://s14.directupload.net/images/120121/dia4hzx3.png

    
    

    
  
(In reply to Sören Hentzschel from comment #234)
> My awesome bar suggests both, bookmarks and history. And the new behaviour
> does not make sense for me... Visit count for woltlab.com/forum: 1699, visit
> count for woltlab.com/: 21. Why does the autocomplete selects woltlab.com/ ?
> 
> http://s14.directupload.net/images/120121/dia4hzx3.png

I think it does make sense , it is just fetching you with the site first , then if you further type foru.. you would get that link as well. Its like i type facebo and get facebook.com even though say i visit a particular profile/page/messages/photos etc more than the actual site. Similarly it will show you bugzilla.mozilla.org first and not the bug number you visit the most. Definitely its not something which should hold its release in my opinion.

    
    

    
  
(In reply to Ivan Regados from comment #233)
> Not sure if it's a bug but let me show you..
> take a look in this pic:
> http://img171.imageshack.us/img171/4296/exampleed.jpg
> 
> this patch throw me to the second option on awesome bar popup..is this what
> should happen?
> 
> I'm using nightly (2012-01-20) x86 in windows 7 x64
> I'd set my awesome bar to suggest only bookmarks and not history
> Let me know if you need anything else...

Why don't you type "n" ahead to get band and eliminate bamboo ? How can browser know what you actually want with mere two characters? I am sure even other browsers will pose such problem , do report if they do not.

    
    

    
  
bogas04, the problem is:

The old behaviour:
I type "w", cursor down, enter.

Or easier, with the Enter Selects extension:
I type "w", enter

The new behaviour, in both cases:

I type "w", cursor right, "f", enter.

That's frustrating... And that's a simple example, select other pages is even more complicated now...

I prefer to open the page I want and not another page just because it's the root domain...

    
    

    
  
Addendum (sorry, no edit-function?):

Select a page and show another page as the first result in the awesomebar is inconsistent! I would expect the same page. That's why I said this does not make sense.

    
    

    
  
Please file a separate bug for follow-up issues and mark it as blocking this bug.  (See, e.g., bug 719890.)

Thanks!

    
    

    
  
(In reply to Sören Hentzschel from comment #237)
> bogas04, the problem is:
> 
> The old behaviour:
> I type "w", cursor down, enter.
> 
> Or easier, with the Enter Selects extension:
> I type "w", enter
> 
> The new behaviour, in both cases:
> 
> I type "w", cursor right, "f", enter.
> 
> That's frustrating... And that's a simple example, select other pages is
> even more complicated now...
> 
> I prefer to open the page I want and not another page just because it's the
> root domain...

Apologies i can't understand the problem clearly but : 


With old behaviour : 
type w > arrow down > woltlab.com/forum (which is at top coz of more page hits)

With new behaviour : 

type w > arrow right > type f > autocompletes to woltlab.com/forum

OR

type w > auto-completed to woltlab.com > arrow down > woltlab.com/forum (which is again at top coz of more page hits) 


Refer to this pic : http://s17.postimage.org/z0ch2id8d/Untitled.png

In fact i feel it saves time for other cases , while for your case the old method STILL WORKS fine provided the extension you are using is not messing up...

    
  
Depends on: 720081

    
    

    
  
Please do not add more comments to this bug to discuss issues which arose from this patch.  It's important that we keep one issue per bug, otherwise the discussion quickly becomes impossible to follow.

Your (legitimate!) issues will not get the attention they deserve unless you file new bugs and mark them as blocking this one.

    
    

    
  
"I prefer to open the page I want and not another page just because it's the root domain..."

The reason for this behaviour is that for many sites that's wanted. Take for example a news site, mostly you want to visit root and not autocomplete to some article.

Of course there are certainly many cases were this is not wanted, like yours. A solution might be to:

Check if a specific part of a URL has a high visit count (100+) and in that case chose the full URL instead of root. That way it would autocomplete to example.com/forum/ in your case but not to example.com/forum/?t=123&p=456

    
  
Depends on: 720066

    
    

    
  
To clarify how inline works, it's unfortunately not related to the first entry in the popup, the result is gathered through the max frecency between _all_ the pages in a certain domain. It's not possible to make it return the first entry in the popup, no performant way, for how the awesome bar works, especially the adaptive behavior.
The scope for it is to make it complete to uris, not titles tags or other, pure uris. It can be disabled with browser.urlbar.autoFill preference.

That said, any further comment in this bug will be ignored, because it's resolved.
Comment 241 is right, File your bug.

    
    

    
  
"Enable inline autocomplete again, but make it smarter" actually made it dumber: https://bugzilla.mozilla.org/show_bug.cgi?id=720240

    
  
Depends on: 720240
Depends on: 720110

    
  
Depends on: 720266

    
  
Depends on: 720284

    
  
Depends on: 720293

    
  
Depends on: 720501

    
    

    
  
Autocomplete breaks Omnibar addon.When autocomplete is enabled anything typed in the urlbar that does not trigger an autocomplete match gets deleted as soon as you stop typing.

STR
1.type "aaaaaaaaaaaaa" by holding the key or any other word that doesn't trigger an autocomplete.
2.release the key

Expected result:the typed text remains.

Actual results:text gets auto deleted

Best way to test it is on a new profile with clean history.On new profile the timeout before it deletes the text is almost none and it deletes characters even while im typing.I can't even type a whole word.Turning off autocomplete or the addon fixes the issue.

    
    

    
  
(In reply to nasko_naskov from comment #245)
> Autocomplete breaks Omnibar addon.

bug 720131. Please don't report bugs in resolved bugs. Thanks.

    
  
Depends on: 720792

    
  
Depends on: 721225

    
  
Depends on: 721338

    
  
No longer blocks: 715133
Depends on: 715133

    
  
Depends on: 722176

    
    

    
  
This feature was temporarily disabled in bug 720792.

    
    

    
  
and will stay disabled in FF12, will be re-enabled in Nightly once that bug is fixed.

    
  
Depends on: 735187

    
    

    
  
Hi !
Actually running Nightly 14.0a1 (2012-03-26) and «search keywords» are still ignored by inline autocomplete (IMO typing a keyword like g→google,d→ddg,... shouldn't bring up autocomplete as it prevents typing the keyword alone what could be used to go directly to the search site)

Should I open a bugreport for this ?

    
    

    
  
(In reply to Romain DEP. from comment #249)
> Hi !
> Actually running Nightly 14.0a1 (2012-03-26) and «search keywords» are still
> ignored by inline autocomplete (IMO typing a keyword like g→google,d→ddg,...
> shouldn't bring up autocomplete as it prevents typing the keyword alone what
> could be used to go directly to the search site)

keywords are already excluded, what do you mean by "bring up autocomplete"? The popup is expected, what's not expected is inline result when you have completely typed the keyword. like I have a "bug" keyword for bugzilla, and if I type "bug" I don't see any inline result.

    
    

    
  
(In reply to Marco Bonardo [:mak] from comment #250)
> (In reply to Romain DEP. from comment #249)
> > Hi !
> > Actually running Nightly 14.0a1 (2012-03-26) and «search keywords» are still
> > ignored by inline autocomplete (IMO typing a keyword like g→google,d→ddg,...
> > shouldn't bring up autocomplete as it prevents typing the keyword alone what
> > could be used to go directly to the search site)
> 
> keywords are already excluded, what do you mean by "bring up autocomplete"?
> The popup is expected, what's not expected is inline result when you have
> completely typed the keyword. like I have a "bug" keyword for bugzilla, and
> if I type "bug" I don't see any inline result.

That's not what I get : for example I have a «d» shortcut which points to duckduckgo. I'm used to press «d»+Enter to be redirected to http://duckduckgo.com/
Now when I press «d» the entry field gets filled with http://diasp.org/ and if I don't pay enough attention I finish on the wrong page :)

    
    

    
  
ah you mean search bar keywords, yeah you should probably file that bug.

    
    

    
  
(In reply to Marco Bonardo [:mak] from comment #252)
> ah you mean search bar keywords, yeah you should probably file that bug.

I just submitted 739580. Thanks !

    
  
Depends on: 739580

    
    

    
  
I'd like to call attention to bug 739267, which is likely a dupe of one of the related bugs blocking this one.
Depends on: 739267

    
  
Depends on: 739751
No longer depends on: 740076

    
    

    
  
This bug is slightly misleading - we initially enabled inline autocomplete in Firefox 12, but then backed it out due to some bugs, and only re-enabled it again in Firefox 13 (see bug 735187).

    
  
Depends on: 742776

    
    

    
  
Shouldn't this be REOPENED until all its dependencies are RESOLVED?
Depends on: 448486

    
    

    
  
(In reply to alex_mayorga from comment #256)
> Shouldn't this be REOPENED until all its dependencies are RESOLVED?

Our convention is that a bug can be fixed even if some of its dependencies are unresolved.  In particular, when a bug causes new bugs (regressions or otherwise), we often mark the new bugs as blocking the original bug.  But that doesn't mean the original bug is somehow not fixed, by virtue of having caused new bugs!

In this case, leaving this bug fixed is helpful because it makes it clear that inline auto-complete *is* enabled in trunk builds, even if there's work yet to be done.

    
  
Whiteboard: [snappy:p1] → [snappy:p1][qa+]

    
  
Depends on: 746572

    
  
Whiteboard: [snappy:p1][qa+] → [snappy:p1][qa+][release tracking in bug 746572]

    
    

    
  
Having Prospector Speak Words could cause instability?

    
    

    
  
(In reply to Daniel "warmth" Delgado from comment #258)
> Having Prospector Speak Words could cause instability?

Yes it will, and also the Omnibar extension, only "Auto Complete" extension will not break as it disables the pref itself and enables it again when you uninstall.

That said. Please don't report on a fixed bug. Instead file a new bug if you find a need to and make it blocking this one.
Depends on: 762453

    
    

    
  
Sorry but I have to write that.
Thank you a lot for this feature in Firefox 14! I missed this feature.

    
  
Blocks: 767364

    
    

    
  
I HATE YOU!!!!!!!!
the one feature of chrome that made me move to ffox...

    
    

    
  
(In reply to jimmyli1528 from comment #261)
> I HATE YOU!!!!!!!!
> the one feature of chrome that made me move to ffox...

That comment is not appropriate.  Unlike Chrome you can turn it off by turning off the browser.urlbar.autocomplete.enabled setting in about:config.

    
    

    
  
(In reply to jimmyli1528 from comment #261)

Please read the etiquette guidelines: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

That type of comment is not acceptable here.

    
    

    
  
(In reply to Jeff Grossman from comment #262)
> That comment is not appropriate.  Unlike Chrome you can turn it off by
> turning off the browser.urlbar.autocomplete.enabled setting in about:config.

The correct pref is browser.urlbar.autoFill

    
    

    
  
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0

Verified that URL autocomplete is enabled in firefox 14 (beta 11).
Status: RESOLVED → VERIFIED

          status-firefox14:
          fixed → verified
Whiteboard: [snappy:p1][qa+][release tracking in bug 746572] → [snappy:p1][qa!][release tracking in bug 746572]

    
    

    
  
This inline auto-complete feature is really annoying me, guys!
Because I use the awesome bar to search in Google A LOT! And now, every time I want to search I have to press Backspace before hitting enter.
Please add an option in About:config to disable the inline auto-complete. (I am completely happy with the auto-complete feature itself. It's only the inline auto-complete feature that makes me mad!)

Thanks a lot :)

    
    

    
  
(In reply to Zeta from comment #266)
> This inline auto-complete feature is really annoying me, guys!
> Because I use the awesome bar to search in Google A LOT! And now, every time
> I want to search I have to press Backspace before hitting enter.
> Please add an option in About:config to disable the inline auto-complete. (I
> am completely happy with the auto-complete feature itself. It's only the
> inline auto-complete feature that makes me mad!)
> 
> Thanks a lot :)

turn browser.urlbar.autoFill to false.

    
    

    
  
(In reply to Girish Sharma [:Optimizer] from comment #267)
> (In reply to Zeta from comment #266)
> > This inline auto-complete feature is really annoying me, guys!
> > Because I use the awesome bar to search in Google A LOT! And now, every time
> > I want to search I have to press Backspace before hitting enter.
> > Please add an option in About:config to disable the inline auto-complete. (I
> > am completely happy with the auto-complete feature itself. It's only the
> > inline auto-complete feature that makes me mad!)
> > 
> > Thanks a lot :)
> 
> turn browser.urlbar.autoFill to false.

Thanks Girish! As J. Gatellier mentioned above!
(I really should've seen that before commenting!)
Thanks again

    
    

    
  
I wish auto-complete was smarter...
I write a few times per day this 2 sites:
mail.yahoo.com
AND
gmail.com

When I start typing "mail" it gives me "mail.google.com" while:
1. I never even wrote this link.

2. It could learn that when I start typing that I usually(in my case actually ALWAYS) select "mail.yahoo.com"(I understand that would mean additional operations and make the search for responses slower, so at least first one could be implemented).

    
    

    
  
(In reply to Moraru Lilian from comment #269)
> I wish auto-complete was smarter...
> I write a few times per day this 2 sites:
> mail.yahoo.com
> AND
> gmail.com
> 
> When I start typing "mail" it gives me "mail.google.com" while:
> 1. I never even wrote this link.
> 
That is because gmail.com always re-directs to mail.google.com

> 2. It could learn that when I start typing that I usually(in my case
> actually ALWAYS) select "mail.yahoo.com"(I understand that would mean
> additional operations and make the search for responses slower, so at least
> first one could be implemented).

You should set up a bookmark keyword to handle going to your email: http://johnbokma.com/firefox/keymarks-explained.html

    
    

    
  
(In reply to David Dahl :ddahl from comment #270)
> (In reply to Moraru Lilian from comment #269)
> > I wish auto-complete was smarter...
> > I write a few times per day this 2 sites:
> > mail.yahoo.com
> > AND
> > gmail.com
> > 
> > When I start typing "mail" it gives me "mail.google.com" while:
> > 1. I never even wrote this link.
> > 
> That is because gmail.com always re-directs to mail.google.com
> 
> > 2. It could learn that when I start typing that I usually(in my case
> > actually ALWAYS) select "mail.yahoo.com"(I understand that would mean
> > additional operations and make the search for responses slower, so at least
> > first one could be implemented).
> 
> You should set up a bookmark keyword to handle going to your email:
> http://johnbokma.com/firefox/keymarks-explained.html

David, I think Moraru's complaint is a valid one (if reproducible). I thought bug 720258 was meant to fix that issue? Telling people to use an obscure feature which needs setting up for each site that this issue affects is not the best response.

    
    

    
  
(In reply to Daniel Cater from comment #271)

> David, I think Moraru's complaint is a valid one (if reproducible). I
> thought bug 720258 was meant to fix that issue? Telling people to use an
> obscure feature which needs setting up for each site that this issue affects
> is not the best response.

Perhaps so. I never said it was not a valid complaint. Telling a user about a very handy feature is not a disservice to him in any way. It is better than "wait till the patch lands who knows when".

    
    

    
  
This feature seems to have caused a lot of bugs in navigating to URLs. For pages where you may wish to visit both the http:// version and the https:// version (because it sadly is not safe to assume the two are the same), this feature makes it very difficult to navigate to one when the other is in your history. It also causes some things to default to https:// which did not before. This has caused a lot of confusion among people I know who use Firefox.

    
    

    
  
It is a good thing that it defaults to https:// , why would someone want the other way? There is almost no overhead to using the secure one...

    
    

    
  
(In reply to David Benjamin from comment #273)
> This feature seems to have caused a lot of bugs in navigating to URLs. For
> pages where you may wish to visit both the http:// version and the https://
> version (because it sadly is not safe to assume the two are the same), this
> feature makes it very difficult to navigate to one when the other is in your
> history. It also causes some things to default to https:// which did not
> before. This has caused a lot of confusion among people I know who use
> Firefox.

Follow bug 769994, there are 2 scenarios about this issue.

    
    

    
  
(In reply to Moraru Lilian from comment #274)
> It is a good thing that it defaults to https:// , why would someone want the
> other way? There is almost no overhead to using the secure one...

Sometimes there are two different websites on http and https. A bank can have its normal website at http://bank.com and a client private banking application at https://bank.com. And you sometimes want to go to http (to see bank offer) and the other time to https (to log in and check your finance).

    
    

    
  
(In reply to Bartosz Piec from comment #276)
> Sometimes there are two different websites on http and https. A bank can

Correct, and it is not just banks. We have a lot of local servers that express this schizophrenia, with nominally public-facing servers on http:// and internal services on https://.

But it is even worse. At present (e.g. FF14.0.1) it doesn't prefer https. It prefers the first one you type (or so it seems). So if you have previously entered https://foobar, it is tough to enter http://foobar; this is the "bank" case above, and it's annoying.

But similarly annoying is that if you have entered http://barbaz, then it becomes tough to enter https://barbaz. This doesn't have the flase justification associated with it.

This kind of confusion between http and https should only happen when HSTS is enabled. It's part of the whole reason for HSTS as a listand an option, and not just https-always-by-default.

    
    

    
  
This bug is fixed, you should open new bugs and set them block this one.
About this specific issue, there's, as already said by Loic, bug 769994.

    
    

    
  
When I type "r" to go to reddit, it now autocompletes with "rypple" because reddit is actually pay.reddit.com which doesn't start with an r. Has anyone else experienced something like this with subdomains? I'm hesitant to open a new bug because I'm not sure if this was the old behavior with browser.urlbar.autoFill on.

    
    

    
  
(In reply to candylandish from comment #279)
> When I type "r" to go to reddit, it now autocompletes with "rypple" because
> reddit is actually pay.reddit.com which doesn't start with an r. Has anyone
> else experienced something like this with subdomains? I'm hesitant to open a
> new bug because I'm not sure if this was the old behavior with
> browser.urlbar.autoFill on.

Please do. We will not fault anyone for opening new bugs, plus this bug has too many comments now.

    
  
No longer blocks: 767364

    
  
Depends on: 863727

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: