Bug 1392533 - Make the places tree view directly communicate batch notifications to the results. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 27 Sep 2017 15:26:35 +0100
changeset 672578 fe70e85422b3c8e2e328ffcd2694c5ccb79e2708
parent 672296 e6c32278f32cd5f7d159627b2157396b62d0c4a9
child 733832 3f5a9d747cc585dc1b7db6d8aa464e8605c90f89
push id82278
push userbmo:standard8@mozilla.com
push dateFri, 29 Sep 2017 10:51:28 +0000
reviewersmak
bugs1392533
milestone58.0a1
Bug 1392533 - Make the places tree view directly communicate batch notifications to the results. r?mak MozReview-Commit-ID: HpN0v0jSwdK
browser/components/places/PlacesUIUtils.jsm
browser/components/places/content/controller.js
browser/components/places/content/treeView.js
toolkit/components/places/tests/unit/test_async_batchUpdatesForNode.js
toolkit/components/places/tests/unit/xpcshell.ini
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -35,16 +35,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "Weave",
                                   "resource://services-sync/main.js");
 
 const gInContentProcess = Services.appinfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT;
 const FAVICON_REQUEST_TIMEOUT = 60 * 1000;
 // Map from windows to arrays of data about pending favicon loads.
 let gFaviconLoadDataMap = new Map();
 
+const ITEM_CHANGED_BATCH_NOTIFICATION_THRESHOLD = 10;
+
 // copied from utilityOverlay.js
 const TAB_DROP_TYPE = "application/x-moz-tabbrowser-tab";
 const PREF_LOAD_BOOKMARKS_IN_BACKGROUND = "browser.tabs.loadBookmarksInBackground";
 const PREF_LOAD_BOOKMARKS_IN_TABS = "browser.tabs.loadBookmarksInTabs";
 
 // This function isn't public both because it's synchronous and because it is
 // going to be removed in bug 1072833.
 function IsLivemark(aItemId) {
@@ -1534,17 +1536,51 @@ this.PlacesUIUtils = {
    *
    * @see promiseNodeLikeFromFetchInfo above and Bookmarks.fetch in Bookmarks.jsm.
    */
   async fetchNodeLike(aGuidOrInfo) {
     let info = await PlacesUtils.bookmarks.fetch(aGuidOrInfo);
     if (!info)
       return null;
     return this.promiseNodeLikeFromFetchInfo(info);
-  }
+  },
+
+  /**
+   * This function wraps potentially large places transaction operations
+   * with batch notifications to the result node, hence switching the views
+   * to batch mode.
+   *
+   * @param {nsINavHistoryResult} resultNode The result node to turn on batching.
+   * @note If resultNode is not supplied, the function will pass-through to
+   *       functionToWrap.
+   * @param {Integer} itemsBeingChanged The count of items being changed. If the
+   *                                    count is lower than a threshold, then
+   *                                    batching won't be set.
+   * @param {Function} functionToWrap The function to
+   */
+  async batchUpdatesForNode(resultNode, itemsBeingChanged, functionToWrap) {
+    if (!resultNode) {
+      await functionToWrap();
+      return;
+    }
+
+    resultNode = resultNode.QueryInterface(Ci.nsINavBookmarkObserver);
+
+    if (itemsBeingChanged > ITEM_CHANGED_BATCH_NOTIFICATION_THRESHOLD) {
+      resultNode.onBeginUpdateBatch();
+    }
+
+    try {
+      await functionToWrap();
+    } finally {
+      if (itemsBeingChanged > ITEM_CHANGED_BATCH_NOTIFICATION_THRESHOLD) {
+        resultNode.onEndUpdateBatch();
+      }
+    }
+  },
 };
 
 
 PlacesUIUtils.PLACES_FLAVORS = [PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER,
                                 PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
                                 PlacesUtils.TYPE_X_MOZ_PLACE];
 
 PlacesUIUtils.URI_FLAVORS = [PlacesUtils.TYPE_X_MOZ_URL,
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -803,17 +803,19 @@ PlacesController.prototype = {
       }
       transactions.push(PlacesTransactions.Move({
         guid: node.bookmarkGuid,
         newParentGuid: args.moveToGuid,
       }));
     }
 
     if (transactions.length) {
-      await PlacesTransactions.batch(transactions);
+      await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => {
+        await PlacesTransactions.batch(transactions);
+      });
     }
   },
 
   /**
    * Sort the selected folder by name
    */
   async sortFolderByName() {
     let itemId = PlacesUtils.getConcreteItemId(this._view.selectedNode);
@@ -961,17 +963,19 @@ PlacesController.prototype = {
     var removedFolders = [];
 
     for (let range of ranges) {
       await this._removeRange(range, transactions, removedFolders);
     }
 
     if (transactions.length > 0) {
       if (PlacesUIUtils.useAsyncTransactions) {
-        await PlacesTransactions.batch(transactions);
+        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => {
+          await PlacesTransactions.batch(transactions);
+        });
       } else {
         var txn = new PlacesAggregatedTransaction(txnName, transactions);
         PlacesUtils.transactionManager.doTransaction(txn);
       }
     }
   },
 
   /**
@@ -1299,40 +1303,49 @@ PlacesController.prototype = {
     }
 
     let itemsToSelect = [];
     if (PlacesUIUtils.useAsyncTransactions) {
       if (ip.isTag) {
         let urls = items.filter(item => "uri" in item).map(item => Services.io.newURI(item.uri));
         await PlacesTransactions.Tag({ urls, tag: ip.tagName }).transact();
       } else {
-        await PlacesTransactions.batch(async function() {
-          let insertionIndex = await ip.getIndex();
-          let parent = ip.guid;
+        let transactionData = [];
+
+        let insertionIndex = await ip.getIndex();
+        let parent = ip.guid;
+
+        for (let item of items) {
+          let doCopy = action == "copy";
 
-          for (let item of items) {
-            let doCopy = action == "copy";
+          // If this is not a copy, check for safety that we can move the
+          // source, otherwise report an error and fallback to a copy.
+          if (!doCopy &&
+              !PlacesControllerDragHelper.canMoveUnwrappedNode(item)) {
+            Components.utils.reportError("Tried to move an unmovable " +
+                           "Places node, reverting to a copy operation.");
+            doCopy = true;
+          }
 
-            // If this is not a copy, check for safety that we can move the
-            // source, otherwise report an error and fallback to a copy.
-            if (!doCopy &&
-                !PlacesControllerDragHelper.canMoveUnwrappedNode(item)) {
-              Components.utils.reportError("Tried to move an unmovable " +
-                             "Places node, reverting to a copy operation.");
-              doCopy = true;
+          transactionData.push([item, type, parent, insertionIndex, doCopy]);
+
+          // Adjust index to make sure items are pasted in the correct
+          // position.  If index is DEFAULT_INDEX, items are just appended.
+          if (insertionIndex != PlacesUtils.bookmarks.DEFAULT_INDEX)
+            insertionIndex++;
+        }
+
+        await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactionData.length, async () => {
+          await PlacesTransactions.batch(async () => {
+            for (let item of transactionData) {
+              let guid = await PlacesUIUtils.getTransactionForData(
+                ...item).transact();
+              itemsToSelect.push(await PlacesUtils.promiseItemId(guid));
             }
-            let guid = await PlacesUIUtils.getTransactionForData(
-              item, type, parent, insertionIndex, doCopy).transact();
-            itemsToSelect.push(await PlacesUtils.promiseItemId(guid));
-
-            // Adjust index to make sure items are pasted in the correct
-            // position.  If index is DEFAULT_INDEX, items are just appended.
-            if (insertionIndex != PlacesUtils.bookmarks.DEFAULT_INDEX)
-              insertionIndex++;
-          }
+          });
         });
       }
     } else {
       let transactions = [];
       let insertionIndex = await ip.getIndex();
       for (let index = insertionIndex, i = 0; i < items.length; ++i) {
         if (ip.isTag) {
           // Pasting into a tag container means tagging the item, regardless of
@@ -1576,20 +1589,25 @@ var PlacesControllerDragHelper = {
     // removed.
     return !(PlacesUtils.nodeIsFolder(parentNode) &&
              PlacesUIUtils.isContentsReadOnly(parentNode)) &&
            !PlacesUtils.nodeIsTagQuery(parentNode);
   },
 
   /**
    * Handles the drop of one or more items onto a view.
-   * @param   insertionPoint
-   *          The insertion point where the items should be dropped
+   *
+   * @param {Object} insertionPoint The insertion point where the items should
+   *                                be dropped.
+   * @param {Object} dt             The dataTransfer information for the drop.
+   * @param {Object} view           The tree view where this object is being
+   *                                dropped to. This allows batching to take
+   *                                place.
    */
-  async onDrop(insertionPoint, dt) {
+  async onDrop(insertionPoint, dt, view) {
     let doCopy = ["copy", "link"].includes(dt.dropEffect);
 
     let transactions = [];
     let dropCount = dt.mozItemCount;
     let movedCount = 0;
     let parentGuid = insertionPoint.guid;
     let tagName = insertionPoint.tagName;
 
@@ -1702,17 +1720,19 @@ var PlacesControllerDragHelper = {
       }
     }
     // Check if we actually have something to add, if we don't it probably wasn't
     // valid, or it was moving to the same location, so just ignore it.
     if (!transactions.length) {
       return;
     }
     if (PlacesUIUtils.useAsyncTransactions) {
-      await PlacesTransactions.batch(transactions);
+      await PlacesUIUtils.batchUpdatesForNode(view && view.result, transactions.length, async () => {
+        await PlacesTransactions.batch(transactions);
+      });
     } else {
       let txn = new PlacesAggregatedTransaction("DropItems", transactions);
       PlacesUtils.transactionManager.doTransaction(txn);
     }
   },
 
   /**
    * Checks if we can insert into a container.
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -1448,21 +1448,24 @@ PlacesTreeView.prototype = {
   },
 
   drop: function PTV_drop(aRow, aOrientation, aDataTransfer) {
     // We are responsible for translating the |index| and |orientation|
     // parameters into a container id and index within the container,
     // since this information is specific to the tree view.
     let ip = this._getInsertionPoint(aRow, aOrientation);
     if (ip) {
-      PlacesControllerDragHelper.onDrop(ip, aDataTransfer)
-                                .catch(Components.utils.reportError);
+      PlacesControllerDragHelper.onDrop(ip, aDataTransfer, this)
+                                .catch(Components.utils.reportError)
+                                .then(() => {
+                                  // We should only clear the drop target once
+                                  // the onDrop is complete, as it is an async function.
+                                  PlacesControllerDragHelper.currentDropTarget = null;
+                                });
     }
-
-    PlacesControllerDragHelper.currentDropTarget = null;
   },
 
   getParentIndex: function PTV_getParentIndex(aRow) {
     let [, parentRow] = this._getParentByChildRow(aRow);
     return parentRow;
   },
 
   hasNextSibling: function PTV_hasNextSibling(aRow, aAfterIndex) {
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_async_batchUpdatesForNode.js
@@ -0,0 +1,93 @@
+// ================================================
+// Load mocking/stubbing library, sinon
+// docs: http://sinonjs.org/releases/v2.3.2/
+Cu.import("resource://gre/modules/Timer.jsm");
+Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js", this);
+/* globals sinon */
+// ================================================
+
+Cu.import("resource:///modules/PlacesUIUtils.jsm");
+
+add_task(async function test_no_result_node() {
+  let functionSpy = sinon.stub().returns(Promise.resolve());
+
+  await PlacesUIUtils.batchUpdatesForNode(null, 1, functionSpy);
+
+  Assert.ok(functionSpy.calledOnce,
+    "Passing a null result node should still call the wrapped function");
+});
+
+add_task(async function test_under_batch_threshold() {
+  let functionSpy = sinon.stub().returns(Promise.resolve());
+  let resultNode = {
+    QueryInterface() {
+      return this;
+    },
+    onBeginUpdateBatch: sinon.spy(),
+    onEndUpdateBatch: sinon.spy(),
+  };
+
+  await PlacesUIUtils.batchUpdatesForNode(resultNode, 1, functionSpy);
+
+  Assert.ok(functionSpy.calledOnce,
+    "Wrapped function should be called once");
+  Assert.ok(resultNode.onBeginUpdateBatch.notCalled,
+    "onBeginUpdateBatch should not have been called");
+  Assert.ok(resultNode.onEndUpdateBatch.notCalled,
+    "onEndUpdateBatch should not have been called");
+});
+
+add_task(async function test_over_batch_threshold() {
+  let functionSpy = sinon.stub().callsFake(() => {
+    Assert.ok(resultNode.onBeginUpdateBatch.calledOnce,
+      "onBeginUpdateBatch should have been called before the function");
+    Assert.ok(resultNode.onEndUpdateBatch.notCalled,
+      "onEndUpdateBatch should not have been called before the function");
+
+    return Promise.resolve()
+  });
+  let resultNode = {
+    QueryInterface() {
+      return this;
+    },
+    onBeginUpdateBatch: sinon.spy(),
+    onEndUpdateBatch: sinon.spy(),
+  };
+
+  await PlacesUIUtils.batchUpdatesForNode(resultNode, 100, functionSpy);
+
+  Assert.ok(functionSpy.calledOnce,
+    "Wrapped function should be called once");
+  Assert.ok(resultNode.onBeginUpdateBatch.calledOnce,
+    "onBeginUpdateBatch should have been called");
+  Assert.ok(resultNode.onEndUpdateBatch.calledOnce,
+    "onEndUpdateBatch should have been called");
+});
+
+add_task(async function test_wrapped_function_throws() {
+  let error = new Error("Failed!");
+  let functionSpy = sinon.stub().throws(error);
+  let resultNode = {
+    QueryInterface() {
+      return this;
+    },
+    onBeginUpdateBatch: sinon.spy(),
+    onEndUpdateBatch: sinon.spy(),
+  };
+
+  let raisedError;
+  try {
+    await PlacesUIUtils.batchUpdatesForNode(resultNode, 100, functionSpy);
+  } catch (ex) {
+    raisedError = ex;
+  }
+
+  Assert.ok(functionSpy.calledOnce,
+    "Wrapped function should be called once");
+  Assert.ok(resultNode.onBeginUpdateBatch.calledOnce,
+    "onBeginUpdateBatch should have been called");
+  Assert.ok(resultNode.onEndUpdateBatch.calledOnce,
+    "onEndUpdateBatch should have been called");
+  Assert.equal(raisedError, error,
+    "batchUpdatesForNode should have raised the error from the wrapped function");
+});
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -52,16 +52,17 @@ skip-if = os == "linux"
 [test_536081.js]
 [test_1085291.js]
 [test_1105208.js]
 [test_1105866.js]
 [test_adaptive.js]
 [test_adaptive_bug527311.js]
 [test_annotations.js]
 [test_asyncExecuteLegacyQueries.js]
+[test_async_batchUpdatesForNode.js]
 [test_async_in_batchmode.js]
 [test_async_transactions.js]
 skip-if = (os == "win" && os_version == "5.1") # Bug 1158887
 [test_autocomplete_stopSearch_no_throw.js]
 [test_bookmark_catobs.js]
 [test_bookmarks_json.js]
 [test_bookmarks_html.js]
 [test_bookmarks_html_corrupt.js]