697194 - Favicon caching

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Brian Nicholson (:bnicholson)]

We need to figure out a way to cache the favicons. Right now, we issue an HTTP request on each page load, even if we've fetched that favicon previously.

Comment 1

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

Important for performance

Why not just start with an in-memory cache? Build a map of favicons, keyed on the URL. Look in the cache before doing the HTTP download.

Comment 2

[Doug Turner (:dougt)]

how is this important for perf?  questioning the p1.

Comment 3

[Lucas Rocha (:lucasr)]

If understood correctly, we're supposed to store favicons in Android's history/bookmark store as a binary blob. At least this is how you access favicons there. See:

http://developer.android.com/reference/android/provider/Browser.BookmarkColumns.html#FAVICON

So, isn't it just about ensuring we're always storing downloaded favicons in Android's history/bookmark store for later access?

Comment 4

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

(In reply to Doug Turner (:dougt) from comment #2)
> how is this important for perf?  questioning the p1.

I have no numbers, but we know that moving the download to a background thread save a few hundred milliseconds. Knowing that networking can affect responsiveness is also a concern.

Feel free to change the priority, but this will be an important improvement.

Comment 5

[Sriram Ramasubramanian [:sriram]]

The downloading of favicon has been moved to the background thread.
We can store the FAVICON resource along with history/bookmark entries too -> http://developer.android.com/reference/android/provider/Browser.BookmarkColumns.html#FAVICON

However, we wouldn't know from where we downloaded the resource, and if the resource has changed.

Probably, we can show this until the actual favicon loads.. to give a perception that page loads are faster.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Implement favicon caching

Here's a first go on the favicon cache. I'm added a simple db to hold urls and their respective favicon URL for cache invalidation purposes. We load favicon from disk whenever possible. Download it otherwise. We only start loading favicons once page is fully loaded.

Follow-up patches:
- Clear favicon URL database when browser history is cleared.
- Add a way to cancel a certain favicon request in case the tab URL changes while favicon is being loaded.
- We still need to handle the page redirects and the side-effects on favicon properly.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 571718 [details] [diff] [review]
Implement favicon caching

Mostly some nits, except for the last comment, which is a regression.


>diff --git a/embedding/android/Favicons.java b/embedding/android/Favicons.java
>...
>+ * Portions created by the Initial Developer are Copyright (C) 2009-2010

Copyright year should probably be 2011.

>+    public interface OnFaviconLoadedListener {
>+        public abstract void onFaviconLoaded(String url, Drawable favicon);
>+    }

Interface functions are implicitly abstract. You shouldn't need to specify "abstract".

>+        private static final int COLUMN_FAVICON_URL_INDEX = 0;

The name of this constant is slightly misleading, since it's the index with respect to mDefaultProjection, not with respect to the table.

>+
>+    private class LoadFaviconTask extends AsyncTask<Void, Void, Drawable> {

You could use BitmapDrawable instead of Drawable here, since you're doing a cast to BitmapDrawable in saveFaviconToDb() anyway, so the abstraction doesn't win you anything.

>+        public LoadFaviconTask(String pageUrl, String faviconUrl, OnFaviconLoadedListener listener) {
>+            mPageUrl = new String(pageUrl);

Creating a new String here is redundant, since strings are immutable anyway. mPageUrl = pageUrl is better.

>+            mListener = listener;
>+
>+            if (faviconUrl != null)
>+                mFaviconUrl = new String(faviconUrl);

Ditto.

>+            @Override
>+            public void onFaviconLoaded(String pageUrl, Drawable favicon) {

The @Override annotation here might fail compilation on java 1.5. @Override on interface implementations is only supported on java 1.6+

>     void handleLinkAdded(final int tabId, String rel, final String href) {
>         if (rel.indexOf("icon") != -1) {
>             Tab tab = Tabs.getInstance().getTab(tabId);
>             if (tab != null)
>-                tab.downloadFavicon(href);
>+                tab.updateFaviconURL(href);

This code path doesn't handle the case where the link tag is added at some point after page load (e.g. via javascript). The mFaviconUrl in the Tab will get updated, but the new favicon won't get downloaded and displayed.

Comment 8

[Brian Nicholson (:bnicholson)]

What if we fetch a favicon at a URL, and the image at that URL is later replaced with a new one?  If we don't check to see when the icon expires, we'll be stuck with the stale one.

Comment 9

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

(In reply to Brian Nicholson from comment #8)
> What if we fetch a favicon at a URL, and the image at that URL is later
> replaced with a new one?  If we don't check to see when the icon expires,
> we'll be stuck with the stale one.

Always fetching the favicon kinda kills the "cache" concept. We'd like to avoid downloading the favicon at all.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> (In reply to Brian Nicholson from comment #8)
> > What if we fetch a favicon at a URL, and the image at that URL is later
> > replaced with a new one?  If we don't check to see when the icon expires,
> > we'll be stuck with the stale one.
> 
> Always fetching the favicon kinda kills the "cache" concept. We'd like to
> avoid downloading the favicon at all.

I didn't mean to always fetch, but instead doing a conditional GET after some expiration period to see if it has been modified (just all other cached items).

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 571718 [details] [diff] [review]
Implement favicon caching

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

::: embedding/android/Favicons.java
@@ +14,5 @@
> + *
> + * The Original Code is Mozilla Android code.
> + *
> + * The Initial Developer of the Original Code is Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2009-2010

it is 2011

@@ +124,5 @@
> +            qb.setProjectionMap(mDefaultProjection);
> +
> +            Cursor c = qb.query(
> +                db,
> +                null,

use a projection

@@ +141,5 @@
> +            Log.d(LOG_NAME, "Calling setFaviconUrlForPageUrl() for " + pageUrl);
> +
> +            SQLiteDatabase db = mDbHelper.getWritableDatabase();
> +
> +            ContentValues initialValues = new ContentValues();

nit, initialValues doesn't seem to be an appropriate variable name if we're updating the database

@@ +199,5 @@
> +
> +            ContentResolver resolver = mContext.getContentResolver();
> +
> +            Cursor c = resolver.query(Browser.BOOKMARKS_URI,
> +                                      null,

use a projection

@@ +282,5 @@
> +            URL faviconUrl = null;
> +
> +            // If favicon is empty, fallback to default favicon URI
> +            if (mFaviconUrl == null || mFaviconUrl.length() == 0)
> +                mFaviconUrl = pageUrl.getProtocol() + "://" + pageUrl.getAuthority() + "/favicon.ico";

use a constructor which takes these url parts explicitly rather than concatenating strings
so:
faviconUrl = new URL(pageUrl.getProtocol(), getAuthority(), "favicon.ico");
or:
faviconUrl = new URI(pageUrl.getProtocol(), pageUrl.getUserInfo, pageUrl.getHost(), pageUrl.getPost(), "favicon.ico", null, null).toURL();
and then 
mFaviconUrl = faviconUrl.toString();

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Kartikaya Gupta (:kats) from comment #7)
> Comment on attachment 571718 [details] [diff] [review] [diff] [details] [review]
> Implement favicon caching
> 
> Mostly some nits, except for the last comment, which is a regression.
> 
> 
> >diff --git a/embedding/android/Favicons.java b/embedding/android/Favicons.java
> >...
> >+ * Portions created by the Initial Developer are Copyright (C) 2009-2010
> 
> Copyright year should probably be 2011.

Fixed.
 
> >+    public interface OnFaviconLoadedListener {
> >+        public abstract void onFaviconLoaded(String url, Drawable favicon);
> >+    }
> 
> Interface functions are implicitly abstract. You shouldn't need to specify
> "abstract".

Done.

> >+        private static final int COLUMN_FAVICON_URL_INDEX = 0;
> 
> The name of this constant is slightly misleading, since it's the index with
> respect to mDefaultProjection, not with respect to the table.

Renamed to PROJECTION_FAVICON_URL_INDEX to better match its semantics.

> >+
> >+    private class LoadFaviconTask extends AsyncTask<Void, Void, Drawable> {
> 
> You could use BitmapDrawable instead of Drawable here, since you're doing a
> cast to BitmapDrawable in saveFaviconToDb() anyway, so the abstraction
> doesn't win you anything.

True. Done.

> >+        public LoadFaviconTask(String pageUrl, String faviconUrl, OnFaviconLoadedListener listener) {
> >+            mPageUrl = new String(pageUrl);
> 
> Creating a new String here is redundant, since strings are immutable anyway.
> mPageUrl = pageUrl is better.

Fixed.

> >+            mListener = listener;
> >+
> >+            if (faviconUrl != null)
> >+                mFaviconUrl = new String(faviconUrl);
> 
> Ditto.

Fixed.

> >+            @Override
> >+            public void onFaviconLoaded(String pageUrl, Drawable favicon) {
> 
> The @Override annotation here might fail compilation on java 1.5. @Override
> on interface implementations is only supported on java 1.6+

Ok, removed the annotation.

> >     void handleLinkAdded(final int tabId, String rel, final String href) {
> >         if (rel.indexOf("icon") != -1) {
> >             Tab tab = Tabs.getInstance().getTab(tabId);
> >             if (tab != null)
> >-                tab.downloadFavicon(href);
> >+                tab.updateFaviconURL(href);
> 
> This code path doesn't handle the case where the link tag is added at some
> point after page load (e.g. via javascript). The mFaviconUrl in the Tab will
> get updated, but the new favicon won't get downloaded and displayed.

Good catch. I'll change patch to download favicon in that case if page is not loading.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #11)
> Comment on attachment 571718 [details] [diff] [review] [diff] [details] [review]
> Implement favicon caching
> 
> Review of attachment 571718 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: embedding/android/Favicons.java
> @@ +14,5 @@
> > + *
> > + * The Original Code is Mozilla Android code.
> > + *
> > + * The Initial Developer of the Original Code is Mozilla Foundation.
> > + * Portions created by the Initial Developer are Copyright (C) 2009-2010
> 
> it is 2011
> 
> @@ +124,5 @@
> > +            qb.setProjectionMap(mDefaultProjection);
> > +
> > +            Cursor c = qb.query(
> > +                db,
> > +                null,
> 
> use a projection

Not actually needed as I set the projection map (see the setProjectionMap() call above). But I replaced it with a simpler projection in the query call.

> @@ +141,5 @@
> > +            Log.d(LOG_NAME, "Calling setFaviconUrlForPageUrl() for " + pageUrl);
> > +
> > +            SQLiteDatabase db = mDbHelper.getWritableDatabase();
> > +
> > +            ContentValues initialValues = new ContentValues();
> 
> nit, initialValues doesn't seem to be an appropriate variable name if we're
> updating the database

Renamed to just 'values' for clarity.

> @@ +199,5 @@
> > +
> > +            ContentResolver resolver = mContext.getContentResolver();
> > +
> > +            Cursor c = resolver.query(Browser.BOOKMARKS_URI,
> > +                                      null,
> 
> use a projection

Done.

> @@ +282,5 @@
> > +            URL faviconUrl = null;
> > +
> > +            // If favicon is empty, fallback to default favicon URI
> > +            if (mFaviconUrl == null || mFaviconUrl.length() == 0)
> > +                mFaviconUrl = pageUrl.getProtocol() + "://" + pageUrl.getAuthority() + "/favicon.ico";
> 
> use a constructor which takes these url parts explicitly rather than
> concatenating strings
> so:
> faviconUrl = new URL(pageUrl.getProtocol(), getAuthority(), "favicon.ico");
> or:
> faviconUrl = new URI(pageUrl.getProtocol(), pageUrl.getUserInfo,
> pageUrl.getHost(), pageUrl.getPost(), "favicon.ico", null, null).toURL();
> and then 
> mFaviconUrl = faviconUrl.toString();

This is the code we had before btw. Fixed.

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson from comment #10)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> > (In reply to Brian Nicholson from comment #8)
> > > What if we fetch a favicon at a URL, and the image at that URL is later
> > > replaced with a new one?  If we don't check to see when the icon expires,
> > > we'll be stuck with the stale one.
> > 
> > Always fetching the favicon kinda kills the "cache" concept. We'd like to
> > avoid downloading the favicon at all.
> 
> I didn't mean to always fetch, but instead doing a conditional GET after
> some expiration period to see if it has been modified (just all other cached
> items).

Sounds like an important improvement (on a follow-up bug). I'll file a separate bug to track that. No need to block on that though. Thanks.

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Implement favicon caching

This patch fixes all issues brought by Brad's and Kats' reviews.

Comment 16

[Lucas Rocha (:lucasr)]

Filed those follow-up bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=699785
https://bugzilla.mozilla.org/show_bug.cgi?id=699786
https://bugzilla.mozilla.org/show_bug.cgi?id=699793
https://bugzilla.mozilla.org/show_bug.cgi?id=699795

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 571967 [details] [diff] [review]
Implement favicon caching

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

::: embedding/android/Favicons.java
@@ +273,5 @@
> +            // Handle the case of malformed favicon URL
> +            try {
> +                // If favicon is empty, fallback to default favicon URI
> +                if (mFaviconUrl == null || mFaviconUrl.length() == 0) {
> +                    faviconUrl = new URL(pageUrl.getProtocol(), pageUrl.getAuthority(), "/favicon.ico");

just "favicon.ico"

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Brad Lassey [:blassey] from comment #17)
> Comment on attachment 571967 [details] [diff] [review] [diff] [details] [review]
> Implement favicon caching
> 
> Review of attachment 571967 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: embedding/android/Favicons.java
> @@ +273,5 @@
> > +            // Handle the case of malformed favicon URL
> > +            try {
> > +                // If favicon is empty, fallback to default favicon URI
> > +                if (mFaviconUrl == null || mFaviconUrl.length() == 0) {
> > +                    faviconUrl = new URL(pageUrl.getProtocol(), pageUrl.getAuthority(), "/favicon.ico");
> 
> just "favicon.ico"

I get URLs like "http://lucasr.orgfavicon.ico" if I don't add the slash.

Comment 19

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/projects/birch/rev/f069ffc41471

Comment 20

[Aaron Train [:aaronmt]]

20111107055846
http://hg.mozilla.org/projects/birch/rev/f069ffc41471
Samsung Nexus S (Android 2.3.6)

Comment 21

[Wesley Johnston (:wesj)]

These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547