Bug 1040806 - Default bookmark favicons are not displayed after browser restart. r=margaret
authorRichard Newman <rnewman@mozilla.com>
Mon, 21 Jul 2014 12:30:45 -0700
changeset 195375 bf683354136b12ac2836e30ae40b12892ded6dbb
parent 195374 70afb8fb9ae5766c32d223a4803f55c3f49ca123
child 195376 dc98e9e61e1344dc444c8d6ce687b7ce5463561e
push id46575
push userkwierso@gmail.com
push dateTue, 22 Jul 2014 00:35:21 +0000
treeherdermozilla-inbound@fee5c4bdd713 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmargaret
bugs1040806
milestone33.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 1040806 - Default bookmark favicons are not displayed after browser restart. r=margaret
mobile/android/base/GeckoProfile.java
mobile/android/base/db/LocalBrowserDB.java
--- a/mobile/android/base/GeckoProfile.java
+++ b/mobile/android/base/GeckoProfile.java
@@ -677,14 +677,19 @@ public final class GeckoProfile {
         final Distribution distribution = Distribution.getInstance(context);
         distribution.addOnDistributionReadyCallback(new Runnable() {
             @Override
             public void run() {
                 Log.d(LOGTAG, "Running post-distribution task: bookmarks.");
 
                 final ContentResolver cr = context.getContentResolver();
 
+                // We pass the number of added bookmarks to ensure that the
+                // indices of the distribution and default bookmarks are
+                // contiguous. Because there are always at least as many
+                // bookmarks as there are favicons, we can also guarantee that
+                // the favicon IDs won't overlap.
                 final int offset = BrowserDB.addDistributionBookmarks(cr, distribution, 0);
                 BrowserDB.addDefaultBookmarks(context, cr, offset);
             }
         });
     }
 }
--- a/mobile/android/base/db/LocalBrowserDB.java
+++ b/mobile/android/base/db/LocalBrowserDB.java
@@ -5,18 +5,18 @@
 
 package org.mozilla.gecko.db;
 
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.lang.reflect.Field;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.EnumSet;
 import java.util.HashMap;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
@@ -109,65 +109,106 @@ public class LocalBrowserDB implements B
         mReadingListUriWithProfile = appendProfile(ReadingListItems.CONTENT_URI);
 
         mUpdateHistoryUriWithProfile = mHistoryUriWithProfile.buildUpon().
             appendQueryParameter(BrowserContract.PARAM_INCREMENT_VISITS, "true").
             appendQueryParameter(BrowserContract.PARAM_INSERT_IF_NEEDED, "true").build();
     }
 
     /**
+     * Not thread safe. A helper to allocate new IDs for arbitrary strings.
+     */
+    private static class NameCounter {
+        private final HashMap<String, Integer> names = new HashMap<String, Integer>();
+        private int counter = 0;
+        private final int increment;
+
+        public NameCounter(int start, int increment) {
+            this.counter = start;
+            this.increment = increment;
+        }
+
+        public int get(final String name) {
+            Integer mapping = names.get(name);
+            if (mapping == null) {
+                int ours = counter;
+                counter += increment;
+                names.put(name, ours);
+                return ours;
+            }
+            return mapping.intValue();
+        }
+
+        public boolean has(final String name) {
+            return names.containsKey(name);
+        }
+    }
+
+    /**
      * Add default bookmarks to the database.
      * Takes an offset; returns a new offset.
      */
     @Override
     public int addDefaultBookmarks(Context context, ContentResolver cr, final int offset) {
-        long folderId = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
-        if (folderId == -1L) {
+        long folderID = getFolderIdFromGuid(cr, Bookmarks.MOBILE_FOLDER_GUID);
+        if (folderID == -1L) {
             Log.e(LOGTAG, "No mobile folder: cannot add default bookmarks.");
             return offset;
         }
 
         // Use reflection to walk the set of bookmark defaults.
         // This is horrible.
         final Class<?> stringsClass = R.string.class;
         final Field[] fields = stringsClass.getFields();
         final Pattern p = Pattern.compile("^bookmarkdefaults_title_");
 
         int pos = offset;
         final long now = System.currentTimeMillis();
 
         final ArrayList<ContentValues> bookmarkValues = new ArrayList<ContentValues>();
         final ArrayList<ContentValues> faviconValues = new ArrayList<ContentValues>();
+
+        // Count down from -offset into negative values to get new favicon IDs.
+        final NameCounter faviconIDs = new NameCounter((-1 - offset), -1);
+
         for (int i = 0; i < fields.length; i++) {
             final String name = fields[i].getName();
             final Matcher m = p.matcher(name);
             if (!m.find()) {
                 continue;
             }
 
             try {
-                final int titleid = fields[i].getInt(null);
-                final String title = context.getString(titleid);
+                final int titleID = fields[i].getInt(null);
+                final String title = context.getString(titleID);
 
                 final Field urlField = stringsClass.getField(name.replace("_title_", "_url_"));
-                final int urlId = urlField.getInt(null);
-                final String url = context.getString(urlId);
+                final int urlID = urlField.getInt(null);
+                final String url = context.getString(urlID);
 
-                bookmarkValues.add(createBookmark(now, title, url, pos++, folderId));
+                final ContentValues bookmarkValue = createBookmark(now, title, url, pos++, folderID);
+                bookmarkValues.add(bookmarkValue);
 
                 Bitmap icon = getDefaultFaviconFromPath(context, name);
                 if (icon == null) {
                     icon = getDefaultFaviconFromDrawable(context, name);
                 }
                 if (icon == null) {
                     continue;
                 }
 
                 final ContentValues iconValue = createFavicon(url, icon);
+
+                // Assign a reserved negative _id to each new favicon.
+                // For now, each name is expected to be unique, and duplicate
+                // icons will be duplicated in the DB. See Bug 1040806 Comment 8.
                 if (iconValue != null) {
+                    final int faviconID = faviconIDs.get(name);
+                    iconValue.put("_id", faviconID);
+                    bookmarkValue.put(Bookmarks.FAVICON_ID, faviconID);
                     faviconValues.add(iconValue);
                 }
             } catch (IllegalAccessException e) {
                 Log.wtf(LOGTAG, "Reflection failure.", e);
             } catch (IllegalArgumentException e) {
                 Log.wtf(LOGTAG, "Reflection failure.", e);
             } catch (NoSuchFieldException e) {
                 Log.wtf(LOGTAG, "Reflection failure.", e);
@@ -219,47 +260,71 @@ public class LocalBrowserDB implements B
 
         final Locale locale = Locale.getDefault();
         final long now = System.currentTimeMillis();
         int mobilePos = offset;
         int pinnedPos = 0;        // Assume nobody has pinned anything yet.
 
         final ArrayList<ContentValues> bookmarkValues = new ArrayList<ContentValues>();
         final ArrayList<ContentValues> faviconValues = new ArrayList<ContentValues>();
+
+        // Count down from -offset into negative values to get new favicon IDs.
+        final NameCounter faviconIDs = new NameCounter((-1 - offset), -1);
+
         for (int i = 0; i < bookmarks.length(); i++) {
             try {
                 final JSONObject bookmark = bookmarks.getJSONObject(i);
 
                 final String title = getLocalizedProperty(bookmark, "title", locale);
                 final String url = getLocalizedProperty(bookmark, "url", locale);
                 final long parent;
                 final int pos;
                 if (bookmark.has("pinned")) {
                     parent = Bookmarks.FIXED_PINNED_LIST_ID;
                     pos = pinnedPos++;
                 } else {
                     parent = folderId;
                     pos = mobilePos++;
                 }
 
-                bookmarkValues.add(createBookmark(now, title, url, pos, parent));
+                final ContentValues bookmarkValue = createBookmark(now, title, url, pos, parent);
+                bookmarkValues.add(bookmarkValue);
 
                 // Return early if there is no icon for this bookmark.
                 if (!bookmark.has("icon")) {
                     continue;
                 }
 
                 try {
                     final String iconData = bookmark.getString("icon");
                     final Bitmap icon = BitmapUtils.getBitmapFromDataURI(iconData);
                     if (icon == null) {
                         continue;
                     }
+
                     final ContentValues iconValue = createFavicon(url, icon);
-                    if (iconValue != null) {
+
+                    if (iconValue == null) {
+                        continue;
+                    }
+
+                    /*
+                     * Find out if this icon is a duplicate. If it is, don't try
+                     * to insert it again, but reuse the shared ID.
+                     * Otherwise, assign a new reserved negative _id.
+                     * Duplicates won't be detected in default bookmarks, or
+                     * those already in the database.
+                     */
+                    final boolean seen = faviconIDs.has(iconData);
+                    final int faviconID = faviconIDs.get(iconData);
+
+                    iconValue.put("_id", faviconID);
+                    bookmarkValue.put(Bookmarks.FAVICON_ID, faviconID);
+
+                    if (!seen) {
                         faviconValues.add(iconValue);
                     }
                 } catch (JSONException e) {
                     Log.e(LOGTAG, "Error creating distribution bookmark icon.", e);
                 }
             } catch (JSONException e) {
                 Log.e(LOGTAG, "Error creating distribution bookmark.", e);
             }