From 00ac6ec09b09de11c7669766e91bf3dde9be0582 Mon Sep 17 00:00:00 2001 From: Lucas Rocha Date: Fri, 15 Jun 2012 19:54:40 +0100 Subject: [PATCH] Bug 748583 - Fix bookmark id fetching in the Combined view (r=margaret) --- .../android/base/db/BrowserProvider.java.in | 73 ++++++++++++++++++- .../base/tests/testBrowserProvider.java.in | 39 ++++++++++ 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in index bbacf1d9dbd..bac2930c8e8 100644 --- a/mobile/android/base/db/BrowserProvider.java.in +++ b/mobile/android/base/db/BrowserProvider.java.in @@ -67,7 +67,7 @@ public class BrowserProvider extends ContentProvider { static final String DATABASE_NAME = "browser.db"; - static final int DATABASE_VERSION = 9; + static final int DATABASE_VERSION = 10; // Maximum age of deleted records to be cleaned up (20 days in ms) static final long MAX_AGE_OF_DELETED_RECORDS = 86400000 * 20; @@ -494,6 +494,65 @@ public class BrowserProvider extends ContentProvider { " ON " + Combined.URL + " = " + qualifyColumn(TABLE_IMAGES, Images.URL)); } + private void createCombinedWithImagesViewOn10(SQLiteDatabase db) { + debug("Creating " + VIEW_COMBINED_WITH_IMAGES + " view"); + + db.execSQL("CREATE VIEW IF NOT EXISTS " + VIEW_COMBINED_WITH_IMAGES + " 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 + ", " + + qualifyColumn(TABLE_IMAGES, Images.FAVICON) + " AS " + Combined.FAVICON + ", " + + qualifyColumn(TABLE_IMAGES, Images.THUMBNAIL) + " AS " + Combined.THUMBNAIL + + " 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 + + " FROM " + TABLE_BOOKMARKS + + " WHERE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE) + " = " + Bookmarks.TYPE_BOOKMARK + " 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 " + + qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " ELSE NULL END AS " + Combined.BOOKMARK_ID + ", " + + qualifyColumn(TABLE_HISTORY, History.URL) + " AS " + Combined.URL + ", " + + // Prioritze 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 + ", " + + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " + + Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " 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 + + " 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 + ")" + + ") LEFT OUTER JOIN " + TABLE_IMAGES + + " ON " + Combined.URL + " = " + qualifyColumn(TABLE_IMAGES, Images.URL)); + } + @Override public void onCreate(SQLiteDatabase db) { debug("Creating browser.db: " + db.getPath()); @@ -504,7 +563,7 @@ public class BrowserProvider extends ContentProvider { createBookmarksWithImagesView(db); createHistoryWithImagesView(db); - createCombinedWithImagesViewOn9(db); + createCombinedWithImagesViewOn10(db); createOrUpdateSpecialFolder(db, Bookmarks.PLACES_FOLDER_GUID, R.string.bookmarks_folder_places, 0); @@ -924,6 +983,13 @@ public class BrowserProvider extends ContentProvider { createCombinedWithImagesViewOn9(db); } + private void upgradeDatabaseFrom9to10(SQLiteDatabase db) { + debug("Dropping view: " + VIEW_COMBINED_WITH_IMAGES); + db.execSQL("DROP VIEW IF EXISTS " + VIEW_COMBINED_WITH_IMAGES); + + createCombinedWithImagesViewOn10(db); + } + @Override public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) { debug("Upgrading browser.db: " + db.getPath() + " from " + @@ -964,6 +1030,9 @@ public class BrowserProvider extends ContentProvider { case 9: upgradeDatabaseFrom8to9(db); + + case 10: + upgradeDatabaseFrom9to10(db); } } diff --git a/mobile/android/base/tests/testBrowserProvider.java.in b/mobile/android/base/tests/testBrowserProvider.java.in index 5e97744d4c1..518a9706ee4 100644 --- a/mobile/android/base/tests/testBrowserProvider.java.in +++ b/mobile/android/base/tests/testBrowserProvider.java.in @@ -316,6 +316,7 @@ public class testBrowserProvider extends ContentProviderTest { mTests.add(new TestCombinedView()); mTests.add(new TestCombinedViewDisplay()); + mTests.add(new TestCombinedViewWithDeletedBookmark()); } public void testBrowserProvider() throws Exception { @@ -1488,4 +1489,42 @@ public class testBrowserProvider extends ContentProviderTest { } } } + + class TestCombinedViewWithDeletedBookmark extends Test { + public void test() throws Exception { + final String TITLE = "Test Page 1"; + final String URL = "http://example.com"; + final int VISITS = 10; + final long LAST_VISITED = System.currentTimeMillis(); + + // Create a combined history entry + ContentValues combinedHistory = createHistoryEntry(TITLE, URL, VISITS, LAST_VISITED); + mProvider.insert(mHistoryUri, combinedHistory); + + // Create a combined bookmark entry + ContentValues combinedBookmark = createBookmark(TITLE, URL, mMobileFolderId, + mBookmarksTypeBookmark, 0, "tags", "description", "keyword"); + long combinedBookmarkId = ContentUris.parseId(mProvider.insert(mBookmarksUri, combinedBookmark)); + + Cursor c = mProvider.query(mCombinedUri, null, "", null, null); + mAsserter.is(c.getCount(), 1, "1 combined entry found"); + + mAsserter.is(c.moveToFirst(), true, "Found combined entry with bookmark id"); + mAsserter.is(new Long(c.getLong(c.getColumnIndex(mCombinedBookmarkIdCol))), new Long(combinedBookmarkId), + "Bookmark id should be set correctly on combined entry"); + + int deleted = mProvider.delete(mBookmarksUri, + mBookmarksIdCol + " = ?", + new String[] { String.valueOf(combinedBookmarkId) }); + + mAsserter.is((deleted == 1), true, "Inserted combined bookmark was deleted"); + + c = mProvider.query(mCombinedUri, null, "", null, null); + mAsserter.is(c.getCount(), 1, "1 combined entry found"); + + mAsserter.is(c.moveToFirst(), true, "Found combined entry without bookmark id"); + mAsserter.is(new Long(c.getLong(c.getColumnIndex(mCombinedBookmarkIdCol))), new Long(0), + "Bookmark id should not be set to removed bookmark id"); + } + } }