Bug 943475 Comment 6.

Also, our autocomplete handling seems really leery of matching results.

Oh, and for "foo.com/bar" we return "foo.com/bar/".

This patch fixes a few minor issues, and avoids a bunch of needless work. It doesn't address every flaw with awesomebar completion -- a feature that kicks in so infrequently that I honestly didn't know we even had it, and I've been working on this product for two years. For example, we don't apparently do substring completion at all (or at least, it doesn't work). I have a tab open: "http://www.reddit.com/r/boop/comments/1s1amu/the_quintessential_boop/". I can type "reddit.co", "reddit.com/r/bo", "reddit.com/r/boop/comments/1s", and none of them autocomplete. In fact, there seems to be no way to autocomplete the entire URL at all. "reddit." autocompletes to "reddit.com", but the moment I hit 'c' it goes away. That's broken (and I mean broken in the current codebase, not with this patch applied!).

(In reply to Richard Newman [:rnewman] from comment #4) > It doesn't address every flaw with awesomebar completion -- a feature that kicks in so infrequently that I honestly > didn't know we even had it, and I've been working on this product for two years. Interesting. I see it frequently, but I tend to visit the same sites a lot. > I have a tab open: > "http://www.reddit.com/r/boop/comments/1s1amu/the_quintessential_boop/". > > I can type "reddit.co", "reddit.com/r/bo", "reddit.com/r/boop/comments/1s", > and none of them autocomplete. In fact, there seems to be no way to > autocomplete the entire URL at all. This works for me. > "reddit." autocompletes to "reddit.com", but the moment I hit 'c' it goes > away. That's broken (and I mean broken in the current codebase, not with > this patch applied!). As does this. It looks like, instead of relying on the Uri class to parse the string, you're doing string manipulation, which is probably good! I'm not sure how it fixes the bugs you listed though (but like I said, I can't reproduce them). It should fix the trailing slash though! Maybe the Uri.parse is failing on your phone for some reason... Some deeper investigation into what's happening would be nice... I can sometimes reproduce some bugs where we over auto-fill (i.e. I type 'r' and get that entire url filled in, not sure why that happens), as well as one where I edit a url and we don't auto-fill (any time 'www' is at the front of my searchTerm).

Thanks for the quick input! (In reply to Wesley Johnston (:wesj) from comment #5) > > I have a tab open: > > "http://www.reddit.com/r/boop/comments/1s1amu/the_quintessential_boop/". > > > > I can type "reddit.co", "reddit.com/r/bo", "reddit.com/r/boop/comments/1s", > > and none of them autocomplete. In fact, there seems to be no way to > > autocomplete the entire URL at all. > > This works for me. On current Nightly? Interesting. (At least this patch shouldn't break anything if it works for you...) > It looks like, instead of relying on the Uri class to parse the string, > you're doing string manipulation, which is probably good! This patch now does both -- it'll match full URI prefixes (if a user types "http..."), prefixes including common subdomains (if a user types "www.reddit..."), and then fall back to a method that includes parsing in order to extract the hostname component, which should be functionally identical to the current code except in handling of trailing slashes. Further work might remove even that parsing, but that's a few steps away. > I'm not sure how it fixes the bugs you listed though (but like I said, I can't reproduce them). This patch doesn't -- see comment "It doesn't address every flaw with awesomebar completion". But it does tidy up and comment the code to a point that it's easier to insert fixes or changes in the right place, and to see why it's doing what it does. > I can sometimes reproduce some bugs where we over auto-fill (i.e. I type 'r' > and get that entire url filled in, not sure why that happens), Certainly a requirement is that the matching site is at the start of the cursor, but yeah, that's weird. There's more to this auto-fill stuff than just the method I'm working on, for sure. > as well as > one where I edit a url and we don't auto-fill (any time 'www' is at the > front of my searchTerm). That's because we only match against the stripped hostname -- and we strip www, so if you type that we'll never match it. This patch fixes that omission. One thing I'd like to do is get some test coverage for this -- I couldn't find any at all. Do you know if there's some hiding in the tree that my search for "utocom" didn't find?

(In reply to Richard Newman [:rnewman] from comment #6) > That's because we only match against the stripped hostname -- and we strip > www, so if you type that we'll never match it. This patch fixes that > omission. Careful here. This was intentional. If I type "w" and the top result in my top sites is "www.google.com" but the third site is "woot.com" we should probably match on woot (i.e. people don't type 'www'. often). That matches desktop's behavior as well.

(In reply to Wesley Johnston (:wesj) from comment #7) > Careful here. This was intentional. If I type "w" and the top result in my > top sites is "www.google.com" but the third site is "woot.com" we should > probably match on woot (i.e. people don't type 'www'. often). That matches > desktop's behavior as well. How about only matching once you've typed a subdomain? So we start matching the full URL when you've typed "www.r". I think that covers both use cases, right?

(In reply to Richard Newman [:rnewman] from comment #8) > (In reply to Wesley Johnston (:wesj) from comment #7) > > > Careful here. This was intentional. If I type "w" and the top result in my > > top sites is "www.google.com" but the third site is "woot.com" we should > > probably match on woot (i.e. people don't type 'www'. often). That matches > > desktop's behavior as well. > > How about only matching once you've typed a subdomain? So we start matching > the full URL when you've typed "www.r". I think that covers both use cases, > right? But then I've had to type 5 characters...

(In reply to Richard Newman [:rnewman] from comment #4) > This patch fixes a few minor issues, and avoids a bunch of needless work. > > It doesn't address every flaw with awesomebar completion -- a feature that > kicks in so infrequently that I honestly didn't know we even had it, and > I've been working on this product for two years. SwiftKey! This feature does not support composition, so you won't see it. Use a different keyboard if you want this feature to work :) > I have a tab open: > "http://www.reddit.com/r/boop/comments/1s1amu/the_quintessential_boop/". > > I can type "reddit.co", "reddit.com/r/bo", "reddit.com/r/boop/comments/1s", > and none of them autocomplete. In fact, there seems to be no way to > autocomplete the entire URL at all. This feature should only ever autocomplete the domain, and never the full URL. That sounds like "switch to tab" and should happen in the result list. Unless you mean, it should automcomplete the domain first, then if you type beyond the domain, it should autocomplete the next chunk of a URL path? If so, then yes. > "reddit." autocompletes to "reddit.com", but the moment I hit 'c' it goes > away. That's broken (and I mean broken in the current codebase, not with > this patch applied!). SwiftKey

Comment on attachment 8342178 [details] [diff] [review] Awesomebar cursor processing is inefficient. I tested this patch and it seems to work OK for me. I have it in my queue with two other patches that touch code in the same area. I can land this with bug 943475.

(In reply to Mark Finkle (:mfinkle) from comment #9) > (In reply to Richard Newman [:rnewman] from comment #8) > > (In reply to Wesley Johnston (:wesj) from comment #7) > > > > > Careful here. This was intentional. If I type "w" and the top result in my > > > top sites is "www.google.com" but the third site is "woot.com" we should > > > probably match on woot (i.e. people don't type 'www'. often). That matches > > > desktop's behavior as well. > > > > How about only matching once you've typed a subdomain? So we start matching > > the full URL when you've typed "www.r". I think that covers both use cases, > > right? > > But then I've had to type 5 characters... That seems to match desktop.

(In reply to Mark Finkle (:mfinkle) from comment #9) > But then I've had to type 5 characters... I'm not saying it's what we *want* users to do... simply that if you're trying to match "www.reddit.com/r/...", we shouldn't force you to type "reddit.com". If you're the kind of user who uses Google's search box to get to Facebook, you're going to type whatever you're used to typing. Alternative phrasing: why should we force users to memorize the list of "common subdomains"? (In reply to Mark Finkle (:mfinkle) from comment #11) > Comment on attachment 8342178 [details] [diff] [review] > Awesomebar cursor processing is inefficient. > > I tested this patch and it seems to work OK for me. I have it in my queue > with two other patches that touch code in the same area. I can land this > with bug 943475. I'd like to make Wes's change first. Let me give you an updated patch.

Comment on attachment 8342744 [details] [diff] [review] Awesomebar cursor processing is inefficient. Amended this bit: + // Check the non-stripped host, if it looks like the user has typed + // a prefix. This avoids us matching a bunch of "www." sites when + // the user types "w", but still allows us to match "www.reddit" + // without forcing the user to omit the "www.". + if (searchTerm.indexOf('.') != -1 && + host.startsWith(searchTerm)) { + return host + "/"; } with carry-forward review. mfinkle, please re-integrate with your patch stack!

Setting needinfo mfinkle so he doesn't forget to land this.

Had to back out because this patch breaks a test https://hg.mozilla.org/integration/fx-team/rev/871fab42fa64 https://tbpl.mozilla.org/php/getParsedLog.php?id=31543257&tree=Fx-Team&full=1 The test does not expect "about:home" to be autocompleted when sending "ab" but now it is.

With a tweak to the test: https://tbpl.mozilla.org/?tree=Try&rev=66a65d10a302

"ab" will match "about:firefox" not "about:home" so I changed from "ab" to "zy" https://tbpl.mozilla.org/?tree=Try&rev=1bcf2d1ea542

(In reply to Mark Finkle (:mfinkle) from comment #20) > "ab" will match "about:firefox" not "about:home" so I changed from "ab" to > "zy" > https://tbpl.mozilla.org/?tree=Try&rev=1bcf2d1ea542 This is green. Wes brought up a question: Are you sure you want us to match "about" urls? It looks like desktop doesn't I guess I would like to know the downside.

I guess just potentially exposing users to things we've tried to keep hidden. We intentionally avoid showing things that aren't bookmarks or in history though, and about pages aren't added to history. So you'd have to have bookmarked one. Its possible you visit about.com occasionally, but it never prefills because our default about:firefox bookmark outweighs it in frecency.

I added a tweak to the broken test. Adding the patch for posterity. Moving r+ forward.

We can make tweaks if the "about:" page completion becomes an issue https://hg.mozilla.org/integration/fx-team/rev/83bce9e1d37f