Dominant color favicon backgrounds are really slow to update

VERIFIED FIXED in Firefox 23



Firefox for Android
4 years ago
a year ago


(Reporter: Margaret, Assigned: Margaret)


Firefox 23

Firefox Tracking Flags

(firefox23 verified)



(1 attachment, 1 obsolete attachment)

15.74 KB, patch
: review+
Details | Diff | Splinter Review


4 years ago
We call updateFavicon() a lot, we should do something smarter to cache the dominant color instead of computing it each time we want to use it.


4 years ago
Assignee: nobody → margaret.leibovic

Comment 1

4 years ago
Created attachment 744373 [details] [diff] [review]

This is a lazy way of caching the colors. Ideally, it would be nice to better centralize all this repeated favicon logic, but that's hard to do with the way our awesomescreen tabs are currently implemented, so maybe that's just something to consider when we implement the new about:home.

I want to do some benchmarking to see how long BitmapUtils.getDominantColor() actually takes for a 16x16 icon, and if it's not very long, it's probably fine to be doing that on the main thread, especially if we're caching it like this.
Attachment #744373 - Flags: feedback?(bnicholson)
Comment on attachment 744373 [details] [diff] [review]

Review of attachment 744373 [details] [diff] [review]:

::: mobile/android/base/
@@ +76,5 @@
>          // Create a failed favicon memory cache that has up to 64 entries
>          mFailedCache = new LruCache<String, Long>(64);
> +
> +        // Create a cache to store favicon dominant colors
> +        mColorCache = new LruCache<String, Integer>(64);

64 seems small to me; I see more than 64 entries in AllPagesTab alone. Assuming an average favicon size of 32x32 (which is an overestimate), that means the typical favicon will be 32x32x4 = 4kB, so we should probably have at least 256 entries.

It'd be nice if we could use the same cache to store a pairing of favicon/color data rather than having two separate caches, but I guess that's not really an option until we use the favicon cache for all tabs.
Attachment #744373 - Flags: feedback?(bnicholson) → feedback+

Comment 3

4 years ago
Created attachment 745932 [details] [diff] [review]

I decided that I didn't like calling getFaviconColor() in a bunch of different places then passing around the color, mainly because we don't even need the color in the case of empty or large favicons. As a compromise, instead of letting FaviconView know about urls, I just made the parameter a string key that's used for caching (this is more accurate for search engines anyway, for which we just use the engine name).

I tested this with my (huge/old) default profile on my Galaxy Nexus, and this performs way better than Nightly. I also timed getDominantColor(), and times ranged from 4-20ms, but most times were around 8-10ms.
Attachment #744373 - Attachment is obsolete: true
Attachment #745932 - Flags: review?(bnicholson)
Comment on attachment 745932 [details] [diff] [review]

Review of attachment 745932 [details] [diff] [review]:

::: mobile/android/base/awesomebar/
@@ -92,5 @@
> -        Bitmap favicon = null;
> -        if (b != null) {
> -            Bitmap bitmap = BitmapUtils.decodeByteArray(b);
> -            if (bitmap != null) {
> -                favicon = Favicons.getInstance().scaleImage(bitmap);

It looks like you moved the scaleImage() call from AwesomeBarTab to BookmarksTab. Won't that break favicon scaling for AllPagesTab and HistoryTab?
Comment on attachment 745932 [details] [diff] [review]

Oh nevermind...that's the only place that uses the cursor.
Attachment #745932 - Flags: review?(bnicholson) → review+

Comment 6

4 years ago
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
status-firefox23: --- → verified
You need to log in before you can comment on or make changes to this bug.