Bug 1392533 - Make the places tree view directly communicate batch notifications to the results. r?mak
MozReview-Commit-ID: HpN0v0jSwdK
--- 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]