Bug 1076438 - Undo ID and imageurl/bgcolor cursor changes. r=lucasr
authorBrian Nicholson <bnicholson@mozilla.com>
Thu, 02 Oct 2014 09:48:50 -0700
changeset 208634 4a837d6936a03dd402ef6cdd31b1cc582284e979
parent 208633 a50ef251a2abdd8d3fd7c879eaccc3b4c0a9589c
child 208635 279afba13f4c109111417b3987205d6fe0f0eaed
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewerslucasr
bugs1076438
milestone35.0a1
Bug 1076438 - Undo ID and imageurl/bgcolor cursor changes. r=lucasr
mobile/android/base/db/BrowserContract.java
mobile/android/base/db/BrowserDB.java
mobile/android/base/db/SuggestedSites.java
mobile/android/base/db/TopSitesCursorWrapper.java
mobile/android/base/home/TopSitesPanel.java
mobile/android/tests/browser/junit3/src/TestSuggestedSites.java
--- a/mobile/android/base/db/BrowserContract.java
+++ b/mobile/android/base/db/BrowserContract.java
@@ -102,22 +102,16 @@ public class BrowserContract {
     }
 
     public interface DeletedColumns {
         public static final String ID = "id";
         public static final String GUID = "guid";
         public static final String TIME_DELETED = "timeDeleted";
     }
 
-    public interface SuggestedSitesColumns {
-        public static final String BGCOLOR = "bgcolor";
-        public static final String IMAGEURL = "imageurl";
-        public static final String TRACKING_ID = "tracking_id";
-    }
-
     @RobocopTarget
     public static final class Favicons implements CommonColumns, DateSyncColumns {
         private Favicons() {}
 
         public static final String TABLE_NAME = "favicons";
 
         public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "favicons");
 
@@ -411,17 +405,17 @@ public class BrowserContract {
 
         // Minimum fields required to create a reading list item.
         public static final String[] REQUIRED_FIELDS = { Bookmarks.URL, Bookmarks.TITLE };
 
         public static final String TABLE_NAME = "reading_list";
     }
 
     @RobocopTarget
-    public static final class TopSites implements CommonColumns, URLColumns, SuggestedSitesColumns {
+    public static final class TopSites implements CommonColumns, URLColumns {
         private TopSites() {}
 
         public static final int TYPE_BLANK = 0;
         public static final int TYPE_TOP = 1;
         public static final int TYPE_PINNED = 2;
         public static final int TYPE_SUGGESTED = 3;
 
         public static final String BOOKMARK_ID = "bookmark_id";
@@ -436,14 +430,14 @@ public class BrowserContract {
         public static final String CONTENT_TYPE = "vnd.android.cursor.dir/searchhistory";
         public static final String QUERY = "query";
         public static final String TABLE_NAME = "searchhistory";
 
         public static final Uri CONTENT_URI = Uri.withAppendedPath(SEARCH_HISTORY_AUTHORITY_URI, "searchhistory");
     }
 
     @RobocopTarget
-    public static final class SuggestedSites implements CommonColumns, URLColumns, SuggestedSitesColumns {
+    public static final class SuggestedSites implements CommonColumns, URLColumns {
         private SuggestedSites() {}
 
         public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "suggestedsites");
     }
 }
--- a/mobile/android/base/db/BrowserDB.java
+++ b/mobile/android/base/db/BrowserDB.java
@@ -16,16 +16,17 @@ import org.mozilla.gecko.favicons.decode
 import org.mozilla.gecko.mozglue.RobocopTarget;
 import org.mozilla.gecko.util.StringUtils;
 
 import android.content.ContentResolver;
 import android.content.ContentValues;
 import android.content.Context;
 import android.database.ContentObserver;
 import android.database.Cursor;
+import android.graphics.Color;
 import android.graphics.drawable.BitmapDrawable;
 
 /**
  * A utility wrapper for accessing a static {@link LocalBrowserDB},
  * manually initialized with a particular profile, and similarly
  * managing a static instance of {@link SuggestedSites}.
  *
  * Be careful using this class: if you're not BrowserApp, you probably
@@ -247,9 +248,26 @@ public class BrowserDB {
     @RobocopTarget
     public static Cursor getBookmarkForUrl(ContentResolver cr, String url) {
         return sDb.getBookmarkForUrl(cr, url);
     }
 
     public static void setEnableContentProviders(boolean enableContentProviders) {
         sAreContentProvidersEnabled = enableContentProviders;
     }
+
+    public static boolean hasSuggestedImageUrl(String url) {
+        return sSuggestedSites.contains(url);
+    }
+
+    public static String getSuggestedImageUrlForUrl(String url) {
+        return sSuggestedSites.getImageUrlForUrl(url);
+    }
+
+    public static int getSuggestedBackgroundColorForUrl(String url) {
+        final String bgColor = sSuggestedSites.getBackgroundColorForUrl(url);
+        if (bgColor != null) {
+            return Color.parseColor(bgColor);
+        }
+
+        return 0;
+    }
 }
--- a/mobile/android/base/db/SuggestedSites.java
+++ b/mobile/android/base/db/SuggestedSites.java
@@ -75,82 +75,71 @@ public class SuggestedSites {
     // Locale used to generate the current suggested sites.
     public static final String PREF_SUGGESTED_SITES_LOCALE = "suggestedSites.locale";
 
     // File in profile dir with the list of suggested sites.
     private static final String FILENAME = "suggestedsites.json";
 
     private static final String[] COLUMNS = new String[] {
         BrowserContract.SuggestedSites._ID,
-        BrowserContract.SuggestedSites.TRACKING_ID,
         BrowserContract.SuggestedSites.URL,
         BrowserContract.SuggestedSites.TITLE,
-        BrowserContract.SuggestedSites.IMAGEURL,
-        BrowserContract.SuggestedSites.BGCOLOR
     };
 
-    private static final String JSON_KEY_TRACKING_ID = "trackingid";
     private static final String JSON_KEY_URL = "url";
     private static final String JSON_KEY_TITLE = "title";
     private static final String JSON_KEY_IMAGE_URL = "imageurl";
     private static final String JSON_KEY_BG_COLOR = "bgcolor";
 
     private static class Site {
-        public final String trackingId;
         public final String url;
         public final String title;
         public final String imageUrl;
         public final String bgColor;
 
         public Site(JSONObject json) throws JSONException {
-            this.trackingId = json.isNull(JSON_KEY_TRACKING_ID) ? null : json.getString(JSON_KEY_TRACKING_ID);
             this.url = json.getString(JSON_KEY_URL);
             this.title = json.getString(JSON_KEY_TITLE);
             this.imageUrl = json.getString(JSON_KEY_IMAGE_URL);
             this.bgColor = json.getString(JSON_KEY_BG_COLOR);
 
             validate();
         }
 
-        public Site(String trackingId, String url, String title, String imageUrl, String bgColor) {
-            this.trackingId = trackingId;
+        public Site(String url, String title, String imageUrl, String bgColor) {
             this.url = url;
             this.title = title;
             this.imageUrl = imageUrl;
             this.bgColor = bgColor;
 
             validate();
         }
 
         private void validate() {
-            // Site instances must have non-empty values for all properties except IDs.
+            // Site instances must have non-empty values for all properties.
             if (TextUtils.isEmpty(url) ||
                 TextUtils.isEmpty(title) ||
                 TextUtils.isEmpty(imageUrl) ||
                 TextUtils.isEmpty(bgColor)) {
                 throw new IllegalStateException("Suggested sites must have a URL, title, " +
                                                 "image URL, and background color.");
             }
         }
 
         @Override
         public String toString() {
-            return "{ id = " + trackingId + "\n" +
-                     "url = " + url + "\n" +
+            return "{ url = " + url + "\n" +
                      "title = " + title + "\n" +
                      "imageUrl = " + imageUrl + "\n" +
                      "bgColor = " + bgColor + " }";
         }
 
         public JSONObject toJSON() throws JSONException {
             final JSONObject json = new JSONObject();
 
-            // If trackingId is null, the key is not added.
-            json.put(JSON_KEY_TRACKING_ID, trackingId);
-
             json.put(JSON_KEY_URL, url);
             json.put(JSON_KEY_TITLE, title);
             json.put(JSON_KEY_IMAGE_URL, imageUrl);
             json.put(JSON_KEY_BG_COLOR, bgColor);
 
             return json;
         }
     }
@@ -496,29 +485,40 @@ public class SuggestedSites {
             }
 
             if (excludeUrls != null && excludeUrls.contains(site.url)) {
                 continue;
             }
 
             final RowBuilder row = cursor.newRow();
             row.add(-1);
-            row.add(site.trackingId);
             row.add(site.url);
             row.add(site.title);
-            row.add(site.imageUrl);
-            row.add(site.bgColor);
         }
 
         cursor.setNotificationUri(context.getContentResolver(),
                                   BrowserContract.SuggestedSites.CONTENT_URI);
 
         return cursor;
     }
 
+    public boolean contains(String url) {
+        return (getSiteForUrl(url) != null);
+    }
+
+    public String getImageUrlForUrl(String url) {
+        final Site site = getSiteForUrl(url);
+        return (site != null ? site.imageUrl : null);
+    }
+
+    public String getBackgroundColorForUrl(String url) {
+        final Site site = getSiteForUrl(url);
+        return (site != null ? site.bgColor : null);
+    }
+
     private Set<String> loadBlacklist() {
         Log.d(LOGTAG, "Loading blacklisted suggested sites from SharedPreferences.");
         final Set<String> blacklist = new HashSet<String>();
 
         final SharedPreferences preferences = GeckoSharedPrefs.forProfile(context);
         final String sitesString = preferences.getString(PREF_SUGGESTED_SITES_HIDDEN, null);
 
         if (sitesString != null) {
--- a/mobile/android/base/db/TopSitesCursorWrapper.java
+++ b/mobile/android/base/db/TopSitesCursorWrapper.java
@@ -33,34 +33,27 @@ public class TopSitesCursorWrapper imple
 
     private static final String[] columnNames = new String[] {
         TopSites._ID,
         TopSites.URL,
         TopSites.TITLE,
         TopSites.BOOKMARK_ID,
         TopSites.HISTORY_ID,
         TopSites.TYPE,
-        TopSites.IMAGEURL,
-        TopSites.BGCOLOR,
-        TopSites.TRACKING_ID,
     };
 
     private static final Map<String, Integer> columnIndexes =
             new HashMap<String, Integer>(columnNames.length);
 
     static {
         for (int i = 0; i < columnNames.length; i++) {
             columnIndexes.put(columnNames[i], i);
         }
     }
 
-    private static final int INDEX_TRACKING_ID = columnIndexes.get(TopSites.TRACKING_ID);
-    private static final int INDEX_IMAGEURL = columnIndexes.get(TopSites.IMAGEURL);
-    private static final int INDEX_BGCOLOR = columnIndexes.get(TopSites.BGCOLOR);
-
     // Maps column indexes from the wrapper to the cursor's.
     private SparseIntArray topIndexes;
     private SparseIntArray pinnedIndexes;
     private SparseIntArray suggestedIndexes;
 
     // Type of content in the current position
     private RowType currentRowType;
 
@@ -242,33 +235,22 @@ public class TopSitesCursorWrapper imple
                 break;
 
             case SUGGESTED:
                 map = suggestedIndexes;
                 break;
         }
 
         if (map != null) {
-            // Only look up suggested columns on suggested cursors.
-            if (currentRowType != RowType.SUGGESTED && isSuggestedSiteColumn(columnIndex)) {
-                return -1;
-            }
-
             return map.get(columnIndex, -1);
         }
 
         return -1;
     }
 
-    private boolean isSuggestedSiteColumn(int columnIndex) {
-        return columnIndex == INDEX_IMAGEURL ||
-               columnIndex == INDEX_BGCOLOR ||
-               columnIndex == INDEX_TRACKING_ID;
-    }
-
     @Override
     public int getPosition() {
         return currentPosition;
     }
 
     @Override
     public int getCount() {
         return count;
--- a/mobile/android/base/home/TopSitesPanel.java
+++ b/mobile/android/base/home/TopSitesPanel.java
@@ -520,22 +520,25 @@ public class TopSitesPanel extends HomeF
             // Thumbnails are delivered late, so we can't short-circuit any
             // sooner than this. But we can avoid a duplicate favicon
             // fetch...
             if (!updated) {
                 debug("bindView called twice for same values; short-circuiting.");
                 return;
             }
 
+            // Make sure we query suggested images without the user-entered wrapper.
+            final String decodedUrl = StringUtils.decodeUserEnteredUrl(url);
+
             // Suggested images have precedence over thumbnails, no need to wait
             // for them to be loaded. See: CursorLoaderCallbacks.onLoadFinished()
-            final String imageUrl = cursor.getString(cursor.getColumnIndexOrThrow(TopSites.IMAGEURL));
+            final String imageUrl = BrowserDB.getSuggestedImageUrlForUrl(decodedUrl);
             if (!TextUtils.isEmpty(imageUrl)) {
-                final String bgColor = cursor.getString(cursor.getColumnIndexOrThrow(TopSites.BGCOLOR));
-                view.displayThumbnail(imageUrl, Color.parseColor(bgColor));
+                final int bgColor = BrowserDB.getSuggestedBackgroundColorForUrl(decodedUrl);
+                view.displayThumbnail(imageUrl, bgColor);
                 return;
             }
 
             // If thumbnails are still being loaded, don't try to load favicons
             // just yet. If we sent in a thumbnail, we're done now.
             if (mThumbnailInfos == null || thumbnail != null) {
                 return;
             }
@@ -616,21 +619,20 @@ public class TopSitesPanel extends HomeF
             if (!c.moveToFirst()) {
                 return;
             }
 
             final ArrayList<String> urls = new ArrayList<String>();
             int i = 1;
             do {
                 final String url = c.getString(col);
-                final String imageUrl = c.getString(c.getColumnIndexOrThrow(TopSites.IMAGEURL));
 
                 // Only try to fetch thumbnails for non-empty URLs that
                 // don't have an associated suggested image URL.
-                if (TextUtils.isEmpty(url) || !TextUtils.isEmpty(imageUrl)) {
+                if (TextUtils.isEmpty(url) || BrowserDB.hasSuggestedImageUrl(url)) {
                     continue;
                 }
 
                 urls.add(url);
             } while (i++ < mMaxGridEntries && c.moveToNext());
 
             if (urls.isEmpty()) {
                 // Short-circuit empty results to the UI.
--- a/mobile/android/tests/browser/junit3/src/TestSuggestedSites.java
+++ b/mobile/android/tests/browser/junit3/src/TestSuggestedSites.java
@@ -147,17 +147,16 @@ public class TestSuggestedSites extends 
     }
 
     private String generateSites(int n, String prefix) {
         JSONArray sites = new JSONArray();
 
         try {
             for (int i = 0; i < n; i++) {
                 JSONObject site = new JSONObject();
-                site.put("trackingid", prefix + "trackingId" + i);
                 site.put("url", prefix + "url" + i);
                 site.put("title", prefix + "title" + i);
                 site.put("imageurl", prefix + "imageUrl" + i);
                 site.put("bgcolor", prefix + "bgColor" + i);
 
                 sites.put(site);
             }
         } catch (Exception e) {
@@ -234,52 +233,31 @@ public class TestSuggestedSites extends 
 
         // Empty string = empty cursor
         checkCursorCount("", 0);
 
         // Invalid json string = empty cursor
         checkCursorCount("{ broken: }", 0);
     }
 
-    public void testNoTrackingId() {
-        String content = "[{ url: \"url\", title: \"title\", imageurl: \"imageurl\", bgcolor: \"bgcolor\" }]";
-        resources.setSuggestedSitesResource(content);
-
-        Cursor c = new SuggestedSites(context).get(DEFAULT_LIMIT);
-        assertEquals(1, c.getCount());
-        c.moveToNext();
-
-        String trackingId = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.TRACKING_ID));
-        assertNull(trackingId);
-    }
-
     public void testCursorContent() {
         resources.setSuggestedSitesResource(generateSites(3));
 
         Cursor c = new SuggestedSites(context).get(DEFAULT_LIMIT);
         assertEquals(3, c.getCount());
 
         c.moveToPosition(-1);
         while (c.moveToNext()) {
             int position = c.getPosition();
 
-            String trackingId = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.TRACKING_ID));
-            assertEquals("trackingId" + position, trackingId);
-
             String url = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.URL));
             assertEquals("url" + position, url);
 
             String title = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.TITLE));
             assertEquals("title" + position, title);
-
-            String imageUrl = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.IMAGEURL));
-            assertEquals("imageUrl" + position, imageUrl);
-
-            String bgColor = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.BGCOLOR));
-            assertEquals("bgColor" + position, bgColor);
         }
 
         c.close();
     }
 
     public void testExcludeUrls() {
         resources.setSuggestedSitesResource(generateSites(6));
 
@@ -356,16 +334,50 @@ public class TestSuggestedSites extends 
                                         .commit();
 
         c = new SuggestedSites(context).get(DEFAULT_LIMIT);
         assertNotNull(c);
         assertEquals(0, c.getCount());
         c.close();
     }
 
+    public void testImageUrlAndBgColor() {
+        final int count = 3;
+        resources.setSuggestedSitesResource(generateSites(count));
+
+        SuggestedSites suggestedSites = new SuggestedSites(context);
+
+        // Suggested sites hasn't been loaded yet.
+        for (int i = 0; i < count; i++) {
+            String url = "url" + i;
+            assertFalse(suggestedSites.contains(url));
+            assertNull(suggestedSites.getImageUrlForUrl(url));
+            assertNull(suggestedSites.getBackgroundColorForUrl(url));
+        }
+
+        Cursor c = suggestedSites.get(DEFAULT_LIMIT);
+        c.moveToPosition(-1);
+
+        // We should have cached results after the get() call.
+        while (c.moveToNext()) {
+            String url = c.getString(c.getColumnIndexOrThrow(BrowserContract.SuggestedSites.URL));
+            assertTrue(suggestedSites.contains(url));
+            assertEquals("imageUrl" + c.getPosition(),
+                         suggestedSites.getImageUrlForUrl(url));
+            assertEquals("bgColor" + c.getPosition(),
+                         suggestedSites.getBackgroundColorForUrl(url));
+        }
+        c.close();
+
+        // No valid values for unknown URLs.
+        assertFalse(suggestedSites.contains("foo"));
+        assertNull(suggestedSites.getImageUrlForUrl("foo"));
+        assertNull(suggestedSites.getBackgroundColorForUrl("foo"));
+    }
+
     public void testLocaleChanges() {
         resources.setSuggestedSitesResource(generateSites(3));
 
         SuggestedSites suggestedSites = new SuggestedSites(context);
 
         // Initial load with predefined locale
         Cursor c = suggestedSites.get(DEFAULT_LIMIT, Locale.UK);
         assertEquals(3, c.getCount());