Bug 1040806 - Default bookmark favicons are not displayed after browser restart. r=margaret, a=sledru
authorRichard Newman <rnewman@mozilla.com>
Mon, 21 Jul 2014 12:30:45 -0700
changeset 217240 89dbad79b0b358a33e2e07e5450cdf0d0915f4c4
parent 217239 91e719c1469037668c96d0341f568aff4b66d2c2
child 217241 4b84feaae1f1cb69e57327e0eb4c03908a24f465
push id515
push userraliiev@mozilla.com
push dateMon, 06 Oct 2014 12:51:51 +0000
treeherdermozilla-release@267c7a481bef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmargaret, sledru
bugs1040806
milestone33.0a2
Bug 1040806 - Default bookmark favicons are not displayed after browser restart. r=margaret, a=sledru
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);
             }