Bug 1254797 - Intersperse blank sites between pinned sites if needed r=rnewman
authorAndrzej Hunt <andrzej@ahunt.org>
Tue, 15 Mar 2016 14:41:15 -0700
changeset 289583 54080662c8491231102e0e81144b8fa6ddcee3d7
parent 289582 c1f7b417f9e26427dc17e870d0a741bf4d231764
child 289584 da1a66f1c4b74c4bd778e91b9589d73763dddfe3
push id30107
push usercbook@mozilla.com
push dateTue, 22 Mar 2016 10:00:23 +0000
treeherdermozilla-central@3587b25bae30 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1254797
milestone48.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 1254797 - Intersperse blank sites between pinned sites if needed r=rnewman 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
@@ -783,21 +783,56 @@ public class BrowserProvider extends Sha
                                                             new String[] {
                                                                     suggestedSitesCursor.getString(idColumnIndex),
                                                                     suggestedSitesCursor.getString(urlColumnIndex),
                                                                     suggestedSitesCursor.getString(titleColumnIndex)
                                                             });
         }
         suggestedSitesCursor.close();
 
+        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 + ", " +
@@ -831,16 +866,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.