Bug 1323333 - Separate `pullNewChanges` and `markChangesAsSyncing`. draft
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 27 Apr 2017 15:13:37 -0700
changeset 569794 36703e4082d5430e47ca84f0348f157abca7a7db
parent 554498 31810a9548fcede48be099fc9823fd2710616d64
child 569795 ec2806ff1649b345e7999a42876592e5dd36ff63
push id56282
push userbmo:kit@mozilla.com
push dateThu, 27 Apr 2017 22:47:44 +0000
bugs1323333
milestone55.0a1
Bug 1323333 - Separate `pullNewChanges` and `markChangesAsSyncing`. MozReview-Commit-ID: IskzY7Hh9AM
services/sync/modules/engines/bookmarks.js
toolkit/components/places/PlacesSyncUtils.jsm
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -470,19 +470,19 @@ BookmarksEngine.prototype = {
       delete this._guidMap;
       return this._guidMap = guidMap;
     });
 
     this._store._childrenToOrder = {};
     this._store.clearPendingDeletions();
   },
 
-  _deletePending() {
+  async _deletePending() {
     // Delete pending items -- See the comment above BookmarkStore's deletePending
-    let newlyModified = Async.promiseSpinningly(this._store.deletePending());
+    let newlyModified = await this._store.deletePending();
     if (newlyModified) {
       this._log.debug("Deleted pending items", newlyModified);
       this._modified.insert(newlyModified);
     }
   },
 
   _shouldReviveRemotelyDeletedRecord(item) {
     let modifiedTimestamp = this._modified.getModifiedTimestamp(item.id);
@@ -501,27 +501,33 @@ BookmarksEngine.prototype = {
     if (newChanges) {
       this._modified.insert(newChanges);
       return true;
     }
     return false;
   },
 
   _processIncoming(newitems) {
-    try {
-      SyncEngine.prototype._processIncoming.call(this, newitems);
-    } finally {
-      try {
-        this._deletePending();
-      } finally {
-        // Reorder children.
-        this._store._orderChildren();
-        delete this._store._childrenToOrder;
-      }
-    }
+    SyncEngine.prototype._processIncoming.call(this, newitems);
+    Async.promiseSpinningly(this._postProcessIncoming());
+  },
+
+  // Applies pending tombstones, sets folder child order, and updates the sync
+  // status of all `NEW` bookmarks to `NORMAL`.
+  async _postProcessIncoming() {
+    await this._deletePending();
+    await this._orderChildren();
+
+    let changes = this._modified.changes;
+    await PlacesSyncUtils.bookmarks.markChangesAsSyncing(changes);
+  },
+
+  async _orderChildren() {
+    await this._store._orderChildren();
+    this._store._childrenToOrder = {};
   },
 
   _syncFinish: function _syncFinish() {
     SyncEngine.prototype._syncFinish.call(this);
     this._tracker._ensureMobileQuery();
   },
 
   _syncCleanup: function _syncCleanup() {
@@ -729,24 +735,25 @@ BookmarksStore.prototype = {
       this._log.debug(`Updated ${item.kind} ${item.syncId} under ${
         item.parentSyncId}`, item);
       if (item.dateAdded != record.dateAdded) {
         this.engine._needWeakReupload.add(item.syncId);
       }
     }
   },
 
-  _orderChildren: function _orderChildren() {
-    let promises = Object.keys(this._childrenToOrder).map(syncID => {
+  async _orderChildren() {
+    for (let syncID in this._childrenToOrder) {
       let children = this._childrenToOrder[syncID];
-      return PlacesSyncUtils.bookmarks.order(syncID, children).catch(ex => {
+      try {
+        await PlacesSyncUtils.bookmarks.order(syncID, children);
+      } catch (ex) {
         this._log.debug(`Could not order children for ${syncID}`, ex);
-      });
-    });
-    Async.promiseSpinningly(Promise.all(promises));
+      }
+    }
   },
 
   // There's some complexity here around pending deletions. Our goals:
   //
   // - Don't delete any bookmarks a user has created but not explicitly deleted
   //   (This includes any bookmark that was not a child of the folder at the
   //   time the deletion was recorded, and also bookmarks restored from a backup).
   // - Don't undelete any bookmark without ensuring the server structure
@@ -1195,17 +1202,19 @@ class BookmarksChangeset extends Changes
     super.changeID(oldID, newID);
     this.weakChanges[newID] = this.weakChanges[oldID];
     delete this.weakChanges[oldID];
   }
 
   ids() {
     let results = new Set();
     for (let id in this.changes) {
-      results.add(id);
+      if (!this.changes[id].synced) {
+        results.add(id);
+      }
     }
     for (let id in this.weakChanges) {
       results.add(id);
     }
     return [...results];
   }
 
   clear() {
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -331,19 +331,27 @@ const BookmarkSyncUtils = PlacesSyncUtil
    * can recover correctly after an interrupted sync.
    *
    * @return {Promise} resolved once all items have been fetched.
    * @resolves to an object containing records for changed bookmarks, keyed by
    *           the sync ID.
    * @see pullSyncChanges for the implementation, and markChangesAsSyncing for
    *      an explanation of why we update the sync status.
    */
-  pullChanges() {
-    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: pullChanges",
-      db => pullSyncChanges(db));
+  async pullChanges() {
+    let db = await PlacesUtils.promiseDBConnection();
+    return pullSyncChanges(db);
+  },
+
+  /**
+   * ...
+   */
+  markChangesAsSyncing(changeRecords) {
+    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: markChangesAsSyncing",
+      db => markChangesAsSyncing(db, changeRecords));
   },
 
   /**
    * Decrements the sync change counter, updates the sync status, and cleans up
    * tombstones for successfully synced items. Sync calls this method at the
    * end of each bookmark sync.
    *
    * @param changeRecords
@@ -468,18 +476,18 @@ const BookmarkSyncUtils = PlacesSyncUtil
       if (wasRemoved) {
         BookmarkSyncLog.trace(`remove: Removed folder ${bookmarkItem.guid}`);
       }
     }
 
     // TODO (Bug 1313890): Refactor the bookmarks engine to pull change records
     // before uploading, instead of returning records to merge into the engine's
     // initial changeset.
-    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: remove",
-      db => pullSyncChanges(db));
+    let db = yield PlacesUtils.promiseDBConnection();
+    return pullSyncChanges(db);
   }),
 
   /**
    * Increments the change counter of a non-folder item and its parent. Sync
    * calls this method to override a remote deletion for an item that's changed
    * locally.
    *
    * @param syncId
@@ -1738,18 +1746,16 @@ var pullSyncChanges = Task.async(functio
     WHERE syncChangeCounter >= 1
     UNION ALL
     SELECT guid, dateRemoved AS modified, 1 AS syncChangeCounter,
            :deletedSyncStatus, 1 AS tombstone
     FROM moz_bookmarks_deleted`,
     { deletedSyncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL },
     row => addRowToChangeRecords(row, changeRecords));
 
-  yield markChangesAsSyncing(db, changeRecords);
-
   return changeRecords;
 });
 
 var touchSyncBookmark = Task.async(function* (db, bookmarkItem) {
   if (BookmarkSyncLog.level <= Log.Level.Trace) {
     BookmarkSyncLog.trace(
       `touch: Reviving item "${bookmarkItem.guid}" and marking parent ` +
       BookmarkSyncUtils.guidToSyncId(bookmarkItem.parentGuid) + ` as modified`);