Bug 1254797 - Intersperse blank sites between pinned sites if needed. r=rnewman, a=ritu
authorAndrzej Hunt <andrzej@ahunt.org>
Tue, 15 Mar 2016 14:41:15 -0700
changeset 310348 d6ec426e0c96a9811881b1a0d5b43fc0707ea7a9
parent 310347 87e35794cf880fc8561ae2c076dbcf3064c4de1b
child 310349 876a1f819d83ef8035e579dd9247693c9526875b
push id9338
push usercbook@mozilla.com
push dateWed, 30 Mar 2016 13:58:47 +0000
treeherdermozilla-aurora@876a1f819d83 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, ritu
bugs1254797
milestone47.0a2
Bug 1254797 - Intersperse blank sites between pinned sites if needed. r=rnewman, a=ritu This fixes an edge case that is most likely to happen to new users if they pin a site followed by removing one or more suggested sites. This results in the topsites table containing less sites than needed, leading to some pinned sites being displayed in a higher than expected position. This also broke unpinning since our code assumes that a topsites physical position corresponds to its DB position (which prior to this patch was not the case). MozReview-Commit-ID: JgTUa55eXnz
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -782,21 +782,56 @@ public class BrowserProvider extends Sha
             suggestedSiteArgs = DBUtils.appendSelectionArgs(suggestedSiteArgs,
                                                             new String[] {
                                                                     suggestedSitesCursor.getString(idColumnIndex),
                                                                     suggestedSitesCursor.getString(urlColumnIndex),
                                                                     suggestedSitesCursor.getString(titleColumnIndex)
                                                             });
         }
 
+        boolean hasPreparedBlankTiles = false;
+
+        // We can somewhat reduce the number of blanks we produce by eliminating suggested sites.
+        // We do the actual limit calculation in SQL (since we need to take into account the number
+        // of pinned sites too), but this might avoid producing 5 or so additional blank tiles
+        // that would then need to be filtered out.
+        final int maxBlanksNeeded = suggestedGridLimit - suggestedSitesCursor.getCount();
+
+        final StringBuilder blanksBuilder = new StringBuilder();
+        for (int i = 0; i < maxBlanksNeeded; i++) {
+            if (hasPreparedBlankTiles) {
+                blanksBuilder.append(" UNION ALL");
+            } else {
+                hasPreparedBlankTiles = true;
+            }
+
+            blanksBuilder.append(" SELECT" +
+                                 " -1 AS " + Bookmarks._ID + "," +
+                                 " '' AS " + Bookmarks.URL + "," +
+                                 " '' AS " + Bookmarks.TITLE);
+        }
+
+
+
         // To restrict suggested sites to the grid, we simply subtract the number of topsites (which have already had
         // the pinned sites filtered out), and the number of pinned sites.
         // SQLite completely ignores negative limits, hence we need to manually limit to 0 in this case.
         final String suggestedLimitClause = " LIMIT MAX(0, (" + suggestedGridLimit + " - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ") - (SELECT COUNT(*) " + pinnedSitesFromClause + "))) ";
 
+        // Pinned site positions are zero indexed, but we need to get the maximum 1-indexed position.
+        // Hence to correctly calculate the largest pinned position (which should be 0 if there are
+        // no sites, or 1-6 if we have at least one pinned site), we coalesce the DB position (0-5)
+        // with -1 to represent no-sites, which allows us to directly add 1 to obtain the expected value
+        // regardless of whether a position was actually retrieved.
+        final String blanksLimitClause = " LIMIT MAX(0, " +
+                                         "COALESCE((SELECT " + Bookmarks.POSITION + " " + pinnedSitesFromClause + "), -1) + 1" +
+                                         " - (SELECT COUNT(*) " + pinnedSitesFromClause + ")" +
+                                         " - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ")" +
+                                         ")";
+
         db.beginTransaction();
         try {
             db.execSQL("DROP TABLE IF EXISTS " + TABLE_TOPSITES);
 
             db.execSQL("CREATE TEMP TABLE " + TABLE_TOPSITES + " AS" +
                        " SELECT " +
                        Bookmarks._ID + ", " +
                        Combined.BOOKMARK_ID + ", " +
@@ -830,16 +865,33 @@ public class BrowserProvider extends Sha
                            Bookmarks.URL + " NOT IN (SELECT url FROM " + TABLE_TOPSITES + ")" +
                            " AND " +
                            Bookmarks.URL + " NOT IN (SELECT url " + pinnedSitesFromClause + ")" +
                            suggestedLimitClause + " )",
 
                            suggestedSiteArgs);
             }
 
+            if (hasPreparedBlankTiles) {
+                db.execSQL("INSERT INTO " + TABLE_TOPSITES +
+                           // We need to LIMIT _after_ selecting the relevant suggested sites, which requires us to
+                           // use an additional internal subquery, since we cannot LIMIT a subquery that is part of UNION ALL.
+                           // Hence the weird SELECT * FROM (SELECT ...relevant suggested sites... LIMIT ?)
+                           " SELECT * FROM (SELECT " +
+                           Bookmarks._ID + ", " +
+                           Bookmarks._ID + " AS " + Combined.BOOKMARK_ID + ", " +
+                           " -1 AS " + Combined.HISTORY_ID + ", " +
+                           Bookmarks.URL + ", " +
+                           Bookmarks.TITLE + ", " +
+                           "NULL AS " + Combined.HISTORY_ID + ", " +
+                           TopSites.TYPE_BLANK + " as " + TopSites.TYPE +
+                           " FROM ( " + blanksBuilder.toString() + " )" +
+                           blanksLimitClause + " )");
+            }
+
             // If we retrieve more topsites than we have free positions for in the freeIdSubquery,
             // we will have topsites that don't receive a position when joining TABLE_TOPSITES
             // with freeIdSubquery. Hence we need to coalesce the position with a generated position.
             // We know that the difference in positions will be at most suggestedGridLimit, hence we
             // can add that to the rowid to generate a safe position.
             // I.e. if we have 6 pinned sites then positions 0..5 are filled, the JOIN results in
             // the first N rows having positions 6..(N+6), so row N+1 should receive a position that is at
             // least N+1+6, which is equal to rowid + 6.