Bug 1302286 - Remove `_getNode` and `_getChildren` from the bookmarks engine. r?markh draft
authorKit Cambridge <kcambridge@mozilla.com>
Tue, 13 Sep 2016 12:07:54 -0700
changeset 413159 c51773d43a95167a47bbfa0ea8d941790c8b95cd
parent 413158 b9f06775d0fdf2a7b03758eb12bed43e58986540
child 531167 82803bf62b2090d447411fb836765de117859ebc
push id29366
push userkcambridge@mozilla.com
push dateTue, 13 Sep 2016 19:12:05 +0000
reviewersmarkh
bugs1302286
milestone51.0a1
Bug 1302286 - Remove `_getNode` and `_getChildren` from the bookmarks engine. r?markh MozReview-Commit-ID: 10HScO81AK3
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
services/sync/tests/unit/test_bookmark_places_query_rewriting.js
toolkit/components/places/PlacesSyncUtils.jsm
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -426,21 +426,17 @@ BookmarksEngine.prototype = {
     let mapped = this._mapDupe(item);
     this._log.debug(item.id + " mapped to " + mapped);
     // We must return a string, not an object, and the entries in the GUIDMap
     // are created via "new String()" making them an object.
     return mapped ? mapped.toString() : mapped;
   },
 
   pullAllChanges() {
-    let changeset = new BookmarksChangeset();
-    for (let id in this._store.getAllIDs()) {
-      changeset.set(id, { modified: 0, deleted: false });
-    }
-    return changeset;
+    return new BookmarksChangeset(this._store.getAllIDs());
   },
 
   pullNewChanges() {
     let modifiedGUIDs = this._getModifiedGUIDs();
     if (!modifiedGUIDs.length) {
       return new BookmarksChangeset(this._tracker.changedIDs);
     }
 
@@ -456,25 +452,26 @@ BookmarksEngine.prototype = {
     // of bound parameters per query.
     for (let startIndex = 0;
          startIndex < modifiedGUIDs.length;
          startIndex += SQLITE_MAX_VARIABLE_NUMBER) {
 
       let chunkLength = Math.min(startIndex + SQLITE_MAX_VARIABLE_NUMBER,
                                  modifiedGUIDs.length);
 
+      // Change root IDs don't need to be escaped because they're integers and
+      // not vulnerable to injections.
+      let changeRootIds = PlacesSyncUtils.bookmarks.getChangeRootIds();
       let query = `
         WITH RECURSIVE
         modifiedGuids(guid) AS (
           VALUES ${new Array(chunkLength).fill("(?)").join(", ")}
         ),
         syncedItems(id) AS (
-          SELECT b.id
-          FROM moz_bookmarks b
-          WHERE b.id IN (${getChangeRootIds().join(", ")})
+          VALUES ${changeRootIds.map(id => `(${id})`).join(", ")}
           UNION ALL
           SELECT b.id
           FROM moz_bookmarks b
           JOIN syncedItems s ON b.parent = s.id
         )
         SELECT b.guid, b.id
         FROM modifiedGuids m
         JOIN moz_bookmarks b ON b.guid = m.guid
@@ -722,23 +719,16 @@ BookmarksStore.prototype = {
     this._log.debug("Changing GUID " + oldID + " to " + newID);
 
     Async.promiseSpinningly(PlacesSyncUtils.bookmarks.changeGuid(
       BookmarkSpecialIds.syncIDToPlacesGUID(oldID),
       BookmarkSpecialIds.syncIDToPlacesGUID(newID)
     ));
   },
 
-  _getNode: function BStore__getNode(folder) {
-    let query = PlacesUtils.history.getNewQuery();
-    query.setFolders([folder], 1);
-    return PlacesUtils.history.executeQuery(
-      query, PlacesUtils.history.getNewQueryOptions()).root;
-  },
-
   _getTags: function BStore__getTags(uri) {
     try {
       if (typeof(uri) == "string")
         uri = Utils.makeURI(uri);
     } catch(e) {
       this._log.warn("Could not parse URI \"" + uri + "\": " + e);
     }
     return PlacesUtils.tagging.getTagsForURI(uri, {});
@@ -911,61 +901,41 @@ BookmarksStore.prototype = {
       let result = Async.querySpinningly(this._frecencyStm, this._frecencyCols);
       if (result.length)
         index += result[0].frecency;
     }
 
     return index;
   },
 
-  _getChildren: function BStore_getChildren(guid, items) {
-    let node = guid; // the recursion case
-    if (typeof(node) == "string") { // callers will give us the guid as the first arg
-      let nodeID = this.idForGUID(guid, true);
-      if (!nodeID) {
-        this._log.debug("No node for GUID " + guid + "; returning no children.");
-        return items;
-      }
-      node = this._getNode(nodeID);
+  getAllIDs: function BStore_getAllIDs() {
+    let items = {};
+
+    let changeRootIds = PlacesSyncUtils.bookmarks.getChangeRootIds();
+    let query = `
+      WITH RECURSIVE
+      changeRootContents(id) AS (
+        VALUES ${changeRootIds.map(id => `(${id})`).join(", ")}
+        UNION ALL
+        SELECT b.id
+        FROM moz_bookmarks b
+        JOIN changeRootContents c ON b.parent = c.id
+      )
+      SELECT id, guid
+      FROM changeRootContents
+      JOIN moz_bookmarks USING (id)
+    `;
+
+    let statement = this._getStmt(query);
+    let results = Async.querySpinningly(statement, ["id", "guid"]);
+    for (let { id, guid } of results) {
+      let syncID = BookmarkSpecialIds.specialGUIDForId(id) || guid;
+      items[syncID] = { modified: 0, deleted: false };
     }
 
-    if (node.type == node.RESULT_TYPE_FOLDER) {
-      node.QueryInterface(Ci.nsINavHistoryQueryResultNode);
-      node.containerOpen = true;
-      try {
-        // Remember all the children GUIDs and recursively get more
-        for (let i = 0; i < node.childCount; i++) {
-          let child = node.getChild(i);
-          items[this.GUIDForId(child.itemId)] = true;
-          this._getChildren(child, items);
-        }
-      }
-      finally {
-        node.containerOpen = false;
-      }
-    }
-
-    return items;
-  },
-
-  getAllIDs: function BStore_getAllIDs() {
-    let items = {"menu": true,
-                 "toolbar": true,
-                 "unfiled": true,
-                };
-    // We also want "mobile" but only if a local mobile folder already exists
-    // (otherwise we'll later end up creating it, which we want to avoid until
-    // we actually need it.)
-    if (BookmarkSpecialIds.findMobileRoot(false)) {
-      items["mobile"] = true;
-    }
-    for (let guid of BookmarkSpecialIds.guids) {
-      if (guid != "places" && guid != "tags")
-        this._getChildren(guid, items);
-    }
     return items;
   },
 
   wipe: function BStore_wipe() {
     let cb = Async.makeSpinningCallback();
     Task.spawn(function* () {
       // Save a backup before clearing out all bookmarks.
       yield PlacesBackups.create(null, true);
@@ -1253,30 +1223,14 @@ BookmarksTracker.prototype = {
     if (--this._batchDepth === 0 && this._batchSawScoreIncrement) {
       this.score += SCORE_INCREMENT_XLARGE;
       this._batchSawScoreIncrement = false;
     }
   },
   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 = [
-    PlacesUtils.bookmarksMenuFolderId,
-    PlacesUtils.toolbarFolderId,
-    PlacesUtils.unfiledBookmarksFolderId,
-  ];
-  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
@@ -127,19 +127,16 @@ add_task(function* bad_record_allIDs() {
       Utils.makeURI("place:folder=1138"),
       PlacesUtils.bookmarks.DEFAULT_INDEX,
       null);
 
   do_check_true(badRecordID > 0);
   _("Record is " + badRecordID);
   _("Type: " + PlacesUtils.bookmarks.getItemType(badRecordID));
 
-  _("Fetching children.");
-  store._getChildren("toolbar", {});
-
   _("Fetching all IDs.");
   let all = store.getAllIDs();
 
   _("All IDs: " + JSON.stringify(all));
   do_check_true("menu" in all);
   do_check_true("toolbar" in all);
 
   _("Clean up.");
--- a/services/sync/tests/unit/test_bookmark_places_query_rewriting.js
+++ b/services/sync/tests/unit/test_bookmark_places_query_rewriting.js
@@ -28,25 +28,28 @@ function run_test() {
 
   let uri = "place:folder=499&type=7&queryType=1";
   let tagRecord = makeTagRecord("abcdefabcdef", uri);
 
   _("Type: " + tagRecord.type);
   _("Folder name: " + tagRecord.folderName);
   store.applyIncoming(tagRecord);
 
-  let tags = store._getNode(PlacesUtils.tagsFolderId);
-  tags.containerOpen = true;
+  let tags = PlacesUtils.getFolderContents(PlacesUtils.tagsFolderId).root;
   let tagID;
-  for (let i = 0; i < tags.childCount; ++i) {
-    let child = tags.getChild(i);
-    if (child.title == "bar")
-      tagID = child.itemId;
+  try {
+    for (let i = 0; i < tags.childCount; ++i) {
+      let child = tags.getChild(i);
+      if (child.title == "bar") {
+        tagID = child.itemId;
+      }
+    }
+  } finally {
+    tags.containerOpen = false;
   }
-  tags.containerOpen = false;
 
   _("Tag ID: " + tagID);
   let insertedRecord = store.createRecord("abcdefabcdef", "bookmarks");
   do_check_eq(insertedRecord.bmkUri, uri.replace("499", tagID));
 
   _("... but not if the type is wrong.");
   let wrongTypeURI = "place:folder=499&type=2&queryType=1";
   let wrongTypeRecord = makeTagRecord("fedcbafedcba", wrongTypeURI);
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -32,29 +32,50 @@ var PlacesSyncUtils = {};
 const SMART_BOOKMARKS_ANNO = "Places/SmartBookmark";
 const DESCRIPTION_ANNO = "bookmarkProperties/description";
 const SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
 const PARENT_ANNO = "sync/parent";
 
 const { SOURCE_SYNC } = Ci.nsINavBookmarksService;
 
 const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
+  SYNC_MOBILE_ROOT_ANNO: "mobile/bookmarksRoot",
+
   KINDS: {
     BOOKMARK: "bookmark",
     // Microsummaries were removed from Places in bug 524091. For now, Sync
     // treats them identically to bookmarks. Bug 745410 tracks removing them
     // entirely.
     MICROSUMMARY: "microsummary",
     QUERY: "query",
     FOLDER: "folder",
     LIVEMARK: "livemark",
     SEPARATOR: "separator",
   },
 
   /**
+   * Returns an array of Places root IDs to recursively query for synced
+   * bookmarks. This includes the menu, toolbar, unfiled bookmarks, and
+   * mobile bookmarks. Items in other roots, like tags and left pane queries,
+   * are not tracked by Sync.
+   */
+  getChangeRootIds() {
+    let rootIds = [
+      PlacesUtils.bookmarksMenuFolderId,
+      PlacesUtils.toolbarFolderId,
+      PlacesUtils.unfiledBookmarksFolderId,
+    ];
+    let mobileRootId = findMobileRoot();
+    if (mobileRootId) {
+      rootIds.push(mobileRootId);
+    }
+    return rootIds;
+  },
+
+  /**
    * Fetches a folder's children, ordered by their position within the folder.
    */
   fetchChildGuids: Task.async(function* (parentGuid) {
     PlacesUtils.SYNC_BOOKMARK_VALIDATORS.guid(parentGuid);
 
     let db = yield PlacesUtils.promiseDBConnection();
     let children = yield fetchAllChildren(db, parentGuid);
     return children.map(child => child.guid);
@@ -852,8 +873,16 @@ var getOrCreateTagFolder = Task.async(fu
   // Create the tag if it doesn't exist.
   let item = yield PlacesUtils.bookmarks.insert({
     type: PlacesUtils.bookmarks.TYPE_FOLDER,
     parentGuid: PlacesUtils.bookmarks.tagsGuid,
     title: tag,
   });
   return PlacesUtils.promiseItemId(item.guid);
 });
+
+// Returns the Sync mobile bookmarks root ID, or `null` if one doesn't exist.
+// This function will be removed in bug 647605.
+function findMobileRoot() {
+  let mobileRoot = PlacesUtils.annotations.getItemsWithAnnotation(
+    BookmarkSyncUtils.SYNC_MOBILE_ROOT_ANNO, {});
+  return mobileRoot.length ? mobileRoot[0] : null;
+}