946074 - Awesomebar cursor processing is inefficient

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Richard Newman [:rnewman]]

Bug 943475 Comment 6.

Comment 1

[Richard Newman [:rnewman]]

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

Comment 2

[Richard Newman [:rnewman]]

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

Comment 3

[Richard Newman [:rnewman]]

Attachment: Awesomebar cursor processing is inefficient.

Comment 4

[Richard Newman [:rnewman]]

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!).

Comment 5

[Wesley Johnston (:wesj)]

(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).

Comment 6

[Richard Newman [:rnewman]]

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?

Comment 7

[Wesley Johnston (:wesj)]

(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.

Comment 8

[Richard Newman [:rnewman]]

(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?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(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...

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 12

[Wesley Johnston (:wesj)]

(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.

Comment 13

[Richard Newman [:rnewman]]

(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 14

[Richard Newman [:rnewman]]

Attachment: Awesomebar cursor processing is inefficient.

Comment 15

[Richard Newman [:rnewman]]

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!

Comment 16

[Richard Newman [:rnewman]]

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

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/f118b840cabe

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 22

[Wesley Johnston (:wesj)]

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.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: awesomebar-autocomplete + test change

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

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83bce9e1d37f