1363621 - Favicon in the first location bar suggestion flickers when typing url

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

[684sigma]

Attachment: Favicon in the first suggestion flickers, by reporter.webm

While testing Bug 1363620, I have noticed a bug in Firefox Nightly 55 (2017-05-09). It doesn't happen in Beta 53.
Favicon in the first location bar suggestion flickers when typing url.
Here's how to reproduce the bug:

1. Visit google.com
2. Open new tab, type in location bar "google.com/helloworld"

Result: Favicon in the first suggestion flickers
Expected: Favicon should be displayed without flickering

Comment 1

[684sigma]

Mozregression-gui 0.9.6 generated this pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b01f44c896b2472b154a48c68cf343ae2289d6bd&tochange=d80c517658f5048491b7bf27da25246a297ab50b
->
977177 - move favicons blobs out of places.sqlite to their own database
https://bugzilla.mozilla.org/show_bug.cgi?id=977177

Comment 2

[Marco Bonardo [:mak]]

this is similar to bug 1360279, but here you're typing a url. it may be a bit more complex to solve because the url effectively changes. Though, we should know if the url is known or not.
It could be just matter of specifying a fixed icon in _matchUnknownUrl like we did in _processHostRow.

Comment 3

[Marco Bonardo [:mak]]

So, here we have an url that we don't know if it's known or not to history and figuring that out may be expensive. So we don't know if we have an icon for it.
The reason for the flickr is exactly that at every new typed char we try to fetch the icon for the newly typed url.
There are 2 ways we could "fix" this:
1. if the uri has an host, we could fetch the icon for the host. This would only partially solve the flicker since until we start typing the path, we'll still try to fetch an icon at every typed char. The advantage is that we could show some more icons.
2. just always use the globe icon. This won't flicker, but if we have the icon for a specific url it won't be shown.

I'm prone to do 2, to completely avoid the flicker, and it will also be cheaper (since the user is typing that matters). The likely to have one icon out of many is very low, so it should not be terrible from a UI point of view.

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1363621 - Favicon in the first location bar suggestion flickers when typing url.

Review commit: https://reviewboard.mozilla.org/r/142786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142786/

Comment 5

[Florian Quèze [:florian]]

Does this patch cause us to show the globe icon for completions? (eg. when I type 'b' and the first awesomebar item says 'bugzilla.mozilla.org - Visit')

Comment 6

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #5)
> Does this patch cause us to show the globe icon for completions? (eg. when I
> type 'b' and the first awesomebar item says 'bugzilla.mozilla.org - Visit')

it shows the globe if what you are typing is _not_ autofilled and it looks like a url.
It doesn't touch autofilled entries (b[ugzilla.org] or bugzilla.org/s[omething]).

For you example, if we show bugzilla.mozilla.org it means it's autofilled, or we'd show "b - Search with Google".

Comment 7

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #6)

> For you example, if we show bugzilla.mozilla.org it means it's autofilled,
> or we'd show "b - Search with Google".

We autofill inside the input field only after I type "bu", when I only type "b", there's no autofill in the field, but the first awesomebar item offers to visit bugzilla. And actually... it turns out this is because I have 'b' as a keyword for bugzilla search, and pressing enter doesn't send me to the home page, but makes me search for an empty string in bugzilla; that's misleading :-/ (but probably off topic for this bug).

Comment 8

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #7)
> We autofill inside the input field only after I type "bu", when I only type
> "b", there's no autofill in the field, but the first awesomebar item offers
> to visit bugzilla. And actually... it turns out this is because I have 'b'
> as a keyword for bugzilla search, and pressing enter doesn't send me to the
> home page, but makes me search for an empty string in bugzilla; that's
> misleading :-/ (but probably off topic for this bug).

Ah that's bug 1124238, I should really find some time to properly test and land that.

Comment 9

[Marco Bonardo [:mak]]

Florian, do you need further answers for this review?

Comment 10

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #9)
> Florian, do you need further answers for this review?

Not really, the reason I haven't r+'ed yet is that I wanted to test the patch locally to see if I find cases where we lose the icon and it feels like an actual loss.

Comment 11

[Marco Bonardo [:mak]]

I don't think anyone will even notice this. You should basically type exactly a url that you have in history, that has a favicon and that is not just a host, at a minimum "somedomain/somethingelse" when you have it in history and it also has a favicon. and it's likely as soon as you have finished typing it you'll just Enter :)

Comment 12

[Florian Quèze [:florian]]

Would it be reasonably easy to display the globe (without flickering) until the user types '/', and then search for an icon for the hostname before the '/'? (this is a compromise between the 2 solutions you proposed in comment 3).

Comment 13

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #12)
> Would it be reasonably easy to display the globe (without flickering) until
> the user types '/', and then search for an icon for the hostname before the
> '/'? (this is a compromise between the 2 solutions you proposed in comment
> 3).

Potentially it may be possible, we should look for a /, then look if the url has a host (with a try catch since urls without a host throw) finally use one icon or the other one.
It would be more work while the user is typing, with a not much higher probability to have the icon for the host. But maybe it will save some icons.

I'll try and see how it works, tomorrow probably.

Comment 14

[Florian Quèze [:florian]]

There's already a check for the host a few lines above: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/components/places/UnifiedComplete.js#1598

We could put hostExpected.has(uri.scheme) in a variable there, and always use the globe when it's false.

Comment 15

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #14)
> There's already a check for the host a few lines above:
> http://searchfox.org/mozilla-central/rev/
> 31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/components/places/
> UnifiedComplete.js#1598
> 
> We could put hostExpected.has(uri.scheme) in a variable there, and always
> use the globe when it's false.

well yes, it would only work for those schemes, but probably we don't care.

Comment 16

[Marco Bonardo [:mak]]

Attachment: Beta one-liner fix

Approval Request Comment
[Feature/Bug causing the regression]: No regression
[User impact if declined]: If the user disabled search suggestions from the opt-in notification and upgrades to 55 in the same session, we may be unable to know whether he picked YES or NO in the notification.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: it will be verified by QA, since QA found the original issue
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a oneliner mirroring a pref value when another pref changes
[String changes made/needed]: none

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8872941 [details] [diff] [review]
Beta one-liner fix

OOps wrong bug.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8871299 [details]
Bug 1363621 - Favicon in the first location bar suggestion flickers when typing url.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142786/diff/1-2/

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8871299 [details]
Bug 1363621 - Favicon in the first location bar suggestion flickers when typing url.

https://reviewboard.mozilla.org/r/142786/#review148334

::: toolkit/components/places/UnifiedComplete.js:1603
(Diff revisions 1 - 2)
>      // Check the host, as "http:///" is a valid nsIURI, but not useful to us.
>      // But, some schemes are expected to have no host. So we check just against
>      // schemes we know should have a host. This allows new schemes to be
>      // implemented without us accidentally blocking access to them.
>      let hostExpected = new Set(["http", "https", "ftp", "chrome"]);
>      if (hostExpected.has(uri.scheme) && !uri.host)

I think here we should save the result of hostExpected.has(uri.scheme) in a variable.

And given the early return if !uri.host, we would need to check 'uri.host' again later.

::: toolkit/components/places/UnifiedComplete.js:1632
(Diff revisions 1 - 2)
> -    // be expensive. Thus we also don't know if we may have an icon for it.
> +    // be expensive. Thus we also don't know if we may have an icon.
>      // If we'd just try to fetch the icon for the typed string, we'd cause icon
> -    // flicker, since the string keeps growing while the user types. Even if
> -    // we'd return the icon for the url host, we'd cause flicker until the user
> -    // starts typing the url path.
> -    // To sum up, we won't provide any icon and fallback to the default one.
> +    // flicker, since the url keeps changing while the user types.
> +    // By default we won't provide an icon, but for the subset of urls with a
> +    // host we'll check for a typed slash and set favicon for the host part.
> +    if (hostExpected && uri.host &&

hostExpected is a set, not a boolean.

Comment 20

[Marco Bonardo [:mak]]

oh lol, I read this really wrong. To my excuse, the variable name is horrible.

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8871299 [details]
Bug 1363621 - Favicon in the first location bar suggestion flickers when typing url.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142786/diff/2-3/

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8871299 [details]
Bug 1363621 - Favicon in the first location bar suggestion flickers when typing url.

https://reviewboard.mozilla.org/r/142786/#review148350

Looks good to me (note: I haven't tested the patch locally).

Comment 23

[Marco Bonardo [:mak]]

I did test the patch locally, it works fine (well the previous one was working too because the Set was always defined!).
There are some incoherencies here and there, like for example for some pages we have icon for the https version but not for the http one, so it may look strange that an entry in the popup has an icon and the other one doesn't, but that's not fault of this patch. We likely should just relax the favicons service to fetch more icons in more cases (even if the situation already improved compared to the previous store).

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5df9b21d7a40
Favicon in the first location bar suggestion flickers when typing url. r=florian

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5df9b21d7a40

Comment 26

[Hani Yacoub, Desktop QA]

Build ID: 20170612224034
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Beta 55.0b1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.