Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: bnicholson, Assigned: lucasr)

Tracking

unspecified
All
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

(Whiteboard: [QA?])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
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.
Priority: -- → P1

Comment 2

6 years ago
how is this important for perf?  questioning the p1.
(Assignee)

Comment 3

6 years ago
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?
(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.
Assignee: nobody → blassey.bugs
Assignee: blassey.bugs → lucasr.at.mozilla
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.
(Assignee)

Comment 6

6 years ago
Created attachment 571718 [details] [diff] [review]
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.
Attachment #571718 - Flags: review?(blassey.bugs)
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.
(Reporter)

Comment 8

6 years ago
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.
(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.
(Reporter)

Comment 10

6 years ago
(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 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();
Attachment #571718 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
(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.
(Assignee)

Comment 15

6 years ago
Created attachment 571967 [details] [diff] [review]
Implement favicon caching

This patch fixes all issues brought by Brad's and Kats' reviews.
Attachment #571718 - Attachment is obsolete: true
Attachment #571967 - Flags: review?(blassey.bugs)
(Assignee)

Comment 16

6 years ago
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 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"
Attachment #571967 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Comment 19

6 years ago
Pushed: http://hg.mozilla.org/projects/birch/rev/f069ffc41471
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
20111107055846
http://hg.mozilla.org/projects/birch/rev/f069ffc41471
Samsung Nexus S (Android 2.3.6)
Status: RESOLVED → VERIFIED
These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [QA?]
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.