Bug 1302901 - Remove mobile root creation from Sync. draft
authorKit Cambridge <kcambridge@mozilla.com>
Thu, 15 Sep 2016 17:15:20 -0700
changeset 414235 03241dbb6f6e3e8047bf9667041e286f14abb3ed
parent 414234 74b7f5040f4cd784c732de51be5f5d4b0831739a
child 531402 4f4e92431e96e1b1d71680a25fdb2eb02110544d
push id29627
push userkcambridge@mozilla.com
push dateFri, 16 Sep 2016 00:29:32 +0000
bugs1302901
milestone51.0a1
Bug 1302901 - Remove mobile root creation from Sync. MozReview-Commit-ID: 6fphDT1Owni
services/sync/modules/bookmark_utils.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
services/sync/tests/unit/test_bookmark_order.js
services/sync/tests/unit/test_bookmark_tracker.js
--- a/services/sync/modules/bookmark_utils.js
+++ b/services/sync/modules/bookmark_utils.js
@@ -30,74 +30,37 @@ let BookmarkAnnos = {
 // so we define this map as a lazy getter.
 XPCOMUtils.defineLazyGetter(this, "SpecialGUIDToPlacesGUID", () => {
   return {
     "menu": PlacesUtils.bookmarks.menuGuid,
     "places": PlacesUtils.bookmarks.rootGuid,
     "tags": PlacesUtils.bookmarks.tagsGuid,
     "toolbar": PlacesUtils.bookmarks.toolbarGuid,
     "unfiled": PlacesUtils.bookmarks.unfiledGuid,
-
-    get mobile() {
-      let mobileRootId = BookmarkSpecialIds.findMobileRoot(true);
-      return Async.promiseSpinningly(PlacesUtils.promiseItemGuid(mobileRootId));
-    },
+    "mobile": PlacesUtils.bookmarks.mobileGuid,
   };
 });
 
 let BookmarkSpecialIds = {
 
-  // Special IDs. Note that mobile can attempt to create a record on
-  // dereference; special accessors are provided to prevent recursion within
-  // observers.
+  // Special IDs.
   guids: ["menu", "places", "tags", "toolbar", "unfiled", "mobile"],
 
-  // Create the special mobile folder to store mobile bookmarks.
-  createMobileRoot: function createMobileRoot() {
-    let root = PlacesUtils.placesRootId;
-    let mRoot = PlacesUtils.bookmarks.createFolder(root, "mobile", -1,
-      /* guid */ null, PlacesUtils.bookmarks.SOURCE_SYNC);
-    PlacesUtils.annotations.setItemAnnotation(
-      mRoot, BookmarkAnnos.MOBILEROOT_ANNO, 1, 0,
-      PlacesUtils.annotations.EXPIRE_NEVER, PlacesUtils.bookmarks.SOURCE_SYNC);
-    PlacesUtils.annotations.setItemAnnotation(
-      mRoot, BookmarkAnnos.EXCLUDEBACKUP_ANNO, 1, 0,
-      PlacesUtils.annotations.EXPIRE_NEVER, PlacesUtils.bookmarks.SOURCE_SYNC);
-    return mRoot;
-  },
-
-  findMobileRoot: function findMobileRoot(create) {
-    // Use the (one) mobile root if it already exists.
-    let root = PlacesUtils.annotations.getItemsWithAnnotation(
-      BookmarkAnnos.MOBILEROOT_ANNO, {});
-    if (root.length != 0)
-      return root[0];
-
-    if (create)
-      return this.createMobileRoot();
-
-    return null;
-  },
-
   // Accessors for IDs.
   isSpecialGUID: function isSpecialGUID(g) {
     return this.guids.indexOf(g) != -1;
   },
 
-  specialIdForGUID: function specialIdForGUID(guid, create) {
-    if (guid == "mobile") {
-      return this.findMobileRoot(create);
-    }
+  specialIdForGUID: function specialIdForGUID(guid) {
     return this[guid];
   },
 
-  // Don't bother creating mobile: if it doesn't exist, this ID can't be it!
   specialGUIDForId: function specialGUIDForId(id) {
     for (let guid of this.guids)
-      if (this.specialIdForGUID(guid, false) == id)
+      if (this.specialIdForGUID(guid) == id)
         return guid;
     return null;
   },
 
   syncIDToPlacesGUID(g) {
     return g in SpecialGUIDToPlacesGUID ? SpecialGUIDToPlacesGUID[g] : g;
   },
 
@@ -112,11 +75,11 @@ let BookmarkSpecialIds = {
   },
   get toolbar() {
     return PlacesUtils.toolbarFolderId;
   },
   get unfiled() {
     return PlacesUtils.unfiledBookmarksFolderId;
   },
   get mobile() {
-    return this.findMobileRoot(true);
+    return PlacesUtils.mobileFolderId;
   },
 };
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -522,17 +522,17 @@ function BookmarksStore(name, engine) {
     }
     this._stmts = {};
   }, this);
 }
 BookmarksStore.prototype = {
   __proto__: Store.prototype,
 
   itemExists: function BStore_itemExists(id) {
-    return this.idForGUID(id, true) > 0;
+    return this.idForGUID(id) > 0;
   },
 
   applyIncoming: function BStore_applyIncoming(record) {
     this._log.debug("Applying record " + record.id);
     let isSpecial = record.id in BookmarkSpecialIds;
 
     if (record.deleted) {
       if (isSpecial) {
@@ -863,24 +863,22 @@ BookmarksStore.prototype = {
     let special = BookmarkSpecialIds.specialGUIDForId(id);
     if (special)
       return special;
 
     return Async.promiseSpinningly(
       PlacesUtils.promiseItemGuid(id));
   },
 
-  // noCreate is provided as an optional argument to prevent the creation of
-  // non-existent special records, such as "mobile".
-  idForGUID: function idForGUID(guid, noCreate) {
+  idForGUID: function idForGUID(guid) {
     // guid might be a String object rather than a string.
     guid = guid.toString();
 
     if (BookmarkSpecialIds.isSpecialGUID(guid))
-      return BookmarkSpecialIds.specialIdForGUID(guid, !noCreate);
+      return BookmarkSpecialIds.specialIdForGUID(guid);
 
     return Async.promiseSpinningly(PlacesUtils.promiseItemId(guid).catch(
       ex => -1));
   },
 
   _calculateIndex: function _calculateIndex(record) {
     // Ensure folders have a very high sort index so they're not synced last.
     if (record.type == "folder")
@@ -926,39 +924,23 @@ BookmarksStore.prototype = {
       let syncID = BookmarkSpecialIds.specialGUIDForId(id) || guid;
       items[syncID] = { modified: 0, deleted: false };
     }
 
     return items;
   },
 
   wipe: function BStore_wipe() {
-    let cb = Async.makeSpinningCallback();
-    Task.spawn(function* () {
+    Async.promiseSpinningly(Task.spawn(function* () {
       // Save a backup before clearing out all bookmarks.
       yield PlacesBackups.create(null, true);
-      // Instead of exposing a "clear folder" method, we should instead have
-      // `PlacesSyncUtils` clear all roots. `eraseEverything` comes close,
-      // but doesn't clear the mobile root. The fix is to move mobile root
-      // and query handling into Places.
-      for (let syncID of BookmarkSpecialIds.guids) {
-        if (syncID == "places") {
-          continue;
-        }
-        if (syncID == "mobile" && !BookmarkSpecialIds.findMobileRoot(false)) {
-          // `syncIDToPlacesGUID` will create the mobile root as a side effect,
-          // which we don't want it to do if we're clearing bookmarks.
-          continue;
-        }
-        let guid = BookmarkSpecialIds.syncIDToPlacesGUID(syncID);
-        yield PlacesSyncUtils.bookmarks.clear(guid);
-      }
-      cb();
-    });
-    cb.wait();
+      yield PlacesUtils.bookmarks.eraseEverything({
+        source: SOURCE_SYNC,
+      });
+    }));
   }
 };
 
 function BookmarksTracker(name, engine) {
   this._batchDepth = 0;
   this._batchSawScoreIncrement = false;
   Tracker.call(this, name, engine);
 
@@ -1223,26 +1205,22 @@ BookmarksTracker.prototype = {
   },
   onItemVisited: function () {}
 };
 
 // Returns an array of root IDs to recursively query for synced bookmarks.
 // Items in other roots, including tags and organizer queries, will be
 // ignored.
 function getChangeRootIds() {
-  let rootIds = [
+  return [
     PlacesUtils.bookmarksMenuFolderId,
     PlacesUtils.toolbarFolderId,
     PlacesUtils.unfiledBookmarksFolderId,
+    PlacesUtils.mobileFolderId,
   ];
-  let mobileRootId = BookmarkSpecialIds.findMobileRoot(false);
-  if (mobileRootId) {
-    rootIds.push(mobileRootId);
-  }
-  return rootIds;
 }
 
 class BookmarksChangeset extends Changeset {
   getModifiedTimestamp(id) {
     let change = this.changes[id];
     return change ? change.modified : Number.NaN;
   }
 }
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -139,60 +139,16 @@ add_task(function* bad_record_allIDs() {
   do_check_true("menu" in all);
   do_check_true("toolbar" in all);
 
   _("Clean up.");
   PlacesUtils.bookmarks.removeItem(badRecordID);
   yield new Promise(r => server.stop(r));
 });
 
-add_task(function* test_ID_caching() {
-  let server = new SyncServer();
-  server.start();
-  let syncTesting = new SyncTestingInfrastructure(server.server);
-
-  _("Ensure that Places IDs are not cached.");
-  let engine = new BookmarksEngine(Service);
-  let store = engine._store;
-  _("All IDs: " + JSON.stringify(store.getAllIDs()));
-
-  let mobileID = store.idForGUID("mobile");
-  _("Change the GUID for that item, and drop the mobile anno.");
-  let mobileRoot = BookmarkSpecialIds.specialIdForGUID("mobile", false);
-  let mobileGUID = yield PlacesUtils.promiseItemGuid(mobileRoot);
-  yield PlacesSyncUtils.bookmarks.changeGuid(mobileGUID, "abcdefghijkl");
-  PlacesUtils.annotations.removeItemAnnotation(mobileID, "mobile/bookmarksRoot");
-
-  let err;
-  let newMobileID;
-
-  // With noCreate, we don't find an entry.
-  try {
-    newMobileID = store.idForGUID("mobile", true);
-    _("New mobile ID: " + newMobileID);
-  } catch (ex) {
-    err = ex;
-    _("Error: " + Log.exceptionStr(err));
-  }
-
-  do_check_true(!err);
-
-  // With !noCreate, lookup works, and it's different.
-  newMobileID = store.idForGUID("mobile", false);
-  _("New mobile ID: " + newMobileID);
-  do_check_true(!!newMobileID);
-  do_check_neq(newMobileID, mobileID);
-
-  // And it's repeatable, even with creation enabled.
-  do_check_eq(newMobileID, store.idForGUID("mobile", false));
-
-  do_check_eq(store.GUIDForId(mobileID), "abcdefghijkl");
-  yield new Promise(r => server.stop(r));
-});
-
 function serverForFoo(engine) {
   return serverForUsers({"foo": "password"}, {
     meta: {global: {engines: {bookmarks: {version: engine.version,
                                           syncID: engine.syncID}}}},
     bookmarks: {}
   });
 }
 
--- a/services/sync/tests/unit/test_bookmark_order.js
+++ b/services/sync/tests/unit/test_bookmark_order.js
@@ -48,16 +48,19 @@ add_task(function* test_bookmark_order()
   }, {
     guid: PlacesUtils.bookmarks.toolbarGuid,
     index: 1,
   }, {
     // Index 2 is the tags root. (Root indices depend on the order of the
     // `CreateRoot` calls in `Database::CreateBookmarkRoots`).
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "clean slate");
 
   function bookmark(name, parent) {
     let bookmark = new Bookmark("http://weave.server/my-bookmark");
     bookmark.id = name;
     bookmark.title = name;
     bookmark.bmkUri = "http://uri/";
     bookmark.parentid = parent || "unfiled";
@@ -91,16 +94,19 @@ add_task(function* test_bookmark_order()
     index: 1,
   }, {
     guid: PlacesUtils.bookmarks.unfiledGuid,
     index: 3,
     children: [{
       guid: id10,
       index: 0,
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "basic add first bookmark");
   let id20 = "20_aaaaaaaaa";
   _("basic append behind 10");
   apply(bookmark(id20, ""));
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
@@ -111,16 +117,19 @@ add_task(function* test_bookmark_order()
     index: 3,
     children: [{
       guid: id10,
       index: 0,
     }, {
       guid: id20,
       index: 1,
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "basic append behind 10");
 
   let id31 = "31_aaaaaaaaa";
   let id30 = "f30_aaaaaaaa";
   _("basic create in folder");
   apply(bookmark(id31, id30));
   let f30 = folder(id30, "", [id31]);
   apply(f30);
@@ -142,16 +151,19 @@ add_task(function* test_bookmark_order()
     }, {
       guid: id30,
       index: 2,
       children: [{
         guid: id31,
         index: 0,
       }],
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "basic create in folder");
 
   let id41 = "41_aaaaaaaaa";
   let id40 = "f40_aaaaaaaa";
   _("insert missing parent -> append to unfiled");
   apply(bookmark(id41, id40));
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -175,16 +187,19 @@ add_task(function* test_bookmark_order()
         guid: id31,
         index: 0,
       }],
     }, {
       guid: id41,
       index: 3,
       requestedParent: id40,
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "insert missing parent -> append to unfiled");
 
   let id42 = "42_aaaaaaaaa";
 
   _("insert another missing parent -> append");
   apply(bookmark(id42, id40));
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -212,16 +227,19 @@ add_task(function* test_bookmark_order()
       guid: id41,
       index: 3,
       requestedParent: id40,
     }, {
       guid: id42,
       index: 4,
       requestedParent: id40,
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "insert another missing parent -> append");
 
   _("insert folder -> move children and followers");
   let f40 = folder(id40, "", [id41, id42]);
   apply(f40);
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
@@ -250,16 +268,19 @@ add_task(function* test_bookmark_order()
       children: [{
         guid: id41,
         index: 0,
       }, {
         guid: id42,
         index: 1,
       }]
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "insert folder -> move children and followers");
 
   _("Moving 41 behind 42 -> update f40");
   f40.children = [id42, id41];
   apply(f40);
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
@@ -288,16 +309,19 @@ add_task(function* test_bookmark_order()
       children: [{
         guid: id42,
         index: 0,
       }, {
         guid: id41,
         index: 1,
       }]
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "Moving 41 behind 42 -> update f40");
 
   _("Moving 10 back to front -> update 10, 20");
   f40.children = [id41, id42];
   apply(f40);
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
@@ -326,16 +350,19 @@ add_task(function* test_bookmark_order()
       children: [{
         guid: id41,
         index: 0,
       }, {
         guid: id42,
         index: 1,
       }]
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "Moving 10 back to front -> update 10, 20");
 
   _("Moving 20 behind 42 in f40 -> update 50");
   apply(bookmark(id20, id40));
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
     index: 0,
   }, {
@@ -363,16 +390,19 @@ add_task(function* test_bookmark_order()
       }, {
         guid: id42,
         index: 1,
       }, {
         guid: id20,
         index: 2,
       }]
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "Moving 20 behind 42 in f40 -> update 50");
 
   _("Moving 10 in front of 31 in f30 -> update 10, f30");
   apply(bookmark(id10, id30));
   f30.children = [id10, id31];
   apply(f30);
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -402,16 +432,19 @@ add_task(function* test_bookmark_order()
       }, {
         guid: id42,
         index: 1,
       }, {
         guid: id20,
         index: 2,
       }]
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "Moving 10 in front of 31 in f30 -> update 10, f30");
 
   _("Moving 20 from f40 to f30 -> update 20, f30");
   apply(bookmark(id20, id30));
   f30.children = [id10, id20, id31];
   apply(f30);
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -441,16 +474,19 @@ add_task(function* test_bookmark_order()
       children: [{
         guid: id41,
         index: 0,
       }, {
         guid: id42,
         index: 1,
       }]
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "Moving 20 from f40 to f30 -> update 20, f30");
 
   _("Move 20 back to front -> update 20, f30");
   apply(bookmark(id20, ""));
   f30.children = [id10, id31];
   apply(f30);
   yield check([{
     guid: PlacesUtils.bookmarks.menuGuid,
@@ -480,11 +516,14 @@ add_task(function* test_bookmark_order()
       }, {
         guid: id42,
         index: 1,
       }],
     }, {
       guid: id20,
       index: 2,
     }],
+  }, {
+    guid: PlacesUtils.bookmarks.mobileGuid,
+    index: 4,
   }], "Move 20 back to front -> update 20, f30");
 
 });
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -1266,29 +1266,26 @@ add_task(function* test_async_onItemDele
 });
 
 add_task(function* test_async_onItemDeleted_eraseEverything() {
   _("Erasing everything should track all deleted items");
 
   try {
     yield stopTracking();
 
-    let mobileRoot = BookmarkSpecialIds.findMobileRoot(true);
-    let mobileGUID = yield PlacesUtils.promiseItemGuid(mobileRoot);
-
     let fxBmk = yield PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
-      parentGuid: mobileGUID,
+      parentGuid: PlacesUtils.bookmarks.mobileGuid,
       url: "http://getfirefox.com",
       title: "Get Firefox!",
     });
     _(`Firefox GUID: ${fxBmk.guid}`);
     let tbBmk = yield PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
-      parentGuid: mobileGUID,
+      parentGuid: PlacesUtils.bookmarks.mobileGuid,
       url: "http://getthunderbird.com",
       title: "Get Thunderbird!",
     });
     _(`Thunderbird GUID: ${tbBmk.guid}`);
     let mozBmk = yield PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
       parentGuid: PlacesUtils.bookmarks.menuGuid,
       url: "https://mozilla.org",
@@ -1334,40 +1331,39 @@ add_task(function* test_async_onItemDele
     yield PlacesUtils.bookmarks.eraseEverything();
 
     // `eraseEverything` removes all items from the database before notifying
     // observers. Because of this, grandchild lookup in the tracker's
     // `onItemRemoved` observer will fail. That means we won't track
     // (bzBmk.guid, bugsGrandChildBmk.guid, bugsChildFolder.guid), even
     // though we should.
     yield verifyTrackedItems(["menu", mozBmk.guid, mdnBmk.guid, "toolbar",
-                              bugsFolder.guid]);
-    do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE * 6);
+                              bugsFolder.guid, "mobile", fxBmk.guid,
+                              tbBmk.guid]);
+    do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE * 10);
   } finally {
     _("Clean up.");
     yield cleanup();
   }
 });
 
 add_task(function* test_onItemDeleted_removeFolderChildren() {
   _("Removing a folder's children should track the folder and its children");
 
   try {
-    let mobileRoot = BookmarkSpecialIds.findMobileRoot(true);
-    let mobileGUID = engine._store.GUIDForId(mobileRoot);
     let fx_id = PlacesUtils.bookmarks.insertBookmark(
-      mobileRoot,
+      PlacesUtils.mobileFolderId,
       Utils.makeURI("http://getfirefox.com"),
       PlacesUtils.bookmarks.DEFAULT_INDEX,
       "Get Firefox!");
     let fx_guid = engine._store.GUIDForId(fx_id);
     _(`Firefox GUID: ${fx_guid}`);
 
     let tb_id = PlacesUtils.bookmarks.insertBookmark(
-      mobileRoot,
+      PlacesUtils.mobileFolderId,
       Utils.makeURI("http://getthunderbird.com"),
       PlacesUtils.bookmarks.DEFAULT_INDEX,
       "Get Thunderbird!");
     let tb_guid = engine._store.GUIDForId(tb_id);
     _(`Thunderbird GUID: ${tb_guid}`);
 
     let moz_id = PlacesUtils.bookmarks.insertBookmark(
       PlacesUtils.bookmarks.bookmarksMenuFolder,
@@ -1375,18 +1371,18 @@ add_task(function* test_onItemDeleted_re
       PlacesUtils.bookmarks.DEFAULT_INDEX,
       "Mozilla"
     );
     let moz_guid = engine._store.GUIDForId(moz_id);
     _(`Mozilla GUID: ${moz_guid}`);
 
     yield startTracking();
 
-    _(`Mobile root ID: ${mobileRoot}`);
-    PlacesUtils.bookmarks.removeFolderChildren(mobileRoot);
+    _(`Mobile root ID: ${PlacesUtils.mobileFolderId}`);
+    PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.mobileFolderId);
 
     yield verifyTrackedItems(["mobile", fx_guid, tb_guid]);
     do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE * 4);
   } finally {
     _("Clean up.");
     yield cleanup();
   }
 });