Bug 1377295 - Migrate or re-use pinned sites from old top-sites panel. r=mcomella draft
authorChenxia Liu <liuche@mozilla.com>
Tue, 18 Jul 2017 18:43:35 -0700
changeset 611366 98d1ad597319a32353d31fb429bdbc4535f1bcd5
parent 610649 dece50457378ac4934afe9fb3c2a8054e8894588
child 638142 9c72f95ea1602a0b9dabce443eb206a432bb62d0
push id69201
push usercliu@mozilla.com
push dateWed, 19 Jul 2017 16:38:33 +0000
reviewersmcomella
bugs1377295
milestone56.0a1
Bug 1377295 - Migrate or re-use pinned sites from old top-sites panel. r=mcomella MozReview-Commit-ID: 6i3Cam8un45
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -959,29 +959,25 @@ public class BrowserProvider extends Sha
 
         if (gridLimitParam == null) {
             suggestedGridLimit = getContext().getResources().getInteger(R.integer.number_of_top_sites);
         } else {
             suggestedGridLimit = Integer.parseInt(gridLimitParam, 10);
         }
 
         // We have two types of pinned sites, positioned and non-positioned. Positioned pins are used
-        // by regular Top Sites, where position in the grid is of importance. Non-positioned pins are
-        // used by Activity Stream Top Sites, where pins are displayed in front of other top site items.
-        // Non-positioned pins all have the same special position value which is used to identify them.
-        // An alternative to this is creating a separate special folder for non-positioned pins, introducing
-        // a database migration, adjusting sync code, etc. While on some level this might
-        // be a cleaner solution, a "position hack" is simpler to implement and manage over time in light
-        // of A-S being either a likely replacement for regular Top Sites, or being scrapped.
+        // by regular Top Sites, where position in the grid is of importance.
+        // Activity Stream Top Sites pins are displayed in front of other top site items - they inherit
+        // the Top Sites positioned pins, and new pins are created as non-positioned.
+        // Since we will be permanently moving to Activity Stream, if a pin is modified in Activity Stream, it is
+        // lost from (legacy) Top Sites.
         String pinnedSitesFromClause = "FROM " + TABLE_BOOKMARKS + " WHERE " +
                 Bookmarks.PARENT + " = " + Bookmarks.FIXED_PINNED_LIST_ID +
                 " AND " + Bookmarks.IS_DELETED + " IS NOT 1";
-        if (nonPositionedPins != null) {
-            pinnedSitesFromClause += " AND " + Bookmarks.POSITION + " = " + Bookmarks.FIXED_AS_PIN_POSITION;
-        } else {
+        if (nonPositionedPins == null) {
             pinnedSitesFromClause += " AND " + Bookmarks.POSITION + " != " + Bookmarks.FIXED_AS_PIN_POSITION;
         }
 
         // Ideally we'd use a recursive CTE to generate our sequence, e.g. something like this worked at one point:
         // " WITH RECURSIVE" +
         // " cnt(x) AS (VALUES(1) UNION ALL SELECT x+1 FROM cnt WHERE x < 6)" +
         // However that requires SQLite >= 3.8.3 (available on Android >= 5.0), so in the meantime
         // we use a temporary numbers table.
--- a/mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
@@ -1742,34 +1742,31 @@ public class LocalBrowserDB extends Brow
 
         cr.insert(mBookmarksUriWithProfile, values);
     }
 
     @Override
     public void unpinSiteForAS(ContentResolver cr, String url) {
         cr.delete(mBookmarksUriWithProfile,
                 Bookmarks.PARENT + " == ? AND " +
-                Bookmarks.POSITION + " == ? AND " +
                 Bookmarks.URL + " = ?",
                 new String[] {
                         String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID),
-                        String.valueOf(Bookmarks.FIXED_AS_PIN_POSITION),
                         url
                 });
     }
 
     @Override
     public boolean isPinnedForAS(ContentResolver cr, String url) {
         final Cursor c = cr.query(bookmarksUriWithLimit(1),
                 new String[] { Bookmarks._ID },
-                Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " = ? AND " + Bookmarks.POSITION + " = ?",
+                Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " = ?",
                 new String[] {
                         url,
                         String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID),
-                        String.valueOf(Bookmarks.FIXED_AS_PIN_POSITION)
                 }, null);
 
         if (c == null) {
             throw new IllegalStateException("Null cursor in isPinnedByUrl");
         }
 
         try {
             return c.getCount() > 0;