Bug 1295521 - Add a `toSyncBookmark` method and clean up `BookmarksStore`. r=markh draft
authorKit Cambridge <kcambridge@mozilla.com>
Tue, 20 Sep 2016 01:52:58 -0700
changeset 423753 0d657035d02c8bd261f1124072d6a248c94c7e09
parent 423752 40f23512b976595a094bd4491db434958ae381a1
child 423848 8e44090aebdbafb0f1effc7ea86aecbfadf86519
push id31975
push userbmo:kcambridge@mozilla.com
push dateTue, 11 Oct 2016 15:53:22 +0000
reviewersmarkh
bugs1295521
milestone52.0a1
Bug 1295521 - Add a `toSyncBookmark` method and clean up `BookmarksStore`. r=markh MozReview-Commit-ID: 3h2qnOtNPN9
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/PlacesUtils.jsm
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -84,64 +84,106 @@ PlacesItem.prototype = {
       case "item":
         return PlacesItem;
     }
     throw "Unknown places item object type: " + type;
   },
 
   __proto__: CryptoWrapper.prototype,
   _logName: "Sync.Record.PlacesItem",
+
+  // Converts the record to a Sync bookmark object that can be passed to
+  // `PlacesSyncUtils.bookmarks.{insert, update}`.
+  toSyncBookmark() {
+    return {
+      kind: this.type,
+      syncId: this.id,
+      parentSyncId: this.parentid,
+    };
+  },
 };
 
 Utils.deferGetSet(PlacesItem,
                   "cleartext",
                   ["hasDupe", "parentid", "parentName", "type"]);
 
 this.Bookmark = function Bookmark(collection, id, type) {
   PlacesItem.call(this, collection, id, type || "bookmark");
 }
 Bookmark.prototype = {
   __proto__: PlacesItem.prototype,
   _logName: "Sync.Record.Bookmark",
+
+  toSyncBookmark() {
+    let info = PlacesItem.prototype.toSyncBookmark.call(this);
+    info.title = this.title;
+    info.url = this.bmkUri;
+    info.description = this.description;
+    info.loadInSidebar = this.loadInSidebar;
+    info.tags = this.tags;
+    info.keyword = this.keyword;
+    return info;
+  },
 };
 
 Utils.deferGetSet(Bookmark,
                   "cleartext",
                   ["title", "bmkUri", "description",
                    "loadInSidebar", "tags", "keyword"]);
 
 this.BookmarkQuery = function BookmarkQuery(collection, id) {
   Bookmark.call(this, collection, id, "query");
 }
 BookmarkQuery.prototype = {
   __proto__: Bookmark.prototype,
   _logName: "Sync.Record.BookmarkQuery",
+
+  toSyncBookmark() {
+    let info = Bookmark.prototype.toSyncBookmark.call(this);
+    info.folder = this.folderName;
+    info.query = this.queryId;
+    return info;
+  },
 };
 
 Utils.deferGetSet(BookmarkQuery,
                   "cleartext",
                   ["folderName", "queryId"]);
 
 this.BookmarkFolder = function BookmarkFolder(collection, id, type) {
   PlacesItem.call(this, collection, id, type || "folder");
 }
 BookmarkFolder.prototype = {
   __proto__: PlacesItem.prototype,
   _logName: "Sync.Record.Folder",
+
+  toSyncBookmark() {
+    let info = PlacesItem.prototype.toSyncBookmark.call(this);
+    info.description = this.description;
+    info.title = this.title;
+    return info;
+  },
 };
 
 Utils.deferGetSet(BookmarkFolder, "cleartext", ["description", "title",
                                                 "children"]);
 
 this.Livemark = function Livemark(collection, id) {
   BookmarkFolder.call(this, collection, id, "livemark");
 }
 Livemark.prototype = {
   __proto__: BookmarkFolder.prototype,
   _logName: "Sync.Record.Livemark",
+
+  toSyncBookmark() {
+    let info = BookmarkFolder.prototype.toSyncBookmark.call(this);
+    info.feed = this.feedUri;
+    info.site = this.siteUri;
+    return info;
+  },
 };
 
 Utils.deferGetSet(Livemark, "cleartext", ["siteUri", "feedUri"]);
 
 this.BookmarkSeparator = function BookmarkSeparator(collection, id) {
   PlacesItem.call(this, collection, id, "separator");
 }
 BookmarkSeparator.prototype = {
@@ -621,122 +663,46 @@ BookmarksStore.prototype = {
     Store.prototype.applyIncoming.call(this, record);
 
     if (record.type == "folder" && record.children) {
       this._childrenToOrder[record.id] = record.children;
     }
   },
 
   create: function BStore_create(record) {
-    switch (record.type) {
-    case "bookmark":
-    case "query":
-    case "microsummary": {
-      let info = {
-        kind: record.type,
-        url: record.bmkUri,
-        parentSyncId: record.parentid,
-        title: record.title,
-        syncId: record.id,
-        tags: record.tags,
-        keyword: record.keyword,
-        loadInSidebar: record.loadInSidebar,
-        query: record.queryId,
-        folder: record.folderName,
-        description: record.description,
-      };
-
-      let bmk = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.insert(info));
-      this._log.debug("created bookmark " + bmk.syncId + " under " + bmk.parentSyncId
-                      + " as " + bmk.title + " " + bmk.url.href);
-
-    } break;
-    case "folder": {
-      let info = {
-        kind: PlacesSyncUtils.bookmarks.KINDS.FOLDER,
-        parentSyncId: record.parentid,
-        syncId: record.id,
-        title: record.title,
-        description: record.description,
-      };
-
-      let folder = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.insert(info));
-      this._log.debug("created folder " + folder.syncId + " under " + folder.parentSyncId
-                      + " as " + folder.title);
-
-      // record.children will be dealt with in _orderChildren.
-    } break;
-    case "livemark":
-      if (!record.feedUri) {
-        this._log.debug("No feed URI: skipping livemark record " + record.id);
-        return;
-      }
-      let info = {
-        kind: PlacesSyncUtils.bookmarks.KINDS.LIVEMARK,
-        title: record.title,
-        parentSyncId: record.parentid,
-        feed: record.feedUri,
-        site: record.siteUri,
-        syncId: record.id,
-      };
-      let livemark = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.insert(info));
-      if (livemark) {
-        this._log.debug("Created livemark " + livemark.syncId + " under " +
-                        livemark.parentSyncId + " as " + livemark.title +
-                        ", " + livemark.site.href + ", " +
-                        livemark.feed.href);
-      }
-      break;
-    case "separator": {
-      let separator = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.insert({
-        kind: PlacesSyncUtils.bookmarks.KINDS.SEPARATOR,
-        parentSyncId: record.parentid,
-        syncId: record.id,
-      }));
-      this._log.debug("created separator " + separator.syncId + " under " + separator.parentSyncId);
-    } break;
-    case "item":
-      this._log.debug(" -> got a generic places item.. do nothing?");
-      return;
-    default:
-      this._log.error("_create: Unknown item type: " + record.type);
-      return;
+    let info = record.toSyncBookmark();
+    // This can throw if we're inserting an invalid or incomplete bookmark.
+    // That's fine; the exception will be caught by `applyIncomingBatch`
+    // without aborting further processing.
+    let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.insert(info));
+    if (item) {
+      this._log.debug(`Created ${item.kind} ${item.syncId} under ${
+        item.parentSyncId}`, item);
     }
   },
 
   remove: function BStore_remove(record) {
     try {
       let info = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.remove(record.id));
       if (info) {
         this._log.debug(`Removed item ${record.id} with type ${record.type}`);
       }
     } catch (ex) {
       // Likely already removed.
       this._log.debug(`Error removing ${record.id}`, ex);
     }
   },
 
   update: function BStore_update(record) {
-    let info = {
-      parentSyncId: record.parentid,
-      syncId: record.id,
-      kind: record.type,
-      title: record.title,
-      url: record.bmkUri,
-      tags: record.tags,
-      keyword: record.keyword,
-      description: record.description,
-      loadInSidebar: record.loadInSidebar,
-      query: record.queryId,
-      site: record.siteUri,
-      feed: record.feedUri,
-    };
-
-    let bmk = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.update(info));
-    this._log.debug("updated bookmark " + bmk.syncId + " under " + bmk.parentSyncId);
+    let info = record.toSyncBookmark();
+    let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.update(info));
+    if (item) {
+      this._log.debug(`Updated ${item.kind} ${item.syncId} under ${
+        item.parentSyncId}`, item);
+    }
   },
 
   _orderChildren: function _orderChildren() {
     let promises = Object.keys(this._childrenToOrder).map(syncID => {
       let children = this._childrenToOrder[syncID];
       return PlacesSyncUtils.bookmarks.order(syncID, children).catch(ex => {
         this._log.debug(`Could not order children for ${syncID}`, ex);
       });
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -354,16 +354,18 @@ add_task(function* test_restorePromptsRe
   }
 });
 
 function FakeRecord(constructor, r) {
   constructor.call(this, "bookmarks", r.id);
   for (let x in r) {
     this[x] = r[x];
   }
+  // Borrow the constructor's conversion functions.
+  this.toSyncBookmark = constructor.prototype.toSyncBookmark;
 }
 
 // Bug 632287.
 add_task(function* test_mismatched_types() {
   _("Ensure that handling a record that changes type causes deletion " +
     "then re-adding.");
 
   let oldRecord = {
@@ -509,27 +511,24 @@ add_task(function* test_bookmark_tag_but
   yield PlacesSyncUtils.bookmarks.insert({
     kind: PlacesSyncUtils.bookmarks.KINDS.BOOKMARK,
     syncId: Utils.makeGUID(),
     url: "about:fake",
     parentSyncId: "toolbar",
     tags: null,
   });
 
-  let record = {
+  let record = new FakeRecord(BookmarkFolder, {
     parentid:    "toolbar",
     id:          Utils.makeGUID(),
     description: "",
     tags:        ["foo"],
     title:       "Taggy tag",
     type:        "folder"
-  };
-
-  // Because update() walks the cleartext.
-  record.cleartext = record;
+  });
 
   store.create(record);
   record.tags = ["bar"];
   store.update(record);
 });
 
 add_task(function* test_misreconciled_root() {
   _("Ensure that we don't reconcile an arbitrary record with a root.");
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -453,16 +453,21 @@ var insertSyncBookmark = Task.async(func
   // Reparent all orphans that expect this folder as the parent.
   yield reparentOrphans(newItem);
 
   return newItem;
 });
 
 // Inserts a synced livemark.
 var insertSyncLivemark = Task.async(function* (insertInfo) {
+  if (!insertInfo.feed) {
+    BookmarkSyncLog.debug(`insertSyncLivemark: ${
+      insertInfo.syncId} missing feed URL`);
+    return null;
+  }
   let livemarkInfo = syncBookmarkToPlacesBookmark(insertInfo);
   let parentId = yield PlacesUtils.promiseItemId(livemarkInfo.parentGuid);
   let parentIsLivemark = PlacesUtils.annotations.itemHasAnnotation(parentId,
     PlacesUtils.LMANNO_FEEDURI);
   if (parentIsLivemark) {
     // A livemark can't be a descendant of another livemark.
     BookmarkSyncLog.debug(`insertSyncLivemark: Invalid parent ${
       insertInfo.parentSyncId}; skipping livemark record ${
@@ -801,18 +806,17 @@ function validateNewBookmark(info) {
     , description: { validIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK
                                    , BookmarkSyncUtils.KINDS.MICROSUMMARY
                                    , BookmarkSyncUtils.KINDS.QUERY
                                    , BookmarkSyncUtils.KINDS.FOLDER
                                    , BookmarkSyncUtils.KINDS.LIVEMARK ].includes(b.kind) }
     , loadInSidebar: { validIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK
                                      , BookmarkSyncUtils.KINDS.MICROSUMMARY
                                      , BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind) }
-    , feed: { requiredIf: b => b.kind == BookmarkSyncUtils.KINDS.LIVEMARK
-            , validIf: b => b.kind == BookmarkSyncUtils.KINDS.LIVEMARK }
+    , feed: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.LIVEMARK }
     , site: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.LIVEMARK }
     });
 
   return insertInfo;
 }
 
 function findAnnoItems(anno, val) {
   let annos = PlacesUtils.annotations;
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -282,17 +282,17 @@ const SYNC_BOOKMARK_VALIDATORS = Object.
         throw new Error(`Invalid tag: ${tag}`);
       }
     }
     return v;
   },
   keyword: simpleValidateFunc(v => v === null || typeof v == "string"),
   description: simpleValidateFunc(v => v === null || typeof v == "string"),
   loadInSidebar: simpleValidateFunc(v => v === true || v === false),
-  feed: BOOKMARK_VALIDATORS.url,
+  feed: v => v === null ? v : BOOKMARK_VALIDATORS.url(v),
   site: v => v === null ? v : BOOKMARK_VALIDATORS.url(v),
   title: BOOKMARK_VALIDATORS.title,
   url: BOOKMARK_VALIDATORS.url,
 });
 
 this.PlacesUtils = {
   // Place entries that are containers, e.g. bookmark folders or queries.
   TYPE_X_MOZ_PLACE_CONTAINER: "text/x-moz-place-container",