Bug 1386052: Join PageMetadata table with Top Sites query. r=grisha draft
authorMichael Comella <michael.l.comella@gmail.com>
Tue, 01 Aug 2017 16:02:50 -0700
changeset 620036 13ffd3a70c2d279de665197040b6373445fbb5eb
parent 620035 d3568dd714b1489992af8e361f94756102bf4f51
child 620088 a5ea44262c9b60c3bd829a84861a86b0d3d3ab37
push id71897
push usermichael.l.comella@gmail.com
push dateWed, 02 Aug 2017 23:58:29 +0000
reviewersgrisha
bugs1386052
milestone56.0a1
Bug 1386052: Join PageMetadata table with Top Sites query. r=grisha This bug does not request that this join is used so I tested this by adding a breakpoint in StreamRecyclerAdapter.swapTopSites cursor and querying the cursor for the desired fields directly. I also verified this does not crash when accessing the old top sites and AS top sites. Since I now SELECT a subset of columns (rather than *), I verified I've SELECTed all the used columns: - For AS, we bind in TopSite.fromCursor: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/TopSite.java#21 - For old top sites, we bind in TopSitesPanel.CursorLoaderCallbacks.onLoadFinished. This fans out to three methods: mListAdapter.swapCursor, mGridAdapter.swapCursor, and updateUiFromCursor, the latter which only gets the count. https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java#692 - mListAdapter = VisitedAdapter, mGridAdapter = TopSitesGridAdapter, via TopSitesPanel.onActivityCreated: https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java#292 - VisitedAdapter.bindView: https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java#549 - TopSitesGridAdapter.bindView: https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/mobile/android/base/java/org/mozilla/gecko/home/TopSitesPanel.java#620 - Both adapters don't use any columns I haven't specified. They also call out to TwoLinePageRow.updateFromCursor: https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java#318 Which also doesn't use new columns. MozReview-Commit-ID: 9bdeHWLWQbh
mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ -618,16 +618,17 @@ public class BrowserContract {
         public static final int TYPE_SUGGESTED = 3;
 
         @IntDef({TYPE_BLANK, TYPE_TOP, TYPE_PINNED, TYPE_SUGGESTED})
         public @interface TopSiteType {}
 
         public static final String BOOKMARK_ID = "bookmark_id";
         public static final String HISTORY_ID = "history_id";
         public static final String TYPE = "type";
+        public static final String PAGE_METADATA_JSON = "page_metadata_json";
 
         public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "topsites");
     }
 
     public static class Highlights {
         public static final String BOOKMARK_ID = "bookmark_id";
         public static final String HISTORY_ID = "history_id";
 
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -1098,16 +1098,17 @@ public class BrowserProvider extends Sha
         try {
             db.execSQL("DROP TABLE IF EXISTS " + TABLE_TOPSITES);
 
             db.execSQL("CREATE TEMP TABLE " + TABLE_TOPSITES + " AS" +
                        " SELECT " +
                        Bookmarks._ID + ", " +
                        Combined.BOOKMARK_ID + ", " +
                        Combined.HISTORY_ID + ", " +
+                       Combined.HISTORY_GUID + ", " +
                        Bookmarks.URL + ", " +
                        Bookmarks.TITLE + ", " +
                        Combined.HISTORY_ID + ", " +
                        TopSites.TYPE_TOP + " AS " + TopSites.TYPE +
                        " FROM " + Combined.VIEW_NAME +
                        " WHERE " + ignoreForTopSitesWhereClause +
                        " ORDER BY " + BrowserContract.getCombinedFrecencySortOrder(true, false) +
                        " LIMIT " + totalLimit,
@@ -1118,16 +1119,17 @@ public class BrowserProvider extends Sha
                 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 + ", " +
                            " NULL " + " AS " + Combined.BOOKMARK_ID + ", " +
                            " -1 AS " + Combined.HISTORY_ID + ", " +
+                           " NULL AS " + Combined.HISTORY_GUID + ", " +
                            Bookmarks.URL + ", " +
                            Bookmarks.TITLE + ", " +
                            "NULL AS " + Combined.HISTORY_ID + ", " +
                            TopSites.TYPE_SUGGESTED + " as " + TopSites.TYPE +
                            " FROM ( " + suggestedSitesBuilder.toString() + " )" +
                            " WHERE " +
                            Bookmarks.URL + " NOT IN (SELECT url FROM " + TABLE_TOPSITES + ")" +
                            " AND " +
@@ -1141,68 +1143,91 @@ public class BrowserProvider extends Sha
                 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 + ", " +
+                           " NULL AS " + Combined.HISTORY_GUID + ", " +
                            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.
+            final String selectTopSites =
+                    "SELECT " +
+                    Bookmarks._ID + ", " +
+                    TopSites.BOOKMARK_ID + ", " +
+                    TopSites.HISTORY_ID + ", " +
+                    Combined.HISTORY_GUID + ", " +
+                    Bookmarks.URL + ", " +
+                    Bookmarks.TITLE + ", " +
+                    "COALESCE(" + Bookmarks.POSITION + ", " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") + " + " + suggestedGridLimit +
+                    ")" + " AS " + Bookmarks.POSITION + ", " +
+                    Combined.HISTORY_ID + ", " +
+                    TopSites.TYPE +
+                    " FROM " + TABLE_TOPSITES +
+                    " LEFT OUTER JOIN " + // TABLE_IDS +
+                    "(" + freeIDSubquery + ") AS id_results" +
+                    " ON " + DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") +
+                    " = " + DBUtils.qualifyColumn("id_results", "rowid") +
+
+                    " UNION ALL " +
+
+                    "SELECT " +
+                    Bookmarks._ID + ", " +
+                    Bookmarks._ID + " AS " + TopSites.BOOKMARK_ID + ", " +
+                    " -1 AS " + TopSites.HISTORY_ID + ", " +
+                    " NULL AS " + Combined.HISTORY_GUID + ", " +
+                    Bookmarks.URL + ", " +
+                    Bookmarks.TITLE + ", " +
+                    Bookmarks.POSITION + ", " +
+                    "NULL AS " + Combined.HISTORY_ID + ", " +
+                    TopSites.TYPE_PINNED + " as " + TopSites.TYPE +
+                    " " + pinnedSitesFromClause;
+
+            // In order to join the PageMetadata with our `SELECT ... UNION ALL SELECT ...` for top sites, the top sites
+            // SELECT must be a subquery (see https://stackoverflow.com/a/19110809/2219998).
             final SQLiteCursor c = (SQLiteCursor) db.rawQuery(
-                        "SELECT " +
-                        Bookmarks._ID + ", " +
-                        TopSites.BOOKMARK_ID + ", " +
-                        TopSites.HISTORY_ID + ", " +
-                        Bookmarks.URL + ", " +
-                        Bookmarks.TITLE + ", " +
-                        "COALESCE(" + Bookmarks.POSITION + ", " +
-                            DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") + " + " + suggestedGridLimit +
-                        ")" + " AS " + Bookmarks.POSITION + ", " +
-                        Combined.HISTORY_ID + ", " +
-                        TopSites.TYPE +
-                        " FROM " + TABLE_TOPSITES +
-                        " LEFT OUTER JOIN " + // TABLE_IDS +
-                        "(" + freeIDSubquery + ") AS id_results" +
-                        " ON " + DBUtils.qualifyColumn(TABLE_TOPSITES, "rowid") +
-                        " = " + DBUtils.qualifyColumn("id_results", "rowid") +
-
-                        " UNION ALL " +
-
-                        "SELECT " +
-                        Bookmarks._ID + ", " +
-                        Bookmarks._ID + " AS " + TopSites.BOOKMARK_ID + ", " +
-                        " -1 AS " + TopSites.HISTORY_ID + ", " +
-                        Bookmarks.URL + ", " +
-                        Bookmarks.TITLE + ", " +
-                        Bookmarks.POSITION + ", " +
-                        "NULL AS " + Combined.HISTORY_ID + ", " +
-                        TopSites.TYPE_PINNED + " as " + TopSites.TYPE +
-                        " " + pinnedSitesFromClause +
-
-                        // In case position is non-unique (as in Activity Stream pins, whose position
-                        // is always zero), we need to ensure we get stable ordering.
-                        " ORDER BY " + Bookmarks.POSITION + ", " + Bookmarks.URL,
-
-                        null);
+                    // Specify a projection so we don't take the whole PageMetadata table, or the joining columns, with us.
+                    "SELECT " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, Bookmarks._ID) + ", " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, TopSites.BOOKMARK_ID) + ", " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, TopSites.HISTORY_ID) + ", " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, Bookmarks.URL) + ", " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, Bookmarks.TITLE) + ", " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, Bookmarks.POSITION) + ", " +
+                    DBUtils.qualifyColumn(TABLE_TOPSITES, TopSites.TYPE) + ", " +
+                    PageMetadata.JSON + " AS " + TopSites.PAGE_METADATA_JSON +
+
+                    " FROM (" + selectTopSites + ") AS " + TABLE_TOPSITES +
+
+                    " LEFT OUTER JOIN " + TABLE_PAGE_METADATA + " ON " +
+                            DBUtils.qualifyColumn(TABLE_TOPSITES, Combined.HISTORY_GUID) + " = " +
+                            DBUtils.qualifyColumn(TABLE_PAGE_METADATA, PageMetadata.HISTORY_GUID) +
+
+                    // In case position is non-unique (as in Activity Stream pins, whose position
+                    // is always zero), we need to ensure we get stable ordering.
+                    " ORDER BY " + Bookmarks.POSITION + ", " + Bookmarks.URL,
+
+                    null);
 
             c.setNotificationUri(getContext().getContentResolver(),
                                  BrowserContract.AUTHORITY_URI);
 
             // Force the cursor to be compiled and the cursor-window filled now:
             // (A) without compiling the cursor now we won't have access to the TEMP table which
             // is removed as soon as we close our connection.
             // (B) this might also mitigate the situation causing this crash where we're accessing