switch to generating individual bookmark records directly from the places store, without using a cache (to improve memory performance); create objects for bookmark types; temporarily disable history sync
authorDan Mills <thunder@mozilla.com>
Sun, 28 Dec 2008 19:59:44 -0800
changeset 45144 30590ed4964197652154756f7ec36fde084924aa
parent 45143 4adb437b889a0e2353793d33824352bbbca68abf
child 45145 7cd4d0d6f13b17482a3f2659f0b892e3172d3fa8
push idunknown
push userunknown
push dateunknown
switch to generating individual bookmark records directly from the places store, without using a cache (to improve memory performance); create objects for bookmark types; temporarily disable history sync
services/sync/modules/base_records/crypto.js
services/sync/modules/base_records/keys.js
services/sync/modules/base_records/wbo.js
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/modules/type_records/bookmark.js
--- a/services/sync/modules/base_records/crypto.js
+++ b/services/sync/modules/base_records/crypto.js
@@ -42,17 +42,16 @@ const Cr = Components.results;
 const Cu = Components.utils;
 
 Cu.import("resource://weave/Observers.js");
 Cu.import("resource://weave/Preferences.js");
 Cu.import("resource://weave/log4moz.js");
 Cu.import("resource://weave/constants.js");
 Cu.import("resource://weave/util.js");
 Cu.import("resource://weave/async.js");
-Cu.import("resource://weave/crypto.js");
 Cu.import("resource://weave/base_records/wbo.js");
 Cu.import("resource://weave/base_records/keys.js");
 
 Function.prototype.async = Async.sugar;
 
 Utils.lazy(this, 'CryptoMetas', RecordManager);
 
 // fixme: global, ugh
--- a/services/sync/modules/base_records/keys.js
+++ b/services/sync/modules/base_records/keys.js
@@ -43,17 +43,16 @@ const Cr = Components.results;
 const Cu = Components.utils;
 
 Cu.import("resource://weave/Observers.js");
 Cu.import("resource://weave/Preferences.js");
 Cu.import("resource://weave/log4moz.js");
 Cu.import("resource://weave/constants.js");
 Cu.import("resource://weave/util.js");
 Cu.import("resource://weave/async.js");
-Cu.import("resource://weave/crypto.js");
 Cu.import("resource://weave/base_records/wbo.js");
 
 Function.prototype.async = Async.sugar;
 
 Utils.lazy(this, 'PubKeys', PubKeyManager);
 Utils.lazy(this, 'PrivKeys', PrivKeyManager);
 
 function PubKey(uri, authenticator) {
--- a/services/sync/modules/base_records/wbo.js
+++ b/services/sync/modules/base_records/wbo.js
@@ -76,35 +76,45 @@ WBORecord.prototype = {
     this.data.parentid = value;
   },
 
   get modified() this.data.modified,
   set modified(value) {
     this.data.modified = value;
   },
 
-  get depth() this.data.depth,
+  get depth() {
+    if (this.data.depth)
+      return this.data.depth;
+    return 0;
+  },
   set depth(value) {
     this.data.depth = value;
   },
 
-  get sortindex() this.data.sortindex,
+  get sortindex() {
+    if (this.data.sortindex)
+      return this.data.sortindex;
+    return 0;
+  },
   set sortindex(value) {
     this.data.sortindex = value;
   },
 
   get payload() this.data.payload,
   set payload(value) {
     this.data.payload = value;
   },
 
   toString: function WBORec_toString() {
-    return "{id: " + this.id + ", depth: " + this.depth +
-      ", sortindex: " + this.sortindex + ",\nmodified: " + this.modified +
-      ", payload: " + json.encode(this.cleartext) + "}";
+    return "id: " + this.id + "\n" +
+      "parent: " + this.parentid + "\n" +
+      "depth: " + this.depth + ", index: " + this.sortindex + "\n" +
+      "modified: " + this.modified + "\n" +
+      "payload: " + json.encode(this.cleartext);
   }
 };
 
 // fixme: global, ugh
 let json = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
 
 function WBOFilter() {}
 WBOFilter.prototype = {
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -282,47 +282,22 @@ SyncEngine.prototype = {
   },
 
   get outgoing() {
     if (!this._outgoing)
       this._outgoing = [];
     return this._outgoing;
   },
 
-  // Create a new record starting from an ID
-  // Calls _store.wrapItem to get the actual item, but sometimes needs to be
-  // overridden anyway (to alter parentid or other properties outside the payload)
-  _createRecord: function SyncEngine__createRecord(id, encrypt) {
-    let self = yield;
-
-    let record = new CryptoWrapper();
+  // Create a new record by querying the store, and add the engine metadata
+  _createRecord: function SyncEngine__createRecord(id) {
+    let record = this._store.createRecord(id);
     record.uri = this.engineURL + id;
     record.encryption = this.cryptoMetaURL;
-    record.cleartext = this._store.wrapItem(id);
-
-    // XXX subclasses might not expect us to delete the payload fields
-    if (record.cleartext) {
-      if (record.cleartext.parentid) {
-        record.parentid = record.cleartext.parentid;
-        delete record.cleartext.parentid;
-      }
-      if (typeof(record.cleartext.depth) != "undefined") {
-        record.depth = record.cleartext.depth;
-        delete record.cleartext.depth;
-      }
-      if (typeof(record.cleartext.sortindex) != "undefined") {
-        record.sortindex = record.cleartext.sortindex;
-        delete record.cleartext.sortindex;
-      }
-    }
-
-    if (encrypt || encrypt == undefined)
-      yield record.encrypt(self.cb, ID.get('WeaveCryptoID').password);
-
-    self.done(record);
+    return record;
   },
 
   // Check if a record is "like" another one, even though the IDs are different,
   // in that case, we'll change the ID of the local item to match
   // Probably needs to be overridden in a subclass, to change which criteria
   // make two records "the same one"
   _recordLike: function SyncEngine__recordLike(a, b) {
     if (a.parentid != b.parentid)
@@ -384,26 +359,26 @@ SyncEngine.prototype = {
       }
     }
 
     // generate queue from changed items list
 
     // XXX should have a heuristic like this, but then we need to be able to
     // serialize each item by itself, something our stores can't currently do
     //if (this._tracker.changedIDs.length >= 30)
-    this._store.cacheItemsHint();
+    //this._store.cacheItemsHint();
 
     // NOTE we want changed items -> outgoing -> server to be as atomic as
     // possible, so we clear the changed IDs after we upload the changed records
     // NOTE2 don't encrypt, we'll do that before uploading instead
     for (let id in this._tracker.changedIDs) {
-      this.outgoing.push(yield this._createRecord.async(this, self.cb, id, false));
+      this.outgoing.push(this._createRecord(id));
     }
 
-    this._store.clearItemCacheHint();
+    //this._store.clearItemCacheHint();
   },
 
   // Generate outgoing records
   _processIncoming: function SyncEngine__processIncoming() {
     let self = yield;
 
     this._log.debug("Downloading & applying server changes");
 
@@ -520,33 +495,33 @@ SyncEngine.prototype = {
 
     if (this.outgoing.length) {
       this._log.debug("Uploading client changes (" + this.outgoing.length + ")");
 
       // collection we'll upload
       let up = new Collection(this.engineURL);
 
       // regen the store cache so we can get item depths
-      this._store.cacheItemsHint();
+      //this._store.cacheItemsHint();
       let depth = {};
 
       let out;
       while ((out = this.outgoing.pop())) {
         this._log.trace("Outgoing:\n" + out);
         yield out.encrypt(self.cb, ID.get('WeaveCryptoID').password);
         yield up.pushRecord(self.cb, out);
         this._store.wrapDepth(out.id, depth);
       }
 
       // now add short depth-only records
       this._log.trace(depth.length + "outgoing depth records");
       for (let id in depth) {
         up.pushDepthRecord({id: id, depth: depth[id]});
       }
-      this._store.clearItemCacheHint();
+      //this._store.clearItemCacheHint();
 
       // do the upload
       yield up.post(self.cb);
 
       // save last modified date
       let mod = up.data.modified;
       if (typeof(mod) == "string")
         mod = parseInt(mod);
@@ -592,122 +567,8 @@ SyncEngine.prototype = {
   },
 
   _resetServer: function SyncEngine__resetServer() {
     let self = yield;
     let all = new Resource(this.engineURL);
     yield all.delete(self.cb);
   }
 };
-
-function BlobEngine() {
-  // subclasses should call _init
-  // we don't call it here because it requires serverPrefix to be set
-}
-BlobEngine.prototype = {
-  __proto__: Engine.prototype,
-
-  get _profileID() {
-    return ClientData.GUID;
-  },
-
-  _init: function BlobEngine__init() {
-    // FIXME meep?
-    this.__proto__.__proto__.__proto__.__proto__._init.call(this);
-    this._keys = new Keychain(this.serverPrefix);
-    this._file = new Resource(this.serverPrefix + "data");
-    this._file.pushFilter(new JsonFilter());
-    this._file.pushFilter(new CryptoFilter(this.engineId));
-  },
-
-  _initialUpload: function BlobEngine__initialUpload() {
-    let self = yield;
-    this._log.info("Initial upload to server");
-    yield this._keys.initialize(self.cb, this.engineId);
-    this._file.data = {};
-    yield this._merge.async(this, self.cb);
-    yield this._file.put(self.cb);
-  },
-
-  // NOTE: Assumes this._file has latest server data
-  // this method is separate from _sync so it's easy to override in subclasses
-  _merge: function BlobEngine__merge() {
-    let self = yield;
-    this._file.data[this._profileID] = this._store.wrap();
-  },
-
-  // This engine is very simple:
-  // 1) Get the latest server data
-  // 2) Merge with our local data store
-  // 3) Upload new merged data
-  // NOTE: a version file will be needed in the future to optimize the case
-  //       where there are no changes
-  _sync: function BlobEngine__sync() {
-    let self = yield;
-
-    this._log.info("Beginning sync");
-    this._os.notifyObservers(null, "weave:service:sync:engine:start", this.name);
-
-    // FIXME
-    //if (!(yield DAV.MKCOL(this.serverPrefix, self.cb)))
-    //  throw "Could not create remote folder";
-
-    try {
-      if ("none" != Utils.prefs.getCharPref("encryption"))
-        yield this._keys.getKeyAndIV(self.cb, this.engineId);
-      yield this._file.get(self.cb);
-      yield this._merge.async(this, self.cb);
-      yield this._file.put(self.cb);
-
-    } catch (e if e.status == 404) {
-      yield this._initialUpload.async(this, self.cb);
-    }
-
-    this._log.info("Sync complete");
-    this._os.notifyObservers(null, "weave:service:sync:engine:success", this.name);
-    self.done(true);
-  }
-};
-
-function HeuristicEngine() {
-}
-HeuristicEngine.prototype = {
-  __proto__: new Engine(),
-
-  get _remote() {
-    let remote = new RemoteStore(this);
-    this.__defineGetter__("_remote", function() remote);
-    return remote;
-  },
-
-  get _snapshot() {
-    let snap = new SnapshotStore(this.name);
-    this.__defineGetter__("_snapshot", function() snap);
-    return snap;
-  },
-
-  _resetServer: function SyncEngine__resetServer() {
-    let self = yield;
-    yield this._remote.wipe(self.cb);
-  },
-
-  _resetClient: function SyncEngine__resetClient() {
-    let self = yield;
-    this._log.debug("Resetting client state");
-    this._snapshot.wipe();
-    this._store.wipe();
-    this._log.debug("Client reset completed successfully");
-  },
-
-  _initialUpload: function HeuristicEngine__initialUpload() {
-    let self = yield;
-    this._log.info("Initial upload to server");
-    this._snapshot.data = this._store.wrap();
-    this._snapshot.version = 0;
-    this._snapshot.GUID = null; // in case there are other snapshots out there
-    yield this._remote.initialize(self.cb, this._snapshot);
-    this._snapshot.save();
-  },
-
-  _sync: function HeuristicEngine__sync() {
-    let self = yield;
-  }
-};
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -61,16 +61,17 @@ Cu.import("resource://weave/util.js");
 Cu.import("resource://weave/crypto.js");
 Cu.import("resource://weave/async.js");
 Cu.import("resource://weave/engines.js");
 Cu.import("resource://weave/stores.js");
 Cu.import("resource://weave/trackers.js");
 Cu.import("resource://weave/identity.js");
 Cu.import("resource://weave/notifications.js");
 Cu.import("resource://weave/resource.js");
+Cu.import("resource://weave/type_records/bookmark.js");
 
 Function.prototype.async = Async.sugar;
 
 function BookmarksEngine() {
   this._init();
 }
 BookmarksEngine.prototype = {
   __proto__: SyncEngine.prototype,
@@ -123,27 +124,28 @@ BookmarksStore.prototype = {
   __ls: null,
   get _ls() {
     if (!this.__ls)
       this.__ls = Cc["@mozilla.org/browser/livemark-service;2"].
         getService(Ci.nsILivemarkService);
     return this.__ls;
   },
 
-  __ms: null,
   get _ms() {
-    if (!this.__ms)
-      try {
-        this.__ms = Cc["@mozilla.org/microsummary/service;1"].
-          getService(Ci.nsIMicrosummaryService);
-      } catch( e ) {
-	this.__ms = SERVICE_NOT_SUPPORTED;
-	this._log.warn("There is no Microsummary service.");
-      }
-    return this.__ms;
+    let ms;
+    try {
+      ms = Cc["@mozilla.org/microsummary/service;1"].
+        getService(Ci.nsIMicrosummaryService);
+    } catch (e) {
+      ms = null;
+      this._log.warn("Could not load microsummary service");
+      this._log.debug(e);
+    }
+    this.__defineGetter__("_ms", function() ms);
+    return ms;
   },
 
   __ts: null,
   get _ts() {
     if (!this.__ts)
       this.__ts = Cc["@mozilla.org/browser/tagging-service;1"].
                   getService(Ci.nsITaggingService);
     return this.__ts;
@@ -166,16 +168,38 @@ BookmarksStore.prototype = {
     case "unfiled":
       return this._bms.unfiledBookmarksFolder;
     default:
       return this._bms.getItemIdForGUID(GUID);
     }
     return null;
   },
 
+  _getWeaveIdForItem: function BStore__getWeaveIdForItem(placeId) {
+    if (placeId == this._bms.bookmarksMenuFolder)
+      return "menu";
+    if (placeId == this._bms.toolbarFolder)
+      return "toolbar";
+    if (placeId == this._bms.unfiledBookmarksFolder)
+      return "unfiled";
+    return this._bms.getItemGUID(placeId);
+  },
+
+  _isToplevel: function BStore__isToplevel(placeId) {
+    if (placeId == this._bms.bookmarksMenuFolder)
+      return true;
+    if (placeId == this._bms.toolbarFolder)
+      return true;
+    if (placeId == this._bms.unfiledBookmarksFolder)
+      return true;
+    if (this._bms.getFolderIdForItem(placeId) < 0)
+      return true;
+    return false;
+  },
+
   _itemExists: function BStore__itemExists(id) {
     return this._getItemIdForGUID(id) >= 0;
   },
 
   create: function BStore_create(record) {
     let newId;
     let parentId = this._getItemIdForGUID(record.parentid);
 
@@ -184,38 +208,38 @@ BookmarksStore.prototype = {
       parentId = this._bms.bookmarksMenuFolder;
     }
 
     switch (record.cleartext.type) {
     case "query":
     case "bookmark":
     case "microsummary": {
       this._log.debug(" -> creating bookmark \"" + record.cleartext.title + "\"");
-      let URI = Utils.makeURI(record.cleartext.URI);
-      newId = this._bms.insertBookmark(parentId, URI, record.sortindex,
+      let uri = Utils.makeURI(record.cleartext.uri);
+      newId = this._bms.insertBookmark(parentId, uri, record.sortindex,
                                        record.cleartext.title);
-      this._ts.untagURI(URI, null);
-      this._ts.tagURI(URI, record.cleartext.tags);
+      this._ts.untagURI(uri, null);
+      this._ts.tagURI(uri, record.cleartext.tags);
       this._bms.setKeywordForBookmark(newId, record.cleartext.keyword);
       if (record.cleartext.description) {
         this._ans.setItemAnnotation(newId, "bookmarkProperties/description",
                                     record.cleartext.description, 0,
                                    this._ans.EXPIRE_NEVER);
       }
 
       if (record.cleartext.type == "microsummary") {
         this._log.debug("   \-> is a microsummary");
         this._ans.setItemAnnotation(newId, "bookmarks/staticTitle",
                                     record.cleartext.staticTitle || "", 0, this._ans.EXPIRE_NEVER);
         let genURI = Utils.makeURI(record.cleartext.generatorURI);
 	if (this._ms == SERVICE_NOT_SUPPORTED) {
 	  this._log.warn("Can't create microsummary -- not supported.");
 	} else {
           try {
-            let micsum = this._ms.createMicrosummary(URI, genURI);
+            let micsum = this._ms.createMicrosummary(uri, genURI);
             this._ms.setMicrosummary(newId, micsum);
           }
           catch(ex) { /* ignore "missing local generator" exceptions */ }
 	}
       }
     } break;
     case "folder":
       this._log.debug(" -> creating folder \"" + record.cleartext.title + "\"");
@@ -343,18 +367,18 @@ BookmarksStore.prototype = {
       this._bms.moveItem(itemId, parentid, record.sortindex);
     }
 
     for (let key in record.cleartext) {
       switch (key) {
       case "title":
         this._bms.setItemTitle(itemId, record.cleartext.title);
         break;
-      case "URI":
-        this._bms.changeBookmarkURI(itemId, Utils.makeURI(record.cleartext.URI));
+      case "uri":
+        this._bms.changeBookmarkURI(itemId, Utils.makeURI(record.cleartext.uri));
         break;
       case "tags": {
         // filter out null/undefined/empty tags
         let tags = record.cleartext.tags.filter(function(t) t);
         let tagsURI = this._bms.getBookmarkURI(itemId);
         this._ts.untagURI(tagsURI, null);
         this._ts.tagURI(tagsURI, tags);
       } break;
@@ -418,162 +442,142 @@ BookmarksStore.prototype = {
       return;
     }
 
     this._log.debug("Changing GUID " + oldID + " to " + newID);
     this._bms.setItemGUID(itemId, newID);
     Engines.get("bookmarks")._tracker._all[itemId] = newID; // HACK - see tracker
   },
 
-  _getNode: function BSS__getNode(folder) {
+  _getNode: function BStore__getNode(folder) {
     let query = this._hsvc.getNewQuery();
     query.setFolders([folder], 1);
     return this._hsvc.executeQuery(query, this._hsvc.getNewQueryOptions()).root;
   },
 
-  __wrap: function BSS___wrap(node, items, parentid, index, depth, guidOverride) {
-    let GUID, item;
+  // XXX a little inefficient - isToplevel calls getFolderIdForItem too
+  _itemDepth: function BStore__itemDepth(id) {
+    if (this._isToplevel(id))
+      return 0;
+    return this._itemDepth(this._bms.getFolderIdForItem(id)) + 1;
+  },
+
+  _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 this._ts.getTagsForURI(uri, {});
+  },
 
-    // we override the guid for the root items, "menu", "toolbar", etc.
-    if (guidOverride) {
-      GUID = guidOverride;
-      item = {sortindex: index, depth: depth};
-    } else {
-      GUID = this._bms.getItemGUID(node.itemId);
-      item = {parentid: parentid, sortindex: index, depth: depth};
+  _getDescription: function BStore__getDescription(id) {
+    try {
+      return this._ans.getItemAnnotation(id, "bookmarkProperties/description");
+    } catch (e) {
+      return undefined;
+    }
+  },
+
+  _getStaticTitle: function BStore__getStaticTitle(id) {
+    try {
+      return this._ans.getItemAnnotation(id, "bookmarks/staticTitle");
+    } catch (e) {
+      return "";
+    }
+  },
+
+  // Create a record starting from the weave id (places guid)
+  // NOTE: the record id will not be set, because WBOs generate it from
+  //       the URL, which we don't have here.  The engine sets it.
+  createRecord: function BStore_createRecord(guid) {
+    let record;
+
+    let placeId = this._bms.getItemIdForGUID(guid);
+    if (placeId < 0) {
+      record = new PlacesItem();
+      record.cleartext = null; // deleted item
+      return record;
     }
 
-    if (node.type == node.RESULT_TYPE_FOLDER) {
-      if (this._ls.isLivemark(node.itemId)) {
-        item.type = "livemark";
-        let siteURI = this._ls.getSiteURI(node.itemId);
-        let feedURI = this._ls.getFeedURI(node.itemId);
-        item.siteURI = siteURI? siteURI.spec : "";
-        item.feedURI = feedURI? feedURI.spec : "";
-      } else if (this._ans.itemHasAnnotation(node.itemId, INCOMING_SHARED_ANNO)){
-	/* When there's an incoming share, we just sync the folder itself
-	 and the values of its annotations: NOT any of its contents.  So
-	 we'll wrap it as type=incoming-share, not as a "folder". */
-	item.type = "incoming-share";
-	item.title = node.title;
-        item.serverPathAnno = this._ans.getItemAnnotation(node.itemId,
-                                                      SERVER_PATH_ANNO);
-	item.incomingSharedAnno = this._ans.getItemAnnotation(node.itemId,
-                                                      INCOMING_SHARED_ANNO);
+    switch (this._bms.getItemType(placeId)) {
+    case this._bms.TYPE_BOOKMARK:
+      if (this._ms && this._ms.hasMicrosummary(placeId)) {
+        record = new BookmarkMicsum();
+        let micsum = this._ms.getMicrosummary(placeId);
+        record.generatorURI = micsum.generator.uri; // breaks local generators
+        record.staticTitle = this._getStaticTitle(placeId);
+
       } else {
-        item.type = "folder";
-        node.QueryInterface(Ci.nsINavHistoryQueryResultNode);
-        node.containerOpen = true;
-	// If folder is an outgoing share, wrap its annotations:
-	if (this._ans.itemHasAnnotation(node.itemId, OUTGOING_SHARED_ANNO)) {
-	  item.outgoingSharedAnno = this._ans.getItemAnnotation(node.itemId,
-                                                      OUTGOING_SHARED_ANNO);
-	}
-	if (this._ans.itemHasAnnotation(node.itemId, SERVER_PATH_ANNO)) {
-	  item.serverPathAnno = this._ans.getItemAnnotation(node.itemId,
-							    SERVER_PATH_ANNO);
-	}
-
-        for (var i = 0; i < node.childCount; i++) {
-          this.__wrap(node.getChild(i), items, GUID, i, depth + 1);
-        }
-      }
-      if (!guidOverride)
-        item.title = node.title; // no titles for root nodes
-
-    } else if (node.type == node.RESULT_TYPE_URI ||
-               node.type == node.RESULT_TYPE_QUERY) {
-      if (this._ms != SERVICE_NOT_SUPPORTED &&
-	  this._ms.hasMicrosummary(node.itemId)) {
-        item.type = "microsummary";
-        let micsum = this._ms.getMicrosummary(node.itemId);
-        item.generatorURI = micsum.generator.uri.spec; // breaks local generators
-        item.staticTitle = "";
-        try {
-          item.staticTitle = this._ans.getItemAnnotation(node.itemId,
-                                                         "bookmarks/staticTitle");
-        } catch (e) {}
-      } else if (node.type == node.RESULT_TYPE_QUERY) {
-        item.type = "query";
-        item.title = node.title;
-      } else {
-        item.type = "bookmark";
-        item.title = node.title;
+        record = new Bookmark();
+        record.title = this._bms.getItemTitle(placeId);
       }
 
-      try {
-        item.description =
-          this._ans.getItemAnnotation(node.itemId, "bookmarkProperties/description");
-      } catch (e) {
-        item.description = undefined;
-      }
+      record.bmkUri = this._bms.getBookmarkURI(placeId);
+      record.tags = this._getTags(record.bmkUri);
+      record.keyword = this._bms.getKeywordForBookmark(placeId);
+      record.description = this._getDescription(placeId);
+      break;
 
-      item.URI = node.uri;
+    case this._bms.TYPE_FOLDER:
+      if (this._ls.isLivemark(placeId)) {
+        record = new Livemark();
+        record.siteURI = this._ls.getSiteURI(placeId);
+        record.feedURI = this._ls.getFeedURI(placeId);
 
-      // This will throw if makeURI can't make an nsIURI object out of the
-      // node.uri string (or return null if node.uri is null), in which case
-      // we won't be able to get tags for the bookmark (but we'll still sync
-      // the rest of the record).
-      let uri;
-      try {
-        uri = Utils.makeURI(node.uri);
+      } else {
+        record = new BookmarkFolder();
+        record.title = this._bms.getItemTitle(placeId);
       }
-      catch(e) {
-        this._log.error("error parsing URI string <" + node.uri + "> " +
-                        "for item " + node.itemId + " (" + node.title + "): " +
-                        e);
-      }
+      break;
 
-      if (uri)
-        item.tags = this._ts.getTagsForURI(uri, {});
+    case this._bms.TYPE_SEPARATOR:
+      record = new BookmarkSeparator();
+      break;
 
-      item.keyword = this._bms.getKeywordForBookmark(node.itemId);
+    case this._bms.TYPE_DYNAMIC_CONTAINER:
+      record = new PlacesItem();
+      this._log.warn("Don't know how to serialize dynamic containers yet");
+      break;
 
-    } else if (node.type == node.RESULT_TYPE_SEPARATOR) {
-      item.type = "separator";
-
-    } else {
-      this._log.warn("Warning: unknown item type, cannot serialize: " + node.type);
-      return;
+    default:
+      record = new PlacesItem();
+      this._log.warn("Unknown item type, cannot serialize: " +
+                     this._bms.getItemType(placeId));
     }
 
-    items[GUID] = item;
-  },
-
-  // helper
-  _wrap: function BStore__wrap(node, items, rootName) {
-    return this.__wrap(node, items, null, 0, 0, rootName);
-  },
+    record.parentid = this._getWeaveIdForItem(this._bms.getFolderIdForItem(placeId));
+    record.depth = this._itemDepth(placeId);
+    record.sortindex = this._bms.getItemIndex(placeId);
 
-  wrap: function BStore_wrap() {
-    var items = {};
-    this._wrap(this._getNode(this._bms.bookmarksMenuFolder), items, "menu");
-    this._wrap(this._getNode(this._bms.toolbarFolder), items, "toolbar");
-    this._wrap(this._getNode(this._bms.unfiledBookmarksFolder), items, "unfiled");
-    this._lookup = items;
-    return items;
+    return record;
   },
 
-  // FIXME: the fast path here is not so bad, specially since the engine always
-  //        gives the cache hint atm.  but wrapping all items to return just one
-  //        (the slow path) is pretty bad
-  wrapItem: function BStore_wrapItem(id) {
-    if (this._itemCache)
-      return this._itemCache[id];
-    let all = this.wrap();
-    return all[id];
-  },
+  getAllIDs: function BStore_getAllIDs(node, items) {
+    if (!node) {
+      let items = {};
+      this.getAllIDs(this._getNode(this._bms.bookmarksMenuFolder), items);
+      this.getAllIDs(this._getNode(this._bms.toolbarFolder), items);
+      this.getAllIDs(this._getNode(this._bms.unfiledBookmarksFolder), items);
+      return items;
+    }
 
-  // XXX need a better way to query Places for all GUIDs
-  getAllIDs: function BStore_getAllIDs() {
-    let all = this.wrap();
-    delete all["unfiled"];
-    delete all["toolbar"];
-    delete all["menu"];
-    return all;
+    if (node.type == node.RESULT_TYPE_FOLDER &&
+        !this._ls.isLivemark(node.itemId)) {
+      node.QueryInterface(Ci.nsINavHistoryQueryResultNode);
+      node.containerOpen = true;
+      for (var i = 0; i < node.childCount; i++) {
+        let child = node.getChild(i);
+        items[this._bms.getItemGUID(child.itemId)] = {placesId: child.itemId};
+        this.getAllIDs(child, items);
+      }
+    }
+
+    return items;
   },
 
   wipe: function BStore_wipe() {
     this._bms.removeFolderChildren(this._bms.bookmarksMenuFolder);
     this._bms.removeFolderChildren(this._bms.toolbarFolder);
     this._bms.removeFolderChildren(this._bms.unfiledBookmarksFolder);
   },
 
@@ -625,17 +629,17 @@ BookmarksTracker.prototype = {
     // NOTE: since the callbacks give us item IDs (not GUIDs), we use
     // getItemGUID to get it within the callback.  For removals, however,
     // that doesn't work because the item is already gone! (and worse, Places
     // has a bug where it will generate a new one instead of throwing).
     // Our solution: cache item IDs -> GUIDs
 
     // FIXME: very roundabout way of getting id -> guid mapping!
     let store = new BookmarksStore();
-    let all = store.wrap();
+    let all = store.getAllIDs();
     this._all = {};
     for (let guid in all) {
       this._all[this._bms.getItemIdForGUID(guid)] = guid;
     }
 
     this._bms.addObserver(this, false);
   },
 
new file mode 100644
--- /dev/null
+++ b/services/sync/modules/type_records/bookmark.js
@@ -0,0 +1,199 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Weave.
+ *
+ * The Initial Developer of the Original Code is Mozilla.
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Dan Mills <thunder@mozilla.com>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+const EXPORTED_SYMBOLS = ['PlacesItem', 'Bookmark', 'BookmarkFolder', 'BookmarkMicsum',
+                          'Livemark', 'BookmarkSeparator'];
+
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+const Cr = Components.results;
+const Cu = Components.utils;
+
+Cu.import("resource://weave/log4moz.js");
+Cu.import("resource://weave/util.js");
+Cu.import("resource://weave/async.js");
+Cu.import("resource://weave/base_records/wbo.js");
+Cu.import("resource://weave/base_records/crypto.js");
+Cu.import("resource://weave/base_records/keys.js");
+
+Function.prototype.async = Async.sugar;
+
+function PlacesItem(uri, authenticator) {
+  this._PlacesItem_init(uri, authenticator);
+}
+PlacesItem.prototype = {
+  __proto__: CryptoWrapper.prototype,
+  _logName: "Record.PlacesItem",
+
+  _PlacesItem_init: function BmkItemRec_init(uri, authenticator) {
+    this._CryptoWrap_init(uri, authenticator);
+    this.cleartext = {
+    };
+  },
+
+  get type() this.cleartext.type,
+  set type(value) {
+    // XXX check type is valid?
+    this.cleartext.type = value;
+  }
+};
+
+function Bookmark(uri, authenticator) {
+  this._Bookmark_init(uri, authenticator);
+}
+Bookmark.prototype = {
+  __proto__: PlacesItem.prototype,
+  _logName: "Record.Bookmark",
+
+  _Bookmark_init: function BmkRec_init(uri, authenticator) {
+    this._PlacesItem_init(uri, authenticator);
+    this.cleartext.type = "bookmark";
+  },
+
+  get title() this.cleartext.title,
+  set title(value) {
+    this.cleartext.title = value;
+  },
+
+  get bmkUri() this.cleartext.uri,
+  set bmkUri(value) {
+    if (typeof(value) == "string")
+      this.cleartext.uri = value;
+    else
+      this.cleartext.uri = value.spec;
+  },
+
+  get description() this.cleartext.description,
+  set description(value) {
+    this.cleartext.description = value;
+  },
+
+  get tags() this.cleartext.tags,
+  set tags(value) {
+    this.cleartext.tags = value;
+  },
+
+  get keyword() this.cleartext.keyword,
+  set keyword(value) {
+    this.cleartext.keyword = value;
+  }
+};
+
+function BookmarkMicsum(uri, authenticator) {
+  this._BookmarkMicsum_init(uri, authenticator);
+}
+BookmarkMicsum.prototype = {
+  __proto__: Bookmark.prototype,
+  _logName: "Record.BookmarkMicsum",
+
+  _BookmarkMicsum_init: function BmkMicsumRec_init(uri, authenticator) {
+    this._Bookmark_init(uri, authenticator);
+    this.cleartext.type = "microsummary";
+  },
+
+  get generatorURI() this.cleartext.generatorURI,
+  set generatorURI(value) {
+    if (typeof(value) == "string")
+      this.cleartext.generatorURI = value;
+    else
+      this.cleartext.generatorURI = value? value.spec : "";
+  },
+
+  get staticTitle() this.cleartext.staticTitle,
+  set staticTitle(value) {
+    this.cleartext.staticTitle = value;
+  }
+};
+
+function BookmarkFolder(uri, authenticator) {
+  this._BookmarkFolder_init(uri, authenticator);
+}
+BookmarkFolder.prototype = {
+  __proto__: PlacesItem.prototype,
+  _logName: "Record.Folder",
+
+  _BookmarkFolder_init: function FolderRec_init(uri, authenticator) {
+    this._PlacesItem_init(uri, authenticator);
+    this.cleartext.type = "folder";
+  },
+
+  get title() this.cleartext.title,
+  set title(value) {
+    this.cleartext.title = value;
+  }
+};
+
+function Livemark(uri, authenticator) {
+  this._Livemark_init(uri, authenticator);
+}
+Livemark.prototype = {
+  __proto__: BookmarkFolder.prototype,
+  _logName: "Record.Livemark",
+
+  _Livemark_init: function LvmkRec_init(uri, authenticator) {
+    this._BookmarkFolder_init(uri, authenticator);
+    this.cleartext.type = "livemark";
+  },
+
+  get siteURI() this.cleartext.siteURI,
+  set siteURI(value) {
+    if (typeof(value) == "string")
+      this.cleartext.siteURI = value;
+    else
+      this.cleartext.siteURI = value? value.spec : "";
+  },
+
+  get feedURI() this.cleartext.feedURI,
+  set feedURI(value) {
+    if (typeof(value) == "string")
+      this.cleartext.feedURI = value;
+    else
+      this.cleartext.feedURI = value? value.spec : "";
+  }
+};
+
+function BookmarkSeparator(uri, authenticator) {
+  this._BookmarkSeparator_init(uri, authenticator);
+}
+BookmarkSeparator.prototype = {
+  __proto__: PlacesItem.prototype,
+  _logName: "Record.Separator",
+
+  _BookmarkSeparator_init: function SepRec_init(uri, authenticator) {
+    this._PlacesItem_init(uri, authenticator);
+    this.cleartext.type = "separator";
+  }
+};