Bug 1397387 - Move async actions out of the opening/closing cycles of the bookmarks dialog to ensure they finish. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 06 Sep 2017 21:53:08 +0100
changeset 660793 bf84e2d20fdfbef14abc56bf9d4d44335cc5fb40
parent 660422 d8e238b811d3dc74515065ae8cab6c74baf0295f
child 730363 8aa6cde87d3c252607af5a2a5aea1eaee2e492cd
push id78537
push userbmo:standard8@mozilla.com
push dateThu, 07 Sep 2017 15:21:13 +0000
reviewersmak
bugs1397387
milestone57.0a1
Bug 1397387 - Move async actions out of the opening/closing cycles of the bookmarks dialog to ensure they finish. r?mak MozReview-Commit-ID: 9H5hPfTNv0S
browser/components/places/PlacesUIUtils.jsm
browser/components/places/content/bookmarkProperties.js
browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -18,16 +18,18 @@ Cu.import("resource://gre/modules/Places
 XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
                                   "resource://gre/modules/PluralForm.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
                                   "resource://gre/modules/PrivateBrowsingUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "RecentWindow",
                                   "resource:///modules/RecentWindow.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
+                                  "resource://gre/modules/PromiseUtils.jsm");
 
 // PlacesUtils exposes multiple symbols, so we can't use defineLazyModuleGetter.
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesTransactions",
                                   "resource://gre/modules/PlacesTransactions.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Weave",
@@ -630,31 +632,55 @@ this.PlacesUIUtils = {
    *        Describes the item to be edited/added in the dialog.
    *        See documentation at the top of bookmarkProperties.js
    * @param aWindow
    *        Owner window for the new dialog.
    *
    * @see documentation at the top of bookmarkProperties.js
    * @return true if any transaction has been performed, false otherwise.
    */
-  showBookmarkDialog:
-  function PUIU_showBookmarkDialog(aInfo, aParentWindow) {
+  showBookmarkDialog(aInfo, aParentWindow) {
     // Preserve size attributes differently based on the fact the dialog has
     // a folder picker or not, since it needs more horizontal space than the
     // other controls.
     let hasFolderPicker = !("hiddenRows" in aInfo) ||
                           !aInfo.hiddenRows.includes("folderPicker");
     // Use a different chrome url to persist different sizes.
     let dialogURL = hasFolderPicker ?
                     "chrome://browser/content/places/bookmarkProperties2.xul" :
                     "chrome://browser/content/places/bookmarkProperties.xul";
 
     let features = "centerscreen,chrome,modal,resizable=yes";
+
+    let topUndoEntry;
+    let batchBlockingDeferred;
+
+    if (this.useAsyncTransactions) {
+      // Set the transaction manager into batching mode.
+      topUndoEntry = PlacesTransactions.topUndoEntry;
+      batchBlockingDeferred = PromiseUtils.defer();
+      PlacesTransactions.batch(async () => {
+        await batchBlockingDeferred.promise;
+      });
+    }
+
     aParentWindow.openDialog(dialogURL, "", features, aInfo);
-    return ("performed" in aInfo && aInfo.performed);
+
+    let performed = ("performed" in aInfo && aInfo.performed);
+
+    if (this.useAsyncTransactions) {
+      batchBlockingDeferred.resolve();
+
+      if (!performed &&
+          topUndoEntry != PlacesTransactions.topUndoEntry) {
+        PlacesTransactions.undo().catch(Components.utils.reportError);
+      }
+    }
+
+    return performed;
   },
 
   _getTopBrowserWin: function PUIU__getTopBrowserWin() {
     return RecentWindow.getMostRecentBrowserWindow();
   },
 
   /**
    * set and fetch a favicon. Can only be used from the parent process.
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -57,18 +57,16 @@
  * been performed by the dialog.
  */
 
 /* import-globals-from editBookmarkOverlay.js */
 
 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
                                   "resource://gre/modules/PrivateBrowsingUtils.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
-                                  "resource://gre/modules/PromiseUtils.jsm");
 
 const BOOKMARK_ITEM = 0;
 const BOOKMARK_FOLDER = 1;
 const LIVEMARK_CONTAINER = 2;
 
 const ACTION_EDIT = 0;
 const ACTION_ADD = 1;
 
@@ -376,42 +374,30 @@ var BookmarkPropertiesPanel = {
 
   // Hack for implementing batched-Undo around the editBookmarkOverlay
   // instant-apply code. For all the details see the comment above beginBatch
   // in browser-places.js
   _batchBlockingDeferred: null,
   _beginBatch() {
     if (this._batching)
       return;
-    if (PlacesUIUtils.useAsyncTransactions) {
-      this._topUndoEntry = PlacesTransactions.topUndoEntry;
-      this._batchBlockingDeferred = PromiseUtils.defer();
-      PlacesTransactions.batch(async () => {
-        await this._batchBlockingDeferred.promise;
-      });
-    } else {
+    if (!PlacesUIUtils.useAsyncTransactions) {
       PlacesUtils.transactionManager.beginBatch(null);
     }
     this._batching = true;
   },
 
   _endBatch() {
     if (!this._batching)
-      return false;
+      return;
 
-    if (PlacesUIUtils.useAsyncTransactions) {
-      this._batchBlockingDeferred.resolve();
-      this._batchBlockingDeferred = null;
-    } else {
+    if (!PlacesUIUtils.useAsyncTransactions) {
       PlacesUtils.transactionManager.endBatch(false);
     }
     this._batching = false;
-    let changed = this._topUndoEntry != PlacesTransactions.topUndoEntry;
-    delete this._topUndoEntry;
-    return changed;
   },
 
   // nsISupports
   QueryInterface: function BPP_QueryInterface(aIID) {
     if (aIID.equals(Ci.nsIDOMEventListener) ||
         aIID.equals(Ci.nsISupports))
       return this;
 
@@ -445,22 +431,18 @@ var BookmarkPropertiesPanel = {
     window.arguments[0].performed = true;
   },
 
   onDialogCancel() {
     // The order here is important! We have to uninit the panel first, otherwise
     // changes done as part of Undo may change the panel contents and by
     // that force it to commit more transactions.
     gEditItemOverlay.uninitPanel(true);
-    let changed = this._endBatch();
-    if (PlacesUIUtils.useAsyncTransactions) {
-      if (changed) {
-        PlacesTransactions.undo().catch(Components.utils.reportError);
-      }
-    } else {
+    this._endBatch();
+    if (!PlacesUIUtils.useAsyncTransactions) {
       PlacesUtils.transactionManager.undoTransaction();
     }
     window.arguments[0].performed = false;
   },
 
   /**
    * This method checks to see if the input fields are in a valid state.
    *
--- a/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js
@@ -63,24 +63,24 @@ add_task(async function test_cancel_with
       },
       async function test(dialogWin) {
         let acceptButton = dialogWin.document.documentElement.getButton("accept");
         await BrowserTestUtils.waitForCondition(() => !acceptButton.disabled,
           "The accept button should be enabled");
       }
     );
 
-    Assert.ok(PlacesTransactions.undo.notCalled, "undo should not have been called");
-
     // Check the bookmark is still removed.
     Assert.ok(!(await PlacesUtils.bookmarks.fetch(bookmarks[0].guid)),
       "The originally removed bookmark should not exist.");
 
     Assert.ok(await PlacesUtils.bookmarks.fetch(bookmarks[1].guid),
       "The second bookmark should still exist");
+
+    Assert.ok(PlacesTransactions.undo.notCalled, "undo should not have been called");
   });
 });
 
 add_task(async function test_cancel_with_changes() {
   if (!PlacesUIUtils.useAsyncTransactions) {
     Assert.ok(true, "Skipping test as async transactions are turned off");
     return;
   }
@@ -107,12 +107,12 @@ add_task(async function test_cancel_with
         // The dialog is instant apply.
         await promiseTitleChangeNotification;
 
         // Ensure that the addition really is finished before we hit cancel.
         await PlacesTestUtils.promiseAsyncUpdates();
       }
     );
 
-    Assert.ok(PlacesTransactions.undo.calledOnce,
+    await BrowserTestUtils.waitForCondition(() => PlacesTransactions.undo.calledOnce,
       "undo should have been called once.");
   });
 });