Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r=mak
authorMark Banner <standard8@mozilla.com>
Thu, 24 Aug 2017 12:51:38 +0100
changeset 428267 4ef718d6269b63f825176056428f8d507a9357f8
parent 428266 d54673f840a41ed3382c44c379deb33c813066f9
child 428268 bfb3d1d3cc8535f265dd8af74b815b770a9f430a
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1391393
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r=mak MozReview-Commit-ID: 7supBHcrpdu
browser/components/places/content/bookmarkProperties.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js
browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
browser/components/places/tests/browser/browser_check_correct_controllers.js
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -377,37 +377,41 @@ 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 {
       PlacesUtils.transactionManager.beginBatch(null);
     }
     this._batching = true;
   },
 
   _endBatch() {
     if (!this._batching)
-      return;
+      return false;
 
     if (PlacesUIUtils.useAsyncTransactions) {
       this._batchBlockingDeferred.resolve();
       this._batchBlockingDeferred = null;
     } else {
       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;
 
@@ -441,21 +445,24 @@ 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);
-    this._endBatch();
-    if (PlacesUIUtils.useAsyncTransactions)
-      PlacesTransactions.undo().catch(Components.utils.reportError);
-    else
+    let changed = this._endBatch();
+    if (PlacesUIUtils.useAsyncTransactions) {
+      if (changed) {
+        PlacesTransactions.undo().catch(Components.utils.reportError);
+      }
+    } else {
       PlacesUtils.transactionManager.undoTransaction();
+    }
     window.arguments[0].performed = false;
   },
 
   /**
    * This method checks to see if the input fields are in a valid state.
    *
    * @returns  true if the input is valid, false otherwise
    */
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -16,16 +16,17 @@ support-files =
 [browser_bookmark_folder_moveability.js]
 [browser_bookmarklet_windowOpen.js]
 support-files =
   pageopeningwindow.html
 [browser_bookmarkProperties_addFolderDefaultButton.js]
 [browser_bookmarkProperties_addKeywordForThisSearch.js]
 [browser_bookmarkProperties_addLivemark.js]
 [browser_bookmarkProperties_bookmarkAllTabs.js]
+[browser_bookmarkProperties_cancel.js]
 [browser_bookmarkProperties_editTagContainer.js]
 [browser_bookmarkProperties_readOnlyRoot.js]
 [browser_bookmarksProperties.js]
 [browser_check_correct_controllers.js]
 [browser_click_bookmarks_on_toolbar.js]
 [browser_cutting_bookmarks.js]
 subsuite = clipboard
 [browser_controller_onDrop.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js
@@ -0,0 +1,118 @@
+"use strict";
+
+/* global sinon */
+Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js");
+
+const sandbox = sinon.sandbox.create();
+
+registerCleanupFunction(async function() {
+  sandbox.restore();
+  delete window.sinon;
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesTestUtils.clearHistory();
+});
+
+let bookmarks; // Bookmarks added via insertTree.
+let bookmarkIds; // Map of Guids to Ids.
+
+add_task(async function setup() {
+  bookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children: [{
+      title: "bm1",
+      url: "http://example.com",
+    }, {
+      title: "bm2",
+      url: "http://example.com/2",
+    }]
+  });
+
+  bookmarkIds = await PlacesUtils.promiseManyItemIds([
+    bookmarks[0].guid, bookmarks[1].guid
+  ]);
+
+  // Undo is called asynchronously - and not waited for. Since we're not
+  // expecting undo to be called, we can only tell this by stubbing it.
+  sandbox.stub(PlacesTransactions, "undo").returns(Promise.resolve());
+});
+
+// Tests for bug 1391393 - Ensures that if the user cancels the bookmark properties
+// dialog without having done any changes, then no undo is called.
+add_task(async function test_cancel_with_no_changes() {
+  if (!PlacesUIUtils.useAsyncTransactions) {
+    Assert.ok(true, "Skipping test as async transactions are turned off");
+    return;
+  }
+
+  await withSidebarTree("bookmarks", async (tree) => {
+    tree.selectItems([bookmarkIds.get(bookmarks[0].guid)]);
+
+    // Delete the bookmark to put something in the undo history.
+    // Rather than calling cmd_delete, we call the remove directly, so that we
+    // can await on it finishing, and be guaranteed that there's something
+    // in the history.
+    await tree.controller.remove("Remove Selection");
+
+    tree.selectItems([bookmarkIds.get(bookmarks[1].guid)]);
+
+    // Now open the bookmarks dialog and cancel it.
+    await withBookmarksDialog(
+      true,
+      function openDialog() {
+        tree.controller.doCommand("placesCmd_show:info");
+      },
+      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");
+  });
+});
+
+add_task(async function test_cancel_with_changes() {
+  if (!PlacesUIUtils.useAsyncTransactions) {
+    Assert.ok(true, "Skipping test as async transactions are turned off");
+    return;
+  }
+
+  await withSidebarTree("bookmarks", async (tree) => {
+    tree.selectItems([bookmarkIds.get(bookmarks[1].guid)]);
+
+    // Now open the bookmarks dialog and cancel it.
+    await withBookmarksDialog(
+      true,
+      function openDialog() {
+        tree.controller.doCommand("placesCmd_show:info");
+      },
+      async function test(dialogWin) {
+        let acceptButton = dialogWin.document.documentElement.getButton("accept");
+        await BrowserTestUtils.waitForCondition(() => !acceptButton.disabled,
+          "The accept button should be enabled");
+
+        let promiseTitleChangeNotification = promiseBookmarksNotification(
+          "onItemChanged", (itemId, prop, isAnno, val) => prop == "title" && val == "n");
+
+        fillBookmarkTextField("editBMPanel_namePicker", "n", dialogWin);
+
+        // 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,
+      "undo should have been called once.");
+  });
+});
--- a/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
@@ -59,16 +59,19 @@ add_task(async function() {
 
       // Check the shortcut's title.
       Assert.equal(tree.selectedNode.title, "tag2", "The node has the correct title");
 
       // Check the tags have been edited.
       let tags = PlacesUtils.tagging.getTagsForURI(uri);
       Assert.equal(tags.length, 1, "Found the right number of tags");
       Assert.ok(tags.includes("tag2"), "Found the expected tag");
+
+      // Ensure that the addition really is finished before we hit cancel.
+      await PlacesTestUtils.promiseAsyncUpdates();
     }
   );
 
   await promiseTitleResetNotification;
 
   // Check the tag change has been reverted.
   let tags = PlacesUtils.tagging.getTagsForURI(uri);
   Assert.equal(tags.length, 1, "Found the right number of tags");
--- a/browser/components/places/tests/browser/browser_check_correct_controllers.js
+++ b/browser/components/places/tests/browser/browser_check_correct_controllers.js
@@ -1,15 +1,25 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/
  */
 
 "use strict";
 
 add_task(async function test() {
+  let bookmark = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    title: "Plain Bob",
+    url: "http://example.com"
+  });
+
+  registerCleanupFunction(async () => {
+    await PlacesUtils.bookmarks.remove(bookmark);
+  });
+
   let sidebarBox = document.getElementById("sidebar-box");
   is(sidebarBox.hidden, true, "The sidebar should be hidden");
 
   // Uncollapse the personal toolbar if needed.
   let toolbar = document.getElementById("PersonalToolbar");
   let wasCollapsed = toolbar.collapsed;
   if (wasCollapsed) {
     await promiseSetToolbarVisibility(toolbar, true);