Bug 1044940 - Favicons in the bookmarks table should be read and written correctly, r=ckitching
authorMark Capella <markcapella@twcny.rr.com>
Sun, 14 Sep 2014 17:22:19 -0700
changeset 205323 54ca5ea4de1dacd96868be7cb520798193461bee
parent 205322 3646cd944abe82efeaf2c44fef429e5a94263c38
child 205324 449fb0bd623fd7cc7174216f935c7108a516e404
push id49139
push usercbook@mozilla.com
push dateMon, 15 Sep 2014 12:34:39 +0000
treeherdermozilla-inbound@ff1d0d059574 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckitching
bugs1044940
milestone35.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1044940 - Favicons in the bookmarks table should be read and written correctly, r=ckitching
mobile/android/base/db/BrowserDB.java
mobile/android/base/db/LocalBrowserDB.java
mobile/android/base/favicons/Favicons.java
mobile/android/base/favicons/LoadFaviconTask.java
--- a/mobile/android/base/db/BrowserDB.java
+++ b/mobile/android/base/db/BrowserDB.java
@@ -195,18 +195,21 @@ public class BrowserDB {
     public static void removeReadingListItemWithURL(ContentResolver cr, String uri) {
         sDb.removeReadingListItemWithURL(cr, uri);
     }
 
     public static LoadFaviconResult getFaviconForFaviconUrl(ContentResolver cr, String faviconURL) {
         return sDb.getFaviconForUrl(cr, faviconURL);
     }
 
-    public static String getFaviconUrlForHistoryUrl(ContentResolver cr, String url) {
-        return sDb.getFaviconUrlForHistoryUrl(cr, url);
+    /**
+     * Try to find a usable favicon URL in the history or bookmarks table.
+     */
+    public static String getFaviconURLFromPageURL(ContentResolver cr, String url) {
+        return sDb.getFaviconURLFromPageURL(cr, url);
     }
 
     public static void updateFaviconForUrl(ContentResolver cr, String pageUri, byte[] encodedFavicon, String faviconUri) {
         sDb.updateFaviconForUrl(cr, pageUri, encodedFavicon, faviconUri);
     }
 
     public static void updateThumbnailForUrl(ContentResolver cr, String uri, BitmapDrawable thumbnail) {
         sDb.updateThumbnailForUrl(cr, uri, thumbnail);
--- a/mobile/android/base/db/LocalBrowserDB.java
+++ b/mobile/android/base/db/LocalBrowserDB.java
@@ -53,16 +53,18 @@ import android.net.Uri;
 import android.provider.Browser;
 import android.text.TextUtils;
 import android.util.Log;
 
 public class LocalBrowserDB {
     // Calculate these once, at initialization. isLoggable is too expensive to
     // have in-line in each log call.
     private static final String LOGTAG = "GeckoLocalBrowserDB";
+    private static final Integer FAVICON_ID_NOT_FOUND = Integer.MIN_VALUE;
+
     private static final boolean logDebug = Log.isLoggable(LOGTAG, Log.DEBUG);
     protected static void debug(String message) {
         if (logDebug) {
             Log.d(LOGTAG, message);
         }
     }
 
     private final String mProfile;
@@ -993,49 +995,124 @@ public class LocalBrowserDB {
 
         if (b == null) {
             return null;
         }
 
         return FaviconDecoder.decodeFavicon(b);
     }
 
-    public String getFaviconUrlForHistoryUrl(ContentResolver cr, String uri) {
-        final Cursor c = cr.query(mHistoryUriWithProfile,
-                                  new String[] { History.FAVICON_URL },
-                                  Combined.URL + " = ?",
-                                  new String[] { uri },
-                                  null);
+    /**
+     * Try to find a usable favicon URL in the history or bookmarks table.
+     */
+    public String getFaviconURLFromPageURL(ContentResolver cr, String uri) {
+        // Check first in the history table.
+        Cursor c = cr.query(mHistoryUriWithProfile,
+                            new String[] { History.FAVICON_URL },
+                            Combined.URL + " = ?",
+                            new String[] { uri },
+                            null);
 
         try {
-            if (!c.moveToFirst()) {
-                return null;
+            if (c.moveToFirst()) {
+                return c.getString(c.getColumnIndexOrThrow(History.FAVICON_URL));
+            }
+        } finally {
+            c.close();
+        }
+
+        // If that fails, check in the bookmarks table.
+        c = cr.query(mBookmarksUriWithProfile,
+                     new String[] { Bookmarks.FAVICON_URL },
+                     Bookmarks.URL + " = ?",
+                     new String[] { uri },
+                     null);
+
+        try {
+            if (c.moveToFirst()) {
+                return c.getString(c.getColumnIndexOrThrow(Bookmarks.FAVICON_URL));
             }
 
-            return c.getString(c.getColumnIndexOrThrow(History.FAVICON_URL));
+            return null;
         } finally {
             c.close();
         }
     }
 
     public void updateFaviconForUrl(ContentResolver cr, String pageUri,
             byte[] encodedFavicon, String faviconUri) {
         ContentValues values = new ContentValues();
         values.put(Favicons.URL, faviconUri);
         values.put(Favicons.PAGE_URL, pageUri);
         values.put(Favicons.DATA, encodedFavicon);
 
         // Update or insert
         Uri faviconsUri = getAllFaviconsUri().buildUpon().
                 appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();
 
-        cr.update(faviconsUri,
-                  values,
-                  Favicons.URL + " = ?",
-                  new String[] { faviconUri });
+        final int updated = cr.update(faviconsUri,
+                                      values,
+                                      Favicons.URL + " = ?",
+                                      new String[] { faviconUri });
+
+        if (updated == 0) {
+            return;
+        }
+
+        // After writing the encodedFavicon, ensure that the favicon_id in both the bookmark and
+        // history tables are also up-to-date.
+        final Integer id = getIDForFaviconURL(cr, faviconUri);
+        if (id == FAVICON_ID_NOT_FOUND) {
+            return;
+        }
+
+        updateHistoryAndBookmarksFaviconID(cr, pageUri, id);
+    }
+
+    /**
+     * Locates and returns the favicon ID of a target URL as an Integer.
+     */
+    private Integer getIDForFaviconURL(ContentResolver cr, String faviconURL) {
+        final Cursor c = cr.query(mFaviconsUriWithProfile,
+                                  new String[] { Favicons._ID },
+                                  Favicons.URL + " = ? AND " + Favicons.DATA + " IS NOT NULL",
+                                  new String[] { faviconURL },
+                                  null);
+
+        try {
+            final int col = c.getColumnIndexOrThrow(Favicons._ID);
+            if (c.moveToFirst() && !c.isNull(col)) {
+                return c.getInt(col);
+            }
+
+            // IDs can be negative, so we return a sentinel value indicating "not found".
+            return FAVICON_ID_NOT_FOUND;
+        } finally {
+            c.close();
+        }
+    }
+
+    /**
+     * Update the favicon ID in the history and bookmark tables after a new
+     * favicon table entry is added.
+     */
+    private void updateHistoryAndBookmarksFaviconID(ContentResolver cr, String pageURL, int id) {
+        final ContentValues bookmarkValues = new ContentValues();
+        bookmarkValues.put(Bookmarks.FAVICON_ID, id);
+        cr.update(mBookmarksUriWithProfile,
+                  bookmarkValues,
+                  Bookmarks.URL + " = ?",
+                  new String[] { pageURL });
+
+        final ContentValues historyValues = new ContentValues();
+        historyValues.put(History.FAVICON_ID, id);
+        cr.update(mHistoryUriWithProfile,
+                  historyValues,
+                  History.URL + " = ?",
+                  new String[] { pageURL });
     }
 
     public void updateThumbnailForUrl(ContentResolver cr, String uri,
             BitmapDrawable thumbnail) {
 
         // If a null thumbnail was passed in, delete the stored thumbnail for this url.
         if (thumbnail == null) {
             cr.delete(mThumbnailsUriWithProfile, Thumbnails.URL + " == ?", new String[] { uri });
--- a/mobile/android/base/favicons/Favicons.java
+++ b/mobile/android/base/favicons/Favicons.java
@@ -12,16 +12,17 @@ import org.mozilla.gecko.R;
 import org.mozilla.gecko.Tab;
 import org.mozilla.gecko.Tabs;
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.favicons.cache.FaviconCache;
 import org.mozilla.gecko.util.GeckoJarReader;
 import org.mozilla.gecko.util.NonEvictingLruCache;
 import org.mozilla.gecko.util.ThreadUtils;
 
+import android.content.ContentResolver;
 import android.content.Context;
 import android.content.res.Resources;
 import android.graphics.Bitmap;
 import android.graphics.BitmapFactory;
 import android.text.TextUtils;
 import android.util.Log;
 import android.util.SparseArray;
 
@@ -48,18 +49,16 @@ public class Favicons {
     // Size of the favicon bitmap cache, in bytes (Counting payload only).
     public static final int FAVICON_CACHE_SIZE_BYTES = 512 * 1024;
 
     // Number of URL mappings from page URL to Favicon URL to cache in memory.
     public static final int NUM_PAGE_URL_MAPPINGS_TO_STORE = 128;
 
     public static final int NOT_LOADING  = 0;
     public static final int LOADED       = 1;
-    public static final int FLAG_PERSIST = 2;
-    public static final int FLAG_SCALE   = 4;
 
     // The default Favicon to show if no other can be found.
     public static Bitmap defaultFavicon;
 
     // The density-adjusted default Favicon dimensions.
     public static int defaultFaviconSize;
 
     // The density-adjusted maximum Favicon dimensions.
@@ -255,22 +254,25 @@ public class Favicons {
         Tab theTab = Tabs.getInstance().getFirstTabForUrl(pageURL);
         if (theTab != null) {
             targetURL = theTab.getFaviconURL();
             if (targetURL != null) {
                 return targetURL;
             }
         }
 
-        targetURL = BrowserDB.getFaviconUrlForHistoryUrl(context.getContentResolver(), pageURL);
-        if (targetURL == null) {
-            // Nothing in the history database. Fall back to the default URL and hope for the best.
-            targetURL = guessDefaultFaviconURL(pageURL);
+        // Try to find the faviconURL in the history and/or bookmarks table.
+        final ContentResolver resolver = context.getContentResolver();
+        targetURL = BrowserDB.getFaviconURLFromPageURL(resolver, pageURL);
+        if (targetURL != null) {
+            return targetURL;
         }
-        return targetURL;
+
+        // If we still can't find it, fall back to the default URL and hope for the best.
+        return guessDefaultFaviconURL(pageURL);
     }
 
     /**
      * Helper function to create an async job to load a Favicon which does not exist in the memcache.
      * Contains logic to prevent the repeated loading of Favicons which have previously failed.
      * There is no support for recovery from transient failures.
      *
      * @param pageURL URL of the page for which to load a Favicon. If null, no job is created.
--- a/mobile/android/base/favicons/LoadFaviconTask.java
+++ b/mobile/android/base/favicons/LoadFaviconTask.java
@@ -41,17 +41,16 @@ import java.util.concurrent.atomic.Atomi
 public class LoadFaviconTask {
     private static final String LOGTAG = "LoadFaviconTask";
 
     // Access to this map needs to be synchronized prevent multiple jobs loading the same favicon
     // from executing concurrently.
     private static final HashMap<String, LoadFaviconTask> loadsInFlight = new HashMap<>();
 
     public static final int FLAG_PERSIST = 1;
-    public static final int FLAG_SCALE = 2;
     private static final int MAX_REDIRECTS_TO_FOLLOW = 5;
     // The default size of the buffer to use for downloading Favicons in the event no size is given
     // by the server.
     private static final int DEFAULT_FAVICON_BUFFER_SIZE = 25000;
 
     private static final AtomicInteger nextFaviconLoadId = new AtomicInteger(0);
     private final Context context;
     private final int id;