Bug 1087255 - Convert Sync JS clients of RemovePage(s) to History.remove;r=rnewman
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Fri, 31 Oct 2014 12:36:19 +0100
changeset 500822 1a3e39513a18f97f4362d3c4bc5abbe07f16bb87
parent 500821 27e1f17f5275d7411dffe9488e5c3b7a05bfd263
child 500823 4975b3ae236b2f5842d2b24b17e6911f025123c2
push id49816
push userbmo:tchiovoloni@mozilla.com
push dateFri, 17 Mar 2017 20:44:02 +0000
reviewersrnewman
bugs1087255
milestone55.0a1
Bug 1087255 - Convert Sync JS clients of RemovePage(s) to History.remove;r=rnewman
services/sync/modules/engines/history.js
services/sync/tps/extensions/tps/resource/modules/history.jsm
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -250,29 +250,31 @@ HistoryStore.prototype = {
     return urls.reduce(function(ids, item) {
       ids[self.GUIDForUri(item.url, true)] = item.url;
       return ids;
     }, {});
   },
 
   applyIncomingBatch: function applyIncomingBatch(records) {
     let failed = [];
+    let blockers = [];
 
     // Convert incoming records to mozIPlaceInfo objects. Some records can be
     // ignored or handled directly, so we're rewriting the array in-place.
     let i, k;
     for (i = 0, k = 0; i < records.length; i++) {
       let record = records[k] = records[i];
       let shouldApply;
 
-      // This is still synchronous I/O for now.
       try {
         if (record.deleted) {
-          // Consider using nsIBrowserHistory::removePages() here.
-          this.remove(record);
+          let promise = this.remove(record);
+          promise = promise.then(null, ex => failed.push(record.id));
+          blockers.push(promise);
+
           // No further processing needed. Remove it from the list.
           shouldApply = false;
         } else {
           shouldApply = this._recordToPlaceInfo(record);
         }
       } catch (ex) {
         if (Async.isShutdownException(ex)) {
           throw ex;
@@ -282,31 +284,45 @@ HistoryStore.prototype = {
       }
 
       if (shouldApply) {
         k += 1;
       }
     }
     records.length = k; // truncate array
 
-    // Nothing to do.
-    if (!records.length) {
-      return failed;
+    let handleAsyncOperationsComplete = Async.makeSyncCallback();
+
+    if (records.length) {
+      blockers.push(new Promise(resolve => {
+        let updatePlacesCallback = {
+          handleResult: function handleResult() {},
+          handleError: function handleError(resultCode, placeInfo) {
+            failed.push(placeInfo.guid);
+          },
+          handleCompletion: resolve,
+        };
+        this._asyncHistory.updatePlaces(records, updatePlacesCallback);
+      }));
     }
 
-    let updatePlacesCallback = {
-      handleResult: function handleResult() {},
-      handleError: function handleError(resultCode, placeInfo) {
-        failed.push(placeInfo.guid);
-      },
-      handleCompletion: Async.makeSyncCallback()
-    };
-    this._asyncHistory.updatePlaces(records, updatePlacesCallback);
-    Async.waitForSyncCallback(updatePlacesCallback.handleCompletion);
-    return failed;
+    // Since `failed` is updated asynchronously and this function is
+    // synchronous, we need to spin-wait until we are sure that all
+    // updates to `fail` have completed.
+    Promise.all(blockers).then(
+      handleAsyncOperationsComplete,
+      ex => {
+        // In case of error, terminate wait, but make sure that the
+        // error is reported nevertheless and still causes test
+        // failures.
+        handleAsyncOperationsComplete();
+        throw ex;
+      });
+    Async.waitForSyncCallback(handleAsyncOperationsComplete);
+      return failed;
   },
 
   /**
    * Converts a Sync history record to a mozIPlaceInfo.
    *
    * Throws if an invalid record is encountered (invalid URI, etc.),
    * returns true if the record is to be applied, false otherwise
    * (no visits to add, etc.),
@@ -383,25 +399,25 @@ HistoryStore.prototype = {
                       + record.uri.spec + ": no visits to add.");
       return false;
     }
 
     return true;
   },
 
   remove: function HistStore_remove(record) {
-    let page = this._findURLByGUID(record.id);
-    if (page == null) {
-      this._log.debug("Page already removed: " + record.id);
-      return;
-    }
-
-    let uri = Utils.makeURI(page.url);
-    PlacesUtils.history.removePage(uri);
-    this._log.trace("Removed page: " + [record.id, page.url, page.title]);
+    return PlacesUtils.history.remove(record.id).then(
+      (removed) => {
+        if (removed) {
+          this._log.trace("Removed page: " + record.id);
+        } else {
+          this._log.debug("Page already removed: " + record.id);
+        }
+    });
+    this._log.trace("Removing page: " + record.id);
   },
 
   itemExists: function HistStore_itemExists(id) {
     return !!this._findURLByGUID(id);
   },
 
   createRecord: function createRecord(id, collection) {
     let foo = this._findURLByGUID(id);
--- a/services/sync/tps/extensions/tps/resource/modules/history.jsm
+++ b/services/sync/tps/extensions/tps/resource/modules/history.jsm
@@ -170,18 +170,17 @@ var HistoryEntry = {
    *
    * @param item An object representing items to delete
    * @param usSinceEpoch The number of microseconds from Epoch to
    *        the time the current Crossweave run was started
    * @return nothing
    */
   Delete(item, usSinceEpoch) {
     if ("uri" in item) {
-      let uri = Services.io.newURI(item.uri);
-      PlacesUtils.history.removePage(uri);
+      PlacesUtils.history.remove(item.uri);
     } else if ("host" in item) {
       PlacesUtils.history.removePagesFromHost(item.host, false);
     } else if ("begin" in item && "end" in item) {
       let cb = Async.makeSpinningCallback();
       let msSinceEpoch = parseInt(usSinceEpoch / 1000);
       let filter = {
         beginDate: new Date(msSinceEpoch + (item.begin * 60 * 60 * 1000)),
         endDate: new Date(msSinceEpoch + (item.end * 60 * 60 * 1000))