From a16d4003dce103628f78ce3e901d62ace3f4ccb6 Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Wed, 6 Sep 2017 21:53:08 +0100 Subject: [PATCH] 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 --HG-- extra : rebase_source : 7012972cb2e4af14500ca7a3376effdb7b1fd7e0 --- browser/components/places/PlacesUIUtils.jsm | 32 +++++++++++++++++-- .../places/content/bookmarkProperties.js | 28 +++------------- .../browser_bookmarkProperties_cancel.js | 6 ++-- 3 files changed, 37 insertions(+), 29 deletions(-) diff --git a/browser/components/places/PlacesUIUtils.jsm b/browser/components/places/PlacesUIUtils.jsm index 18ce7a88fdfe..1dba0a695e25 100644 --- a/browser/components/places/PlacesUIUtils.jsm +++ b/browser/components/places/PlacesUIUtils.jsm @@ -23,6 +23,8 @@ 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"); @@ -635,8 +637,7 @@ this.PlacesUIUtils = { * @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. @@ -648,8 +649,33 @@ this.PlacesUIUtils = { "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() { diff --git a/browser/components/places/content/bookmarkProperties.js b/browser/components/places/content/bookmarkProperties.js index 31674844ca07..36ed105d8288 100644 --- a/browser/components/places/content/bookmarkProperties.js +++ b/browser/components/places/content/bookmarkProperties.js @@ -62,8 +62,6 @@ 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; @@ -381,13 +379,7 @@ var BookmarkPropertiesPanel = { _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; @@ -395,18 +387,12 @@ var BookmarkPropertiesPanel = { _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 @@ -450,12 +436,8 @@ var BookmarkPropertiesPanel = { // 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; diff --git a/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js b/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js index b2b8a0d49863..ba3219e535e3 100644 --- a/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js +++ b/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js @@ -68,14 +68,14 @@ add_task(async function test_cancel_with_no_changes() { } ); - 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"); }); }); @@ -112,7 +112,7 @@ add_task(async function test_cancel_with_changes() { } ); - Assert.ok(PlacesTransactions.undo.calledOnce, + await BrowserTestUtils.waitForCondition(() => PlacesTransactions.undo.calledOnce, "undo should have been called once."); }); });