Bug 1306445 - Remove the mobile bookmarks annotation from the mobile root as it is no longer required. r=mak
authorMark Banner <standard8@mozilla.com>
Tue, 20 Mar 2018 20:25:10 +0000
changeset 409508 f371354d136461f2aaaeaffbb2646ad1f7334698
parent 409507 2491f8caeffa214e7a2ded24978486b304c5cc8f
child 409509 562edc8ff72791e0f936a8a01569c2825da15bb4
push id61533
push usermbanner@mozilla.com
push dateThu, 22 Mar 2018 14:19:41 +0000
treeherderautoland@f371354d1364 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1306445
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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