Merge fx-sync

services/sync/modules/engines.js

@@ -49,7 +49,6 @@ const Cu = Components.utils;
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/ext/Observers.js");
-Cu.import("resource://services-sync/ext/Sync.js");
 Cu.import("resource://services-sync/identity.js");
 Cu.import("resource://services-sync/log4moz.js");
 Cu.import("resource://services-sync/resource.js");
@@ -484,6 +483,10 @@ Engine.prototype = {
 function SyncEngine(name) {
   Engine.call(this, name || "SyncEngine");
   this.loadToFetch();
+
+  Utils.lazy2(this, "_timer", function() {
+    return Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+  });
 }
 SyncEngine.prototype = {
   __proto__: Engine.prototype,
@@ -575,6 +578,13 @@ SyncEngine.prototype = {
     return record;
   },
 
+  _sleep: function _sleep(delay) {
+    let cb = Utils.makeSyncCallback();
+    this._timer.initWithCallback({notify: cb}, delay,
+                                 Ci.nsITimer.TYPE_ONE_SHOT);
+    Utils.waitForSyncCallback(cb);
+  },
+
   // Any setup that needs to happen at the beginning of each sync.
   _syncStartup: function SyncEngine__syncStartup() {
 
@@ -741,7 +751,7 @@ SyncEngine.prototype = {
       if (applyBatch.length == this.applyIncomingBatchSize) {
         doApplyBatch.call(this);
       }
-      Sync.sleep(0);
+      this._sleep(0);
     });
 
     // Only bother getting data from the server if there's new things
@@ -974,7 +984,7 @@ SyncEngine.prototype = {
         if ((++count % MAX_UPLOAD_RECORDS) == 0)
           doUpload((count - MAX_UPLOAD_RECORDS) + " - " + count + " out");
 
-        Sync.sleep(0);
+        this._sleep(0);
       }
 
       // Final upload

services/sync/modules/engines/bookmarks.js

@@ -635,9 +635,43 @@ BookmarksStore.prototype = {
     return false;
   },
 
+  // Turn a record's nsINavBookmarksService constant and other attributes into
+  // a granular type for comparison.
+  _recordType: function _recordType(itemId) {
+    let bms  = this._bms;
+    let type = bms.getItemType(itemId);
+
+    switch (type) {
+      case bms.TYPE_FOLDER:
+        if (this._ls.isLivemark(itemId))
+          return "livemark";
+        return "folder";
+
+      case bms.TYPE_BOOKMARK:
+        if (this._ms && this._ms.hasMicrosummary(itemId))
+          return "microsummary";
+        let bmkUri = bms.getBookmarkURI(itemId).spec;
+        if (bmkUri.search(/^place:/) == 0)
+          return "query";
+        return "bookmark";
+
+      case bms.TYPE_SEPARATOR:
+        return "separator";
+
+      default:
+        return null;
+    }
+  },
+
   create: function BStore_create(record) {
-    // Default to unfiled if we don't have the parent yet
-    if (!record._parent) {
+    // Default to unfiled if we don't have the parent yet.
+
+    // Valid parent IDs are all positive integers. Other values -- undefined,
+    // null, -1 -- all compare false for > 0, so this catches them all. We
+    // don't just use <= without the !, because undefined and null compare
+    // false for that, too!
+    if (!(record._parent > 0)) {
+      this._log.debug("Parent is " + record._parent + "; reparenting to unfiled.");
       record._parent = kSpecialIds.unfiled;
     }
 
@@ -695,14 +729,27 @@ BookmarksStore.prototype = {
       break;
     case "livemark":
       let siteURI = null;
+      if (!record.feedUri) {
+        this._log.debug("No feed URI: skipping livemark record " + record.id);
+        return;
+      }
+      if (this._ls.isLivemark(record._parent)) {
+        this._log.debug("Invalid parent: skipping livemark record " + record.id);
+        return;
+      }
+
       if (record.siteUri != null)
         siteURI = Utils.makeURI(record.siteUri);
 
-      newId = this._ls.createLivemark(record._parent, record.title, siteURI,
-                                      Utils.makeURI(record.feedUri),
-                                      Svc.Bookmark.DEFAULT_INDEX);
-      this._log.debug(["created livemark", newId, "under", record._parent, "as",
-                       record.title, record.siteUri, record.feedUri].join(" "));
+      // Use createLivemarkFolderOnly, not createLivemark, to avoid it
+      // automatically updating during a sync.
+      newId = this._ls.createLivemarkFolderOnly(record._parent, record.title,
+                                                siteURI,
+                                                Utils.makeURI(record.feedUri),
+                                                Svc.Bookmark.DEFAULT_INDEX);
+      this._log.debug("Created livemark " + newId + " under " + record._parent +
+                      " as " + record.title + ", " + record.siteUri + ", " + 
+                      record.feedUri + ", GUID " + record.id);
       break;
     case "separator":
       newId = this._bms.insertSeparator(record._parent,
@@ -722,26 +769,23 @@ BookmarksStore.prototype = {
     this._setGUID(newId, record.id);
   },
 
-  remove: function BStore_remove(record) {
-    let itemId = this.idForGUID(record.id);
-    if (itemId <= 0) {
-      this._log.debug("Item " + record.id + " already removed");
-      return;
-    }
-    var type = this._bms.getItemType(itemId);
+  // Factored out of `remove` to avoid redundant DB queries when the Places ID
+  // is already known.
+  removeById: function removeById(itemId, guid) {
+    let type = this._bms.getItemType(itemId);
 
     switch (type) {
     case this._bms.TYPE_BOOKMARK:
-      this._log.debug("  -> removing bookmark " + record.id);
+      this._log.debug("  -> removing bookmark " + guid);
       this._ts.untagURI(this._bms.getBookmarkURI(itemId), null);
       this._bms.removeItem(itemId);
       break;
     case this._bms.TYPE_FOLDER:
-      this._log.debug("  -> removing folder " + record.id);
+      this._log.debug("  -> removing folder " + guid);
       Svc.Bookmark.removeItem(itemId);
       break;
     case this._bms.TYPE_SEPARATOR:
-      this._log.debug("  -> removing separator " + record.id);
+      this._log.debug("  -> removing separator " + guid);
       this._bms.removeItem(itemId);
       break;
     default:
@@ -750,6 +794,15 @@ BookmarksStore.prototype = {
     }
   },
 
+  remove: function BStore_remove(record) {
+    let itemId = this.idForGUID(record.id);
+    if (itemId <= 0) {
+      this._log.debug("Item " + record.id + " already removed");
+      return;
+    }
+    this.removeById(itemId, record.id);
+  },
+
   update: function BStore_update(record) {
     let itemId = this.idForGUID(record.id);
 
@@ -758,6 +811,25 @@ BookmarksStore.prototype = {
       return;
     }
 
+    // Two items are the same type if they have the same ItemType in Places,
+    // and also share some key characteristics (e.g., both being livemarks).
+    // We figure this out by examining the item to find the equivalent granular
+    // (string) type.
+    // If they're not the same type, we can't just update attributes. Delete
+    // then recreate the record instead.
+    let localItemType    = this._recordType(itemId);
+    let remoteRecordType = record.type;
+    this._log.trace("Local type: " + localItemType + ". " +
+                    "Remote type: " + remoteRecordType + ".");
+
+    if (localItemType != remoteRecordType) {
+      this._log.debug("Local record and remote record differ in type. " +
+                      "Deleting and recreating.");
+      this.removeById(itemId, record.id);
+      this.create(record);
+      return;
+    }
+
     this._log.trace("Updating " + record.id + " (" + itemId + ")");
 
     // Move the bookmark to a new parent or new position if necessary
@@ -1325,7 +1397,7 @@ BookmarksStore.prototype = {
       node.containerOpen = true;
 
       // Remember all the children GUIDs and recursively get more
-      for (var i = 0; i < node.childCount; i++) {
+      for (let i = 0; i < node.childCount; i++) {
         let child = node.getChild(i);
         items[this.GUIDForId(child.itemId)] = true;
         this._getChildren(child, items);

services/sync/modules/engines/history.js

@@ -54,7 +54,6 @@ Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/log4moz.js");
-Cu.import("resource://services-sync/ext/Sync.js");
 
 function HistoryRec(collection, id) {
   CryptoWrapper.call(this, collection, id);
@@ -459,8 +458,7 @@ HistoryStore.prototype = {
       return failed;
     }
 
-    let [updatePlaces, cb] = Sync.withCb(this._asyncHistory.updatePlaces,
-                                         this._asyncHistory);
+    let cb = Utils.makeSyncCallback();
     let onPlace = function onPlace(result, placeInfo) {
       if (!Components.isSuccessCode(result)) {
         failed.push(placeInfo.guid);
@@ -471,7 +469,8 @@ HistoryStore.prototype = {
       cb();
     };
     Svc.Obs.add(TOPIC_UPDATEPLACES_COMPLETE, onComplete);
-    updatePlaces(placeInfos, onPlace);
+    this._asyncHistory.updatePlaces(placeInfos, onPlace);
+    Utils.waitForSyncCallback(cb);
     return failed;
   },