923218 - Don't display default favicon for milliseconds before the real icon loads

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Richard Newman [:rnewman]]

See Bug 914296 Comment 73:

---
I'm going to file a follow-up bug for UI polish: because we asynchronously load favicons, we're pretty much guaranteed to briefly show the grey globe icon in the URL bar before, some milliseconds later, drawing the site's favicon. That looks janky, but it's not a regression against current Nightly. I imagine we know in advance whether we'll be drawing a favicon; in that case we can skip the intermediate step.
---

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Don't show default favicon until a favicon load has failed.

With this patch, nothing is shown until the favicon load attempt has concluded. If it is a failure, the default favicon is shown, otherwise we show whatever was returned.
Seems to do the trick.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 813634 [details] [diff] [review]
Don't show default favicon until a favicon load has failed.

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

This looks sane to me, assuming that we're happy with this experience instead.

Alternatives would be to delay the default favicon set, but still always do it; to animate in the favicon; or to do different things for favicons that are cached in memory versus in DB versus network.

Ian, do you have an opinion here?

::: mobile/android/base/BrowserToolbar.java
@@ +1116,5 @@
>          if (image != null) {
>              image = Bitmap.createScaledBitmap(image, mFaviconSize, mFaviconSize, false);
>              mFavicon.setImageBitmap(image);
>          } else {
> +            mFavicon.setImageBitmap(null);

if (image != null) {
  image = Bitmap.create…(…);
}
mFavicon.setImageBitmap(image);

Comment 3

[Ian Barlow (:ibarlow)]

It sounds from your description that we don't show *anything*, not even cached favicons, until we try to load new ones. I don't think that's what we want. 


----

To me, the desired behaviour would be

1. Load whatever favicons we have cached as instantaneously as possible
2. Attempt to load additional favicons, and display nothing in those favicon slots until (a.) we have a favicon to display or (b.) there is no favicon, at which point we display the globe.

Comment 4

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

(In reply to Ian Barlow (:ibarlow) from comment #3)

> 1. Load whatever favicons we have cached as instantaneously as possible
> 2. Attempt to load additional favicons, and display nothing in those favicon
> slots until (a.) we have a favicon to display or (b.) there is no favicon,
> at which point we display the globe.

I believe that is what this patch does. Currently, we show the globe favicon while trying to load true favicon.

Comment 5

[Chris Kitching [:ckitching]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> I believe that is what this patch does. Currently, we show the globe favicon
> while trying to load true favicon.

Aye.

Initially, nothing is shown.
Then, an attempt is made to load a favicon. This will come from somewhere locally, or perhaps from the internet if we have nothing local (Or whatever we have locally is too old).
After this attempt to load a favicon has completed, one of two things happens (With nothing being shown throughout the loading process):

If the attempt to load a favicon was successful, locally or otherwise, the result is displayed.
If the attempt to load a favicon was a failure - corrupt favicon was downloaded, no favicon available, etc - then we display the default favicon.

Hopefully this clarifies somewhat.

Comment 6

[Ian Barlow (:ibarlow)]

Yep, sounds perfect. Thanks guys.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 813634 [details] [diff] [review]
Don't show default favicon until a favicon load has failed.

Thanks for weighing in, Ian!

Comment 8

[Sriram Ramasubramanian [:sriram]]

(In reply to Richard Newman [:rnewman] from comment #2)
> Comment on attachment 813634 [details] [diff] [review]
> Don't show default favicon until a favicon load has failed.
> 
> Review of attachment 813634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks sane to me, assuming that we're happy with this experience
> instead.
> 
> Alternatives would be to delay the default favicon set, but still always do
> it; to animate in the favicon; or to do different things for favicons that
> are cached in memory versus in DB versus network.
> 
> Ian, do you have an opinion here?
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +1116,5 @@
> >          if (image != null) {
> >              image = Bitmap.createScaledBitmap(image, mFaviconSize, mFaviconSize, false);
> >              mFavicon.setImageBitmap(image);
> >          } else {
> > +            mFavicon.setImageBitmap(null);
> 
> if (image != null) {
>   image = Bitmap.create…(…);
> }
> mFavicon.setImageBitmap(image);

It is highly recommended not to use setImageBitmap(null) especially with list views. Android behaves strangely with it (at times it doesnt become blank). It's better to set a transparent icon there.

mFavicon.setImageResource(android.R.color.transparent);

is preferable in such cases.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Nevermind, this is not for lists. But still it's better to use "transparent" resource.

Comment 10

[Chris Kitching [:ckitching]]

Attachment: V2 - Don't show default favicon until a favicon load has failed.

Unbitrotting. Has to mildly alter Favicons now.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 816065 [details] [diff] [review]
V2 - Don't show default favicon until a favicon load has failed.

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

::: mobile/android/base/BrowserToolbar.java
@@ +1120,5 @@
>          if (image != null) {
>              image = Bitmap.createScaledBitmap(image, mFaviconSize, mFaviconSize, false);
>              mFavicon.setImageBitmap(image);
>          } else {
> +            mFavicon.setImageBitmap(null);

If you're going to keep the 'else', use Sriram's suggestion. Otherwise, see my last review comment!

Comment 12

[Richard Newman [:rnewman]]

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

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/27a7bb8d8327