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 draft
authorMark Banner <standard8@mozilla.com>
Tue, 05 Sep 2017 17:15:58 +0100
changeset 659971 6b52d63bd9a6b090ba3f4edc2670909fea8c0dc1
parent 659873 c959327c6b75cd4930a6ea087583c38b805e7524
child 730103 f38b0236b670041814093117105cad3eb66b4327
push id78252
push userbmo:standard8@mozilla.com
push dateWed, 06 Sep 2017 12:45:48 +0000
reviewersmak
bugs1396888
milestone57.0a1
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
browser/components/places/content/controller.js
browser/components/places/content/moveBookmarks.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_library_move_bookmarks.js
browser/components/places/tests/browser/head.js
--- 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;
     }
   }
 };