Closed Bug 923218 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: rnewman, Assigned: ckitching)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file, 1 obsolete file)

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.
---
Depends on: FaviconRevamp
Assignee: nobody → chriskitching
Status: NEW → ASSIGNED
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.
Attachment #813634 - Flags: review?(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);
Attachment #813634 - Flags: review?(rnewman)
Attachment #813634 - Flags: review+
Attachment #813634 - Flags: feedback?(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.
(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.
(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.
Yep, sounds perfect. Thanks guys.
Comment on attachment 813634 [details] [diff] [review]
Don't show default favicon until a favicon load has failed.

Thanks for weighing in, Ian!
Attachment #813634 - Flags: feedback?(ibarlow)
(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.
Nevermind, this is not for lists. But still it's better to use "transparent" resource.
Unbitrotting. Has to mildly alter Favicons now.
Attachment #813634 - Attachment is obsolete: true
Attachment #816065 - Flags: review?(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!
Attachment #816065 - Flags: review?(rnewman) → review+
No longer depends on: FaviconRevamp
https://hg.mozilla.org/integration/fx-team/rev/27a7bb8d8327
Whiteboard: [fixed in fx-team][qa+]
Target Milestone: --- → Firefox 27
https://hg.mozilla.org/mozilla-central/rev/27a7bb8d8327
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team][qa+] → [qa+]
Depends on: 938153
You need to log in before you can comment on or make changes to this bug.