Bug 1388884 - Do not delete bookmark tombstones r=rnewman draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Tue, 29 Aug 2017 14:01:14 -0400
changeset 655234 d4abf10bded3f6ab39d13f0152cf981cd9fe4df7
parent 654398 3529b653ede26f990eb7320649015294ad0f8e76
child 728774 acfa2ef56aede98e5b4f27d2e4537258259ca2c3
push id76807
push userbmo:gkruglov@mozilla.com
push dateTue, 29 Aug 2017 19:32:18 +0000
reviewersrnewman
bugs1388884
milestone57.0a1
Bug 1388884 - Do not delete bookmark tombstones r=rnewman As implemented, this means we might upload tombstones for never-synced bookmarks. This _should_ be harmless. MozReview-Commit-ID: DZx9yWDs1ie
mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java
mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -2383,22 +2383,16 @@ public class BrowserProvider extends Sha
 
         // Finally, delete everything that needs to be deleted all at once.
         // We need to compute list of all bookmarks and their descendants first.
         // We calculate our deletion tree based on 'selection', and so above queries do not null-out
         // any of the bookmark fields. This will be done in `bulkDeleteByBookmarkGUIDs`.
         final List<String> guids = getBookmarkDescendantGUIDs(db, selection, selectionArgs);
         changed += bulkDeleteByBookmarkGUIDs(db, uri, guids);
 
-        try {
-            cleanUpSomeDeletedRecords(uri, TABLE_BOOKMARKS);
-        } catch (Exception e) {
-            // We don't care.
-            Log.e(LOGTAG, "Unable to clean up deleted bookmark records: ", e);
-        }
         return changed;
     }
 
     /**
      * Get bookmark descendant IDs with conditions.
      * @return A list of bookmark GUID.
      */
     private List<String> getBookmarkDescendantGUIDs(SQLiteDatabase db, String selection, String[] selectionArgs) {
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java
@@ -367,50 +367,50 @@ public class BrowserProviderBookmarksTes
         assertVersionsForGuid(guid2, 2, 1);
 
         // Test that explicitely asking to update localVersion from sync does increment it.
         updateBookmarkTitleByGuid(getBookmarksTestSyncIncrementLocalVersionUri, guid2, "again, title!");
         assertVersionsForGuid(guid2, 3, 1);
 
         assertVersionsForGuid(BrowserContract.Bookmarks.MOBILE_FOLDER_GUID, 2, 0);
 
-        // delete bookmark from fennec, since this record is not synced, it will be DELETED entirely
+        // delete bookmark from fennec, expect bookmark tombstones to persist for unsynced records
         deleteByGuid(bookmarksTestUri, guid1);
-        assertEquals(0, getRowCount(Bookmarks.GUID + " = ?", new String[]{guid1}));
+        assertEquals(1, getRowCount(Bookmarks.GUID + " = ?", new String[]{guid1}));
 
-        assertEquals(9, getTotalNumberOfBookmarks());
+        assertEquals(10, getTotalNumberOfBookmarks());
 
         assertVersionsForGuid(BrowserContract.Bookmarks.MOBILE_FOLDER_GUID, 3, 0);
 
         // insert a bookmark from sync, and delete it from fennec. this should bump its localVersion
         String guid4 = UUID.randomUUID().toString();
         insert("bookmark-4", "https://www.mozilla-4.org", guid4,
                 rootId, BrowserContract.Bookmarks.TYPE_BOOKMARK, true);
 
-        assertEquals(10, getTotalNumberOfBookmarks());
+        assertEquals(11, getTotalNumberOfBookmarks());
 
         // inserting bookmark from sync should not touch its parent's localVersion
         assertVersionsForGuid(BrowserContract.Bookmarks.MOBILE_FOLDER_GUID, 3, 0);
 
         assertVersionsForGuid(guid4, 1, 1);
         deleteByGuid(bookmarksTestUri, guid4);
 
         // deleting bookmark from fennec _must_ increment its parent's localVersion
         assertVersionsForGuid(BrowserContract.Bookmarks.MOBILE_FOLDER_GUID, 4, 0);
 
         // sanity check our total number of bookmarks, it shouldn't have changed because of the delete
-        assertEquals(10, getTotalNumberOfBookmarks());
+        assertEquals(11, getTotalNumberOfBookmarks());
 
         // assert that bookmark is still present after deletion, and its local version has been bumped
         assertEquals(1, getRowCount(Bookmarks.GUID + " = ?", new String[]{guid4}));
         assertVersionsForGuid(guid4, 2, 1);
 
         // delete bookmark from sync, will just DELETE it from the database entirely
         deleteByGuid(bookmarksTestSyncUri, guid2);
-        assertEquals(9, getTotalNumberOfBookmarks());
+        assertEquals(10, getTotalNumberOfBookmarks());
         assertEquals(0, getRowCount(Bookmarks.GUID + " = ?", new String[]{guid2}));
 
         // deleting bookmark from sync should not touch its parent's localVersion
         assertVersionsForGuid(BrowserContract.Bookmarks.MOBILE_FOLDER_GUID, 4, 0);
 
         // check that deleting a folder from fennec bumps localVersions of its entire subtree
         // root  -- localVersion bumped
         // -> sibling
--- a/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java
+++ b/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java
@@ -670,45 +670,17 @@ public class testBrowserProvider extends
 
             Cursor c = getBookmarkById(id);
             mAsserter.is(c.moveToFirst(), true, "Inserted bookmark found");
             c.close();
 
             return id;
         }
 
-        @Override
-        public void test() throws Exception {
-            // Test that unsynced bookmarks are dropped from the database.
-            long id = insertOneBookmark();
-
-            int changed = mProvider.delete(BrowserContract.Bookmarks.CONTENT_URI,
-                                           BrowserContract.Bookmarks._ID + " = ?",
-                                           new String[] { String.valueOf(id) });
-
-            // Deletions also affect parents of folders, and so that must be accounted for.
-            mAsserter.is((changed == 2), true, "Inserted bookmark was deleted");
-
-            Cursor c = getBookmarkById(appendUriParam(BrowserContract.Bookmarks.CONTENT_URI, BrowserContract.PARAM_SHOW_DELETED, "1"), id);
-            mAsserter.is(c.moveToFirst(), false, "Unsynced deleted bookmark was dropped from the database");
-
-            // Test that synced bookmarks are only marked as deleted.
-            id = insertOneBookmark("test-guid", true);
-
-            // Bookmark has been inserted from sync. Let's delete it again, and test that it has not
-            // been dropped from the database.
-            changed = mProvider.delete(BrowserContract.Bookmarks.CONTENT_URI,
-                    BrowserContract.Bookmarks._ID + " = ?",
-                    new String[] { String.valueOf(id) });
-
-            // Deletions also affect parents of folders, and so that must be accounted for.
-            mAsserter.is((changed == 2), true, "Inserted bookmark was deleted");
-
-            c = getBookmarkById(appendUriParam(BrowserContract.Bookmarks.CONTENT_URI, BrowserContract.PARAM_SHOW_DELETED, "1"), id);
-            mAsserter.is(c.moveToFirst(), true, "Deleted bookmark was only marked as deleted");
+        private void verifyMarkedAsDeleted(Cursor c) {
             mAsserter.is(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.TITLE)), null,
                     "Deleted bookmark title is null");
             mAsserter.is(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.URL)), null,
                     "Deleted bookmark URL is null");
             mAsserter.is(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.TAGS)), null,
                     "Deleted bookmark tags is null");
             mAsserter.is(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.KEYWORD)), null,
                     "Deleted bookmark keyword is null");
@@ -719,16 +691,50 @@ public class testBrowserProvider extends
             mAsserter.is(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.PARENT)), null,
                     "Deleted bookmark parent ID is null");
             mAsserter.is(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.IS_DELETED)), String.valueOf(1),
                     "Deleted bookmark has correct is-deleted state");
             mAsserter.is(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.FAVICON_ID)), null,
                     "Deleted bookmark Favicon ID is null");
             mAsserter.isnot(c.getString(c.getColumnIndex(BrowserContract.Bookmarks.GUID)), null,
                     "Deleted bookmark GUID is not null");
+        }
+
+        @Override
+        public void test() throws Exception {
+            // Test that unsynced bookmarks are not dropped from the database.
+            long id = insertOneBookmark();
+
+            int changed = mProvider.delete(BrowserContract.Bookmarks.CONTENT_URI,
+                                           BrowserContract.Bookmarks._ID + " = ?",
+                                           new String[] { String.valueOf(id) });
+
+            // Deletions also affect parents of folders, and so that must be accounted for.
+            mAsserter.is((changed == 2), true, "Inserted bookmark was deleted");
+
+            Cursor c = getBookmarkById(appendUriParam(BrowserContract.Bookmarks.CONTENT_URI, BrowserContract.PARAM_SHOW_DELETED, "1"), id);
+            mAsserter.is(c.moveToFirst(), true, "Unsynced deleted was only marked as deleted");
+            verifyMarkedAsDeleted(c);
+            c.close();
+
+            // Test that synced bookmarks are only marked as deleted.
+            id = insertOneBookmark("test-guid", true);
+
+            // Bookmark has been inserted from sync. Let's delete it again, and test that it has not
+            // been dropped from the database.
+            changed = mProvider.delete(BrowserContract.Bookmarks.CONTENT_URI,
+                    BrowserContract.Bookmarks._ID + " = ?",
+                    new String[] { String.valueOf(id) });
+
+            // Deletions also affect parents of folders, and so that must be accounted for.
+            mAsserter.is((changed == 2), true, "Inserted bookmark was deleted");
+
+            c = getBookmarkById(appendUriParam(BrowserContract.Bookmarks.CONTENT_URI, BrowserContract.PARAM_SHOW_DELETED, "1"), id);
+            mAsserter.is(c.moveToFirst(), true, "Deleted bookmark was only marked as deleted");
+            verifyMarkedAsDeleted(c);
             c.close();
 
             changed = mProvider.delete(appendUriParam(BrowserContract.Bookmarks.CONTENT_URI, BrowserContract.PARAM_IS_SYNC, "1"),
                                        BrowserContract.Bookmarks._ID + " = ?",
                                        new String[] { String.valueOf(id) });
 
             // Deletions from sync skip bumping timestamps of parents.
             mAsserter.is((changed == 1), true, "Inserted bookmark was deleted");