Bug 1396888 - Handle async moving of bookmarks in the caller for the move dialog, to avoid going out of scope whilst async actions are happening. r?mak
MozReview-Commit-ID: JhETMnmxNOD
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -277,17 +277,17 @@ PlacesController.prototype = {
break;
case "placesCmd_new:separator":
this.newSeparator().catch(Components.utils.reportError);
break;
case "placesCmd_show:info":
this.showBookmarkPropertiesForSelection();
break;
case "placesCmd_moveBookmarks":
- this.moveSelectedBookmarks();
+ this.moveSelectedBookmarks().catch(Components.utils.reportError);
break;
case "placesCmd_reload":
this.reloadSelectedLivemark();
break;
case "placesCmd_sortBy:name":
this.sortFolderByName().catch(Components.utils.reportError);
break;
case "placesCmd_createBookmark":
@@ -773,20 +773,48 @@ PlacesController.prototype = {
let itemId = await PlacesUtils.promiseItemId(guid);
// Select the new item.
this._view.selectItems([itemId], false);
},
/**
* Opens a dialog for moving the selected nodes.
*/
- moveSelectedBookmarks: function PC_moveBookmarks() {
+ async moveSelectedBookmarks() {
+ let args = {
+ // The guid of the folder to move bookmarks to. This will only be
+ // set in the useAsyncTransactions case.
+ moveToGuid: null,
+ // nodes is passed to support !useAsyncTransactions.
+ nodes: this._view.selectedNodes,
+ };
window.openDialog("chrome://browser/content/places/moveBookmarks.xul",
"", "chrome, modal",
- this._view.selectedNodes);
+ args);
+
+ if (!args.moveToGuid) {
+ return;
+ }
+
+ let transactions = [];
+
+ for (let node of this._view.selectedNodes) {
+ // Nothing to do if the node is already under the selected folder.
+ if (node.parent.bookmarkGuid == args.moveToGuid) {
+ continue;
+ }
+ transactions.push(PlacesTransactions.Move({
+ guid: node.bookmarkGuid,
+ newParentGuid: args.moveToGuid,
+ }));
+ }
+
+ if (transactions.length) {
+ await PlacesTransactions.batch(transactions);
+ }
},
/**
* Sort the selected folder by name
*/
async sortFolderByName() {
let itemId = PlacesUtils.getConcreteItemId(this._view.selectedNode);
if (!PlacesUIUtils.useAsyncTransactions) {
--- a/browser/components/places/content/moveBookmarks.js
+++ b/browser/components/places/content/moveBookmarks.js
@@ -10,17 +10,17 @@ var gMoveBookmarksDialog = {
get foldersTree() {
if (!this._foldersTree)
this._foldersTree = document.getElementById("foldersTree");
return this._foldersTree;
},
init() {
- this._nodes = window.arguments[0];
+ this._nodes = window.arguments[0].nodes;
this.foldersTree.place =
"place:excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1&folder=" +
PlacesUIUtils.allBookmarksFolderId;
},
onOK: function MBD_onOK(aEvent) {
let selectedNode = this.foldersTree.selectedNode;
@@ -40,26 +40,19 @@ var gMoveBookmarksDialog = {
}
if (transactions.length != 0) {
let txn = new PlacesAggregatedTransaction("Move Items", transactions);
PlacesUtils.transactionManager.doTransaction(txn);
}
return;
}
- PlacesTransactions.batch(async () => {
- let newParentGuid = await PlacesUtils.promiseItemGuid(selectedFolderId);
- for (let node of this._nodes) {
- // Nothing to do if the node is already under the selected folder.
- if (node.parent.itemId == selectedFolderId)
- continue;
- await PlacesTransactions.Move({ guid: node.bookmarkGuid,
- newParentGuid }).transact();
- }
- }).catch(Components.utils.reportError);
+ // Async transactions must do the move in the caller to avoid going out of
+ // scope whilst the dialog is still closing.
+ window.arguments[0].moveToGuid = selectedNode.bookmarkGuid;
},
newFolder: function MBD_newFolder() {
// The command is disabled when the tree is not focused
this.foldersTree.focus();
goDoCommand("placesCmd_new:folder");
}
};
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -40,16 +40,17 @@ subsuite = clipboard
[browser_library_commands.js]
[browser_library_delete_tags.js]
[browser_library_downloads.js]
[browser_library_infoBox.js]
[browser_library_left_pane_fixnames.js]
[browser_library_left_pane_middleclick.js]
[browser_library_left_pane_select_hierarchy.js]
[browser_library_middleclick.js]
+[browser_library_move_bookmarks.js]
[browser_library_open_leak.js]
[browser_library_openFlatContainer.js]
[browser_library_panel_leak.js]
[browser_library_search.js]
[browser_library_views_liveupdate.js]
[browser_markPageAsFollowedLink.js]
[browser_paste_into_tags.js]
subsuite = clipboard
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_library_move_bookmarks.js
@@ -0,0 +1,79 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * Test enabled commands in the left pane folder of the Library.
+ */
+
+registerCleanupFunction(async function() {
+ await PlacesUtils.bookmarks.eraseEverything();
+ await PlacesTestUtils.clearHistory();
+});
+
+add_task(async function test_moveBookmarks() {
+ let children = [{
+ title: "TestFolder",
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ }];
+
+ for (let i = 0; i < 10; i++) {
+ children.push({
+ title: `test${i}`,
+ url: `http://example.com/${i}`,
+ });
+ }
+
+ let bookmarks = await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.unfiledGuid,
+ children
+ });
+
+ let folderId = await PlacesUtils.promiseItemId(bookmarks[0].guid);
+
+ let itemIds = [];
+ let promiseMoveNotifications = [];
+ for (let i = 0; i < 10; i++) {
+ // + 1 due to the folder being inserted first.
+ let guid = bookmarks[i + 1].guid;
+
+ itemIds.push(await PlacesUtils.promiseItemId(guid));
+ promiseMoveNotifications.push(PlacesTestUtils.waitForNotification(
+ "onItemMoved",
+ (itemId, parentId, oldIndex, newParentId, newIndex, itemType, itemGuid,
+ oldParentGuid, newParentGuid) =>
+ itemGuid == guid && newParentGuid == bookmarks[0].guid
+ ));
+ }
+
+ let library = await promiseLibrary("UnfiledBookmarks");
+
+ library.ContentTree.view.selectItems(itemIds);
+
+ await withBookmarksDialog(
+ false,
+ () => {
+ library.ContentTree.view._controller.doCommand("placesCmd_moveBookmarks");
+ },
+ async (dialogWin) => {
+ let tree = dialogWin.document.getElementById("foldersTree");
+
+ tree.selectItems([PlacesUtils.unfiledBookmarksFolderId], true);
+
+ tree.selectItems([folderId], true);
+
+ dialogWin.document.documentElement.acceptDialog();
+
+ info("Waiting for notifications of moves");
+ await Promise.all(promiseMoveNotifications);
+ Assert.ok(true, "should have completed all moves successfully");
+ },
+ null,
+ "chrome://browser/content/places/moveBookmarks.xul",
+ true
+ );
+
+ library.close();
+});
--- a/browser/components/places/tests/browser/head.js
+++ b/browser/components/places/tests/browser/head.js
@@ -272,24 +272,26 @@ function isToolbarVisible(aToolbar) {
* @param openFn
* generator function causing the dialog to open
* @param taskFn
* the task to execute once the dialog is open
* @param closeFn
* A function to be used to wait for pending work when the dialog is
* closing. It is passed the dialog window handle and should return a promise.
*/
-var withBookmarksDialog = async function(autoCancel, openFn, taskFn, closeFn) {
+var withBookmarksDialog = async function(autoCancel, openFn, taskFn, closeFn,
+ dialogUrl = "chrome://browser/content/places/bookmarkProperties",
+ skipOverlayWait = false) {
let closed = false;
let dialogPromise = new Promise(resolve => {
Services.ww.registerNotification(function winObserver(subject, topic, data) {
if (topic == "domwindowopened") {
let win = subject.QueryInterface(Ci.nsIDOMWindow);
win.addEventListener("load", function() {
- ok(win.location.href.startsWith("chrome://browser/content/places/bookmarkProperties"),
+ ok(win.location.href.startsWith(dialogUrl),
"The bookmark properties dialog is open");
// This is needed for the overlay.
waitForFocus(() => {
resolve(win);
}, win);
}, {once: true});
} else if (topic == "domwindowclosed") {
Services.ww.unregisterNotification(winObserver);
@@ -301,19 +303,21 @@ var withBookmarksDialog = async function
info("withBookmarksDialog: opening the dialog");
// The dialog might be modal and could block our events loop, so executeSoon.
executeSoon(openFn);
info("withBookmarksDialog: waiting for the dialog");
let dialogWin = await dialogPromise;
// Ensure overlay is loaded
- info("waiting for the overlay to be loaded");
- await waitForCondition(() => dialogWin.gEditItemOverlay.initialized,
- "EditItemOverlay should be initialized");
+ if (!skipOverlayWait) {
+ info("waiting for the overlay to be loaded");
+ await waitForCondition(() => dialogWin.gEditItemOverlay.initialized,
+ "EditItemOverlay should be initialized");
+ }
// Check the first textbox is focused.
let doc = dialogWin.document;
let elt = doc.querySelector("textbox:not([collapsed=true])");
if (elt) {
info("waiting for focus on the first textfield");
await waitForCondition(() => doc.activeElement == elt.inputField,
"The first non collapsed textbox should have been focused");
@@ -324,20 +328,23 @@ var withBookmarksDialog = async function
let closePromise = () => Promise.resolve();
if (closeFn) {
closePromise = closeFn(dialogWin);
}
try {
await taskFn(dialogWin);
} finally {
+ if (!closed && !autoCancel) {
+ // Give the dialog a little time to close itself in the manually closing
+ // case.
+ await BrowserTestUtils.waitForCondition(() => closed,
+ "The test should have closed the dialog!");
+ }
if (!closed) {
- if (!autoCancel) {
- ok(false, "The test should have closed the dialog!");
- }
info("withBookmarksDialog: canceling the dialog");
doc.documentElement.cancelDialog();
await closePromise;
}
}
};