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
--- 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.");
});
});