From 87d6b65e7be6ed6883b36ded8d8ac6773f7d5bb8 Mon Sep 17 00:00:00 2001 From: Chris Kitching Date: Mon, 7 Jul 2014 14:52:37 -0700 Subject: [PATCH] Bug 1024289: Part 1: remove redundant subquery from combined view and add upgrade path. r=rnewman --- .../base/db/BrowserDatabaseHelper.java | 191 +++++++++++------- 1 file changed, 121 insertions(+), 70 deletions(-) diff --git a/mobile/android/base/db/BrowserDatabaseHelper.java b/mobile/android/base/db/BrowserDatabaseHelper.java index dd7c5745ee77..682f88931074 100644 --- a/mobile/android/base/db/BrowserDatabaseHelper.java +++ b/mobile/android/base/db/BrowserDatabaseHelper.java @@ -34,7 +34,7 @@ import android.util.Log; final class BrowserDatabaseHelper extends SQLiteOpenHelper { private static final String LOGTAG = "GeckoBrowserDBHelper"; - public static final int DATABASE_VERSION = 18; + public static final int DATABASE_VERSION = 19; public static final String DATABASE_NAME = "browser.db"; final protected Context mContext; @@ -653,73 +653,108 @@ final class BrowserDatabaseHelper extends SQLiteOpenHelper { " ON " + Combined.FAVICON_ID + " = " + qualifyColumn(TABLE_FAVICONS, Favicons._ID)); } - private void createCombinedViewOn16(SQLiteDatabase db) { - debug("Creating " + VIEW_COMBINED + " view"); + private void createCombinedViewOn19(SQLiteDatabase db) { + /* + The v19 combined view removes the redundant subquery from the v16 + combined view and reorders the columns as necessary to prevent this + from breaking any code that might be referencing columns by index. + + The rows in the ensuing view are, in order: + + Combined.BOOKMARK_ID + Combined.HISTORY_ID + Combined._ID (always 0) + Combined.URL + Combined.TITLE + Combined.VISITS + Combined.DISPLAY + Combined.DATE_LAST_VISITED + Combined.FAVICON_ID + + We need to return an _id column because CursorAdapter requires it for its + default implementation for the getItemId() method. However, since + we're not using this feature in the parts of the UI using this view, + we can just use 0 for all rows. + */ db.execSQL("CREATE VIEW IF NOT EXISTS " + VIEW_COMBINED + " AS" + - " SELECT " + Combined.BOOKMARK_ID + ", " + - Combined.HISTORY_ID + ", " + - // We need to return an _id column because CursorAdapter requires it for its - // default implementation for the getItemId() method. However, since - // we're not using this feature in the parts of the UI using this view, - // we can just use 0 for all rows. - "0 AS " + Combined._ID + ", " + - Combined.URL + ", " + - Combined.TITLE + ", " + - Combined.VISITS + ", " + - Combined.DISPLAY + ", " + - Combined.DATE_LAST_VISITED + ", " + - Combined.FAVICON_ID + - " FROM (" + - // Bookmarks without history. - " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + ", " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " AS " + Combined.URL + ", " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + " AS " + Combined.TITLE + ", " + - "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " + - Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " + - Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " + - "-1 AS " + Combined.HISTORY_ID + ", " + - "-1 AS " + Combined.VISITS + ", " + - "-1 AS " + Combined.DATE_LAST_VISITED + ", " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.FAVICON_ID) + " AS " + Combined.FAVICON_ID + - " FROM " + TABLE_BOOKMARKS + - " WHERE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " = " + Bookmarks.TYPE_BOOKMARK + " AND " + - // Ignore pinned bookmarks. - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " <> " + Bookmarks.FIXED_PINNED_LIST_ID + " AND " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " = 0 AND " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + - " NOT IN (SELECT " + History.URL + " FROM " + TABLE_HISTORY + ")" + - " UNION ALL" + + + // Bookmarks without history. + " SELECT " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " AS " + Combined.BOOKMARK_ID + "," + + "-1 AS " + Combined.HISTORY_ID + "," + + "0 AS " + Combined._ID + "," + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " AS " + Combined.URL + ", " + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + " AS " + Combined.TITLE + ", " + + "-1 AS " + Combined.VISITS + ", " + + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + + " WHEN " + Bookmarks.FIXED_READING_LIST_ID + " THEN " + + Combined.DISPLAY_READER + + " ELSE " + + Combined.DISPLAY_NORMAL + + " END AS " + Combined.DISPLAY + "," + + "-1 AS " + Combined.DATE_LAST_VISITED + "," + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.FAVICON_ID) + " AS " + Combined.FAVICON_ID + + " FROM " + TABLE_BOOKMARKS + + " WHERE " + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " = " + Bookmarks.TYPE_BOOKMARK + " AND " + + // Ignore pinned bookmarks. + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " <> " + Bookmarks.FIXED_PINNED_LIST_ID + " AND " + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " = 0 AND " + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + + " NOT IN (SELECT " + History.URL + " FROM " + TABLE_HISTORY + ")" + + " UNION ALL" + + // History with and without bookmark. - " SELECT " + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN " + - // Give pinned bookmarks a NULL ID so that they're not treated as bookmarks. We can't - // completely ignore them here because they're joined with history entries we care about. - "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " + - Bookmarks.FIXED_PINNED_LIST_ID + " THEN NULL ELSE " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " END " + - "ELSE NULL END AS " + Combined.BOOKMARK_ID + ", " + - qualifyColumn(TABLE_HISTORY, History.URL) + " AS " + Combined.URL + ", " + - // Prioritize bookmark titles over history titles, since the user may have - // customized the title for a bookmark. - "COALESCE(" + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + ", " + - qualifyColumn(TABLE_HISTORY, History.TITLE) +")" + " AS " + Combined.TITLE + ", " + - // Only use DISPLAY_READER if the matching bookmark entry inside reading - // list folder is not marked as deleted. - "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN CASE " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " + Bookmarks.FIXED_READING_LIST_ID + - " THEN " + Combined.DISPLAY_READER + " ELSE " + Combined.DISPLAY_NORMAL + " END ELSE " + - Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " + - qualifyColumn(TABLE_HISTORY, History._ID) + " AS " + Combined.HISTORY_ID + ", " + - qualifyColumn(TABLE_HISTORY, History.VISITS) + " AS " + Combined.VISITS + ", " + - qualifyColumn(TABLE_HISTORY, History.DATE_LAST_VISITED) + " AS " + Combined.DATE_LAST_VISITED + ", " + - qualifyColumn(TABLE_HISTORY, History.FAVICON_ID) + " AS " + Combined.FAVICON_ID + + " SELECT " + + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + + + // Give pinned bookmarks a NULL ID so that they're not treated as bookmarks. We can't + // completely ignore them here because they're joined with history entries we care about. + " WHEN 0 THEN " + + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + + " WHEN " + Bookmarks.FIXED_PINNED_LIST_ID + " THEN " + + "NULL " + + "ELSE " + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + + " END " + + "ELSE " + + "NULL " + + "END AS " + Combined.BOOKMARK_ID + "," + + qualifyColumn(TABLE_HISTORY, History._ID) + " AS " + Combined.HISTORY_ID + "," + + "0 AS " + Combined._ID + "," + + qualifyColumn(TABLE_HISTORY, History.URL) + " AS " + Combined.URL + "," + + + // Prioritize bookmark titles over history titles, since the user may have + // customized the title for a bookmark. + "COALESCE(" + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + ", " + + qualifyColumn(TABLE_HISTORY, History.TITLE) + + ") AS " + Combined.TITLE + "," + + qualifyColumn(TABLE_HISTORY, History.VISITS) + " AS " + Combined.VISITS + "," + + + // Only use DISPLAY_READER if the matching bookmark entry inside reading + // list folder is not marked as deleted. + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " " + + "WHEN 0 THEN " + + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " " + + "WHEN " + Bookmarks.FIXED_READING_LIST_ID + " THEN " + + Combined.DISPLAY_READER + " " + + "ELSE " + Combined.DISPLAY_NORMAL + " " + + "END " + + "ELSE " + + Combined.DISPLAY_NORMAL + " " + + "END AS " + Combined.DISPLAY + "," + + qualifyColumn(TABLE_HISTORY, History.DATE_LAST_VISITED) + " AS " + Combined.DATE_LAST_VISITED + "," + + qualifyColumn(TABLE_HISTORY, History.FAVICON_ID) + " AS " + Combined.FAVICON_ID + " FROM " + TABLE_HISTORY + " LEFT OUTER JOIN " + TABLE_BOOKMARKS + - " ON " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " = " + qualifyColumn(TABLE_HISTORY, History.URL) + - " WHERE " + qualifyColumn(TABLE_HISTORY, History.URL) + " IS NOT NULL AND " + - qualifyColumn(TABLE_HISTORY, History.IS_DELETED) + " = 0 AND (" + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " IS NULL OR " + - qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " = " + Bookmarks.TYPE_BOOKMARK + ") " + - ")"); + " ON " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) + " = " + qualifyColumn(TABLE_HISTORY, History.URL) + + " WHERE " + + qualifyColumn(TABLE_HISTORY, History.URL) + " IS NOT NULL AND " + + qualifyColumn(TABLE_HISTORY, History.IS_DELETED) + " = 0 AND " + + "(" + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " IS NULL OR " + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " = " + Bookmarks.TYPE_BOOKMARK + + ")" + ); debug("Creating " + VIEW_COMBINED_WITH_FAVICONS + " view"); @@ -742,7 +777,7 @@ final class BrowserDatabaseHelper extends SQLiteOpenHelper { createBookmarksWithFaviconsView(db); createHistoryWithFaviconsView(db); - createCombinedViewOn16(db); + createCombinedViewOn19(db); createOrUpdateSpecialFolder(db, Bookmarks.PLACES_FOLDER_GUID, R.string.bookmarks_folder_places, 0); @@ -1298,10 +1333,10 @@ final class BrowserDatabaseHelper extends SQLiteOpenHelper { } private void upgradeDatabaseFrom15to16(SQLiteDatabase db) { - db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED); - db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED_WITH_FAVICONS); - - createCombinedViewOn16(db); + // No harm in creating the v19 combined view here: means we don't need two almost-identical + // functions to define both the v16 and v19 ones. The upgrade path will redundantly drop + // and recreate the view again. *shrug* + createV19CombinedView(db); } private void upgradeDatabaseFrom16to17(SQLiteDatabase db) { @@ -1378,6 +1413,18 @@ final class BrowserDatabaseHelper extends SQLiteOpenHelper { } } + private void upgradeDatabaseFrom18to19(SQLiteDatabase db) { + // Redefine the "combined" view... + createV19CombinedView(db); + } + + private void createV19CombinedView(SQLiteDatabase db) { + db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED); + db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED_WITH_FAVICONS); + + createCombinedViewOn19(db); + } + @Override public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) { debug("Upgrading browser.db: " + db.getPath() + " from " + @@ -1454,6 +1501,10 @@ final class BrowserDatabaseHelper extends SQLiteOpenHelper { case 18: upgradeDatabaseFrom17to18(db); break; + + case 19: + upgradeDatabaseFrom18to19(db); + break; } } @@ -1566,4 +1617,4 @@ final class BrowserDatabaseHelper extends SQLiteOpenHelper { bookmark.remove("folder"); } } -} \ No newline at end of file +}