Bug 1392533 - Make the places tree view directly communicate batch notifications to the results. r=mak
authorMark Banner <standard8@mozilla.com>
Wed, 27 Sep 2017 15:26:35 +0100
changeset 383733 9a6261ed5c414add6f0fe588f300e830f54512c5
parent 383732 e1c1ebf60808e5e94b9a03d8cb577990ac0b9ca1
child 383736 3143c9015660e1d9dd54e9b9e047f58b21badfce
push id52348
push usermbanner@mozilla.com
push dateFri, 29 Sep 2017 10:54:23 +0000
treeherderautoland@9a6261ed5c41 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1392533
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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]