Bug 1306445 - Remove the mobile bookmarks annotation from the mobile root as it is no longer required. r?mak draft
authorMark Banner <standard8@mozilla.com>
Tue, 20 Mar 2018 20:25:10 +0000
changeset 770643 49ac05521f62e6526e123b475a755281bb3bdd29
parent 770373 3d21d31141dc5e2d2aff89203458125a3cce6c64
push id103464
push userbmo:standard8@mozilla.com
push dateWed, 21 Mar 2018 17:32:39 +0000
reviewersmak
bugs1306445
milestone61.0a1
Bug 1306445 - Remove the mobile bookmarks annotation from the mobile root as it is no longer required. r?mak MozReview-Commit-ID: 4HhxEpY7NNs
toolkit/components/places/Database.cpp
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
toolkit/components/places/tests/migration/test_current_from_v34.js
toolkit/components/places/tests/migration/test_current_from_v43.js
toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js
toolkit/components/places/tests/unit/test_telemetry.js
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -103,16 +103,18 @@
 // Places string bundle, contains internationalized bookmark root names.
 #define PLACES_BUNDLE "chrome://places/locale/places.properties"
 
 // Livemarks annotations.
 #define LMANNO_FEEDURI "livemark/feedURI"
 #define LMANNO_SITEURI "livemark/siteURI"
 
 #define MOBILE_ROOT_GUID "mobile______"
+// This is no longer used & obsolete except for during migration.
+// Note: it may still be found in older places databases.
 #define MOBILE_ROOT_ANNO "mobile/bookmarksRoot"
 
 // We use a fixed title for the mobile root to avoid marking the database as
 // corrupt if we can't look up the localized title in the string bundle. Sync
 // sets the title to the localized version when it creates the left pane query.
 #define MOBILE_ROOT_TITLE "mobile"
 
 using namespace mozilla;
@@ -2213,65 +2215,16 @@ Database::CreateMobileRoot()
   bool hasResult = false;
   rv = findIdStmt->ExecuteStep(&hasResult);
   if (NS_FAILED(rv) || !hasResult) return -1;
 
   int64_t rootId;
   rv = findIdStmt->GetInt64(0, &rootId);
   if (NS_FAILED(rv)) return -1;
 
-  // Set the mobile bookmarks anno on the new root, so that Sync code on an
-  // older channel can still find it in case of a downgrade. This can be
-  // removed in bug 1306445.
-  nsCOMPtr<mozIStorageStatement> addAnnoNameStmt;
-  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
-    "INSERT OR IGNORE INTO moz_anno_attributes (name) VALUES (:anno_name)"
-  ), getter_AddRefs(addAnnoNameStmt));
-  if (NS_FAILED(rv)) return -1;
-  mozStorageStatementScoper addAnnoNameScoper(addAnnoNameStmt);
-
-  rv = addAnnoNameStmt->BindUTF8StringByName(
-    NS_LITERAL_CSTRING("anno_name"), NS_LITERAL_CSTRING(MOBILE_ROOT_ANNO));
-  if (NS_FAILED(rv)) return -1;
-  rv = addAnnoNameStmt->Execute();
-  if (NS_FAILED(rv)) return -1;
-
-  nsCOMPtr<mozIStorageStatement> addAnnoStmt;
-  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
-    "INSERT OR IGNORE INTO moz_items_annos "
-      "(id, item_id, anno_attribute_id, content, flags, "
-       "expiration, type, dateAdded, lastModified) "
-    "SELECT "
-      "(SELECT a.id FROM moz_items_annos a "
-       "WHERE a.anno_attribute_id = n.id AND "
-             "a.item_id = :root_id), "
-      ":root_id, n.id, 1, 0, :expiration, :type, :timestamp, :timestamp "
-    "FROM moz_anno_attributes n WHERE name = :anno_name"
-  ), getter_AddRefs(addAnnoStmt));
-  if (NS_FAILED(rv)) return -1;
-  mozStorageStatementScoper addAnnoScoper(addAnnoStmt);
-
-  rv = addAnnoStmt->BindInt64ByName(NS_LITERAL_CSTRING("root_id"), rootId);
-  if (NS_FAILED(rv)) return -1;
-  rv = addAnnoStmt->BindUTF8StringByName(
-    NS_LITERAL_CSTRING("anno_name"), NS_LITERAL_CSTRING(MOBILE_ROOT_ANNO));
-  if (NS_FAILED(rv)) return -1;
-  rv = addAnnoStmt->BindInt32ByName(NS_LITERAL_CSTRING("expiration"),
-                                    nsIAnnotationService::EXPIRE_NEVER);
-  if (NS_FAILED(rv)) return -1;
-  rv = addAnnoStmt->BindInt32ByName(NS_LITERAL_CSTRING("type"),
-                                    nsIAnnotationService::TYPE_INT32);
-  if (NS_FAILED(rv)) return -1;
-  rv = addAnnoStmt->BindInt32ByName(NS_LITERAL_CSTRING("timestamp"),
-                                    RoundedPRNow());
-  if (NS_FAILED(rv)) return -1;
-
-  rv = addAnnoStmt->Execute();
-  if (NS_FAILED(rv)) return -1;
-
   return rootId;
 }
 
 void
 Database::Shutdown()
 {
   // As the last step in the shutdown path, finalize the database handle.
   MOZ_ASSERT(NS_IsMainThread());
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -377,17 +377,16 @@ const HistorySyncUtils = PlacesSyncUtils
   },
 });
 
 const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
   SMART_BOOKMARKS_ANNO: "Places/SmartBookmark",
   DESCRIPTION_ANNO: "bookmarkProperties/description",
   SIDEBAR_ANNO: "bookmarkProperties/loadInSidebar",
   SYNC_PARENT_ANNO: "sync/parent",
-  SYNC_MOBILE_ROOT_ANNO: "mobile/bookmarksRoot",
 
   SYNC_ID_META_KEY: "sync/bookmarks/syncId",
   LAST_SYNC_META_KEY: "sync/bookmarks/lastSync",
   WIPE_REMOTE_META_KEY: "sync/bookmarks/wipeRemote",
 
   // Jan 23, 1993 in milliseconds since 1970. Corresponds roughly to the release
   // of the original NCSA Mosiac. We can safely assume that any dates before
   // this time are invalid.
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -324,16 +324,17 @@ var PlacesUtils = {
   TYPE_X_MOZ_PLACE_ACTION: "text/x-moz-place-action",
 
   EXCLUDE_FROM_BACKUP_ANNO: "places/excludeFromBackup",
   LMANNO_FEEDURI: "livemark/feedURI",
   LMANNO_SITEURI: "livemark/siteURI",
   POST_DATA_ANNO: "bookmarkProperties/POSTData",
   READ_ONLY_ANNO: "placesInternal/READ_ONLY",
   CHARSET_ANNO: "URIProperties/characterSet",
+  // Deprecated: This is only used for supporting import from older datasets.
   MOBILE_ROOT_ANNO: "mobile/bookmarksRoot",
 
   TOPIC_SHUTDOWN: "places-shutdown",
   TOPIC_INIT_COMPLETE: "places-init-complete",
   TOPIC_DATABASE_LOCKED: "places-database-locked",
   TOPIC_EXPIRATION_FINISHED: "places-expiration-finished",
   TOPIC_FEEDBACK_UPDATED: "places-autocomplete-feedback-updated",
   TOPIC_FAVICONS_EXPIRED: "places-favicons-expired",
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
@@ -78,23 +78,19 @@ add_task(async function test_eraseEveryt
   await manyFrecenciesPromise;
 
   Assert.equal(frecencyForUrl("http://example.com/"), frecencyForExample);
   Assert.equal(frecencyForUrl("http://example.com/"), frecencyForMozilla);
 
   // Check there are no orphan annotations.
   let conn = await PlacesUtils.promiseDBConnection();
   let annoAttrs = await conn.execute(`SELECT id, name FROM moz_anno_attributes`);
-  // Bug 1306445 will eventually remove the mobile root anno.
-  Assert.equal(annoAttrs.length, 1);
-  Assert.equal(annoAttrs[0].getResultByName("name"), PlacesUtils.MOBILE_ROOT_ANNO);
+  Assert.equal(annoAttrs.length, 0);
   let annos = await conn.execute(`SELECT item_id, anno_attribute_id FROM moz_items_annos`);
-  Assert.equal(annos.length, 1);
-  Assert.equal(annos[0].getResultByName("item_id"), PlacesUtils.mobileFolderId);
-  Assert.equal(annos[0].getResultByName("anno_attribute_id"), annoAttrs[0].getResultByName("id"));
+  Assert.equal(annos.length, 0);
 });
 
 add_task(async function test_eraseEverything_roots() {
   await PlacesUtils.bookmarks.eraseEverything();
 
   // Ensure the roots have not been removed.
   Assert.ok(await PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.unfiledGuid));
   Assert.ok(await PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.toolbarGuid));
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -132,23 +132,19 @@ add_task(async function remove_bookmark_
   PlacesUtils.annotations.setItemAnnotation((await PlacesUtils.promiseItemId(bm1.guid)),
                                             "testanno", "testvalue", 0, 0);
 
   await PlacesUtils.bookmarks.remove(bm1.guid);
 
   // Check there are no orphan annotations.
   let conn = await PlacesUtils.promiseDBConnection();
   let annoAttrs = await conn.execute(`SELECT id, name FROM moz_anno_attributes`);
-  // Bug 1306445 will eventually remove the mobile root anno.
-  Assert.equal(annoAttrs.length, 1);
-  Assert.equal(annoAttrs[0].getResultByName("name"), PlacesUtils.MOBILE_ROOT_ANNO);
+  Assert.equal(annoAttrs.length, 0);
   let annos = await conn.execute(`SELECT item_id, anno_attribute_id FROM moz_items_annos`);
-  Assert.equal(annos.length, 1);
-  Assert.equal(annos[0].getResultByName("item_id"), PlacesUtils.mobileFolderId);
-  Assert.equal(annos[0].getResultByName("anno_attribute_id"), annoAttrs[0].getResultByName("id"));
+  Assert.equal(annos.length, 0);
 });
 
 add_task(async function remove_bookmark_empty_title() {
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                  url: "http://example.com/",
                                                  title: "" });
   checkBookmarkObject(bm1);
--- a/toolkit/components/places/tests/migration/test_current_from_v34.js
+++ b/toolkit/components/places/tests/migration/test_current_from_v34.js
@@ -126,16 +126,9 @@ add_task(async function test_mobile_root
     "Firefox bookmark should be moved to new mobile root");
   equal(fxBmk.index, 0, "Firefox bookmark should be first child of new root");
 
   let tbBmk = await PlacesUtils.bookmarks.fetch(tbGuid);
   equal(tbBmk.parentGuid, PlacesUtils.bookmarks.mobileGuid,
     "Thunderbird bookmark should be moved to new mobile root");
   equal(tbBmk.index, 1,
     "Thunderbird bookmark should be second child of new root");
-
-  let mobileRootId = await PlacesUtils.promiseItemId(
-    PlacesUtils.bookmarks.mobileGuid);
-  let annoItemIds = PlacesUtils.annotations.getItemsWithAnnotation(
-    PlacesUtils.MOBILE_ROOT_ANNO, {});
-  deepEqual(annoItemIds, [mobileRootId],
-    "Only mobile root should have mobile anno");
 });
--- a/toolkit/components/places/tests/migration/test_current_from_v43.js
+++ b/toolkit/components/places/tests/migration/test_current_from_v43.js
@@ -180,28 +180,16 @@ add_task(async function test_no_orphan_a
     SELECT id FROM moz_anno_attributes
     WHERE id NOT IN (SELECT id from moz_items_annos)
   `);
 
   Assert.equal(rows.length, 0,
     `Should have no orphan annotation attributes.`);
 });
 
-add_task(async function test_mobile_bookmarks_root_still_exists() {
-  let db = await PlacesUtils.promiseDBConnection();
-
-  let rows = await db.execute(`
-    SELECT id FROM moz_anno_attributes
-    WHERE name = 'mobile/bookmarksRoot'
-  `);
-
-  Assert.equal(rows.length, 1,
-    "Mobile bookmarks root annotation should still exist");
-});
-
 add_task(async function test_no_orphan_keywords() {
   let db = await PlacesUtils.promiseDBConnection();
 
   let rows = await db.execute(`
     SELECT place_id FROM moz_keywords
     WHERE place_id NOT IN (SELECT id from moz_places)
   `);
 
--- a/toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js
+++ b/toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js
@@ -43,22 +43,16 @@ add_task(async function test_restore_mob
       guid: PlacesUtils.bookmarks.toolbarGuid,
       index: 1,
     }, {
       guid: PlacesUtils.bookmarks.unfiledGuid,
       index: 3,
     }, {
       guid: PlacesUtils.bookmarks.mobileGuid,
       index: 4,
-      annos: [{
-        name: "mobile/bookmarksRoot",
-        flags: 0,
-        expires: 4,
-        value: 1,
-      }],
       children: [
         { guid: "_o8e1_zxTJFg", index: 0 },
         { guid: "QCtSqkVYUbXB", index: 1 },
       ],
     }],
   }, "Should restore mobile bookmarks from root");
 
   await PlacesUtils.bookmarks.eraseEverything();
@@ -84,37 +78,33 @@ add_task(async function test_import_mobi
       guid: PlacesUtils.bookmarks.toolbarGuid,
       index: 1,
     }, {
       guid: PlacesUtils.bookmarks.unfiledGuid,
       index: 3,
     }, {
       guid: PlacesUtils.bookmarks.mobileGuid,
       index: 4,
-      annos: [{
-        name: "mobile/bookmarksRoot",
-        flags: 0,
-        expires: 4,
-        value: 1,
-      }],
       children: [
         // The first two are in ..._import.json, the second two are in
         // ..._merge.json
         { guid: "_o8e1_zxTJFg", index: 0 },
         { guid: "QCtSqkVYUbXB", index: 1 },
         { guid: "a17yW6-nTxEJ", index: 2 },
         { guid: "xV10h9Wi3FBM", index: 3 },
       ],
     }],
   }, "Should merge bookmarks root contents");
 
   await PlacesUtils.bookmarks.eraseEverything();
 });
 
 add_task(async function test_restore_mobile_bookmarks_folder() {
+  // This tests importing a mobile bookmarks folder with the annotation,
+  // and the old, random guid.
   await importFromFixture("mobile_bookmarks_folder_import.json",
                            /* replace */ true);
 
   await treeEquals(PlacesUtils.bookmarks.rootGuid, {
     guid: PlacesUtils.bookmarks.rootGuid,
     index: 0,
     children: [{
       guid: PlacesUtils.bookmarks.menuGuid,
@@ -129,22 +119,16 @@ add_task(async function test_restore_mob
       children: [{ guid: "buy7711R3ZgE", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.unfiledGuid,
       index: 3,
       children: [{ guid: "KIa9iKZab2Z5", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.mobileGuid,
       index: 4,
-      annos: [{
-        name: "mobile/bookmarksRoot",
-        flags: 0,
-        expires: 4,
-        value: 1,
-      }],
       children: [
         { guid: "_o8e1_zxTJFg", index: 0 },
         { guid: "QCtSqkVYUbXB", index: 1 },
       ],
     }],
   }, "Should restore mobile bookmark folder contents into mobile root");
 
   // We rewrite queries to point to the root ID instead of the name
@@ -181,22 +165,16 @@ add_task(async function test_import_mobi
       children: [{ guid: "buy7711R3ZgE", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.unfiledGuid,
       index: 3,
       children: [{ guid: "KIa9iKZab2Z5", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.mobileGuid,
       index: 4,
-      annos: [{
-        name: "mobile/bookmarksRoot",
-        flags: 0,
-        expires: 4,
-        value: 1,
-      }],
       children: [
         { guid: "_o8e1_zxTJFg", index: 0 },
         { guid: "QCtSqkVYUbXB", index: 1 },
         { guid: "a17yW6-nTxEJ", index: 2 },
         { guid: "xV10h9Wi3FBM", index: 3 },
       ],
     }],
   }, "Should merge bookmarks folder contents into mobile root");
@@ -225,22 +203,16 @@ add_task(async function test_restore_mul
       children: [{ guid: "Utodo9b0oVws", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.unfiledGuid,
       index: 3,
       children: [{ guid: "xV10h9Wi3FBM", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.mobileGuid,
       index: 4,
-      annos: [{
-        name: "mobile/bookmarksRoot",
-        flags: 0,
-        expires: 4,
-        value: 1,
-      }],
       children: [
         { guid: "a17yW6-nTxEJ", index: 0 },
         { guid: "sSZ86WT9WbN3", index: 1 },
       ],
     }],
   }, "Should restore multiple bookmarks folder contents into root");
 
   await PlacesUtils.bookmarks.eraseEverything();
@@ -270,22 +242,16 @@ add_task(async function test_import_mult
       children: [{ guid: "Utodo9b0oVws", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.unfiledGuid,
       index: 3,
       children: [{ guid: "xV10h9Wi3FBM", index: 0 }],
     }, {
       guid: PlacesUtils.bookmarks.mobileGuid,
       index: 4,
-      annos: [{
-        name: "mobile/bookmarksRoot",
-        flags: 0,
-        expires: 4,
-        value: 1,
-      }],
       children: [
         { guid: "_o8e1_zxTJFg", index: 0 },
         { guid: "QCtSqkVYUbXB", index: 1 },
         { guid: "a17yW6-nTxEJ", index: 2 },
         { guid: "sSZ86WT9WbN3", index: 3 },
       ],
     }],
   }, "Should merge multiple mobile folders into root");
--- a/toolkit/components/places/tests/unit/test_telemetry.js
+++ b/toolkit/components/places/tests/unit/test_telemetry.js
@@ -15,19 +15,17 @@ var histograms = {
   PLACES_DATABASE_FILESIZE_MB: val => Assert.ok(val > 0),
   PLACES_DATABASE_FAVICONS_FILESIZE_MB: val => Assert.ok(val > 0),
   PLACES_DATABASE_PAGESIZE_B: val => Assert.equal(val, 32768),
   PLACES_DATABASE_SIZE_PER_PAGE_B: val => Assert.ok(val > 0),
   PLACES_EXPIRATION_STEPS_TO_CLEAN2: val => Assert.ok(val > 1),
   // PLACES_AUTOCOMPLETE_1ST_RESULT_TIME_MS:  val => do_check_true(val > 1),
   PLACES_IDLE_FRECENCY_DECAY_TIME_MS: val => Assert.ok(val >= 0),
   PLACES_IDLE_MAINTENANCE_TIME_MS: val => Assert.ok(val > 0),
-  // One from the `setItemAnnotation` call; the other from the mobile root.
-  // This can be removed along with the anno in bug 1306445.
-  PLACES_ANNOS_BOOKMARKS_COUNT: val => Assert.equal(val, 2),
+  PLACES_ANNOS_BOOKMARKS_COUNT: val => Assert.equal(val, 1),
   PLACES_ANNOS_PAGES_COUNT: val => Assert.equal(val, 1),
   PLACES_MAINTENANCE_DAYSFROMLAST: val => Assert.ok(val >= 0),
 };
 
 /**
  * Forces an expiration run.
  *
  * @param [optional] aLimit