Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r=mak

MozReview-Commit-ID: 7supBHcrpdu

--HG--
extra : rebase_source : 83947eae2c31253659a9acb0a924fcee48d11871

browser/components/places/content/bookmarkProperties.js

@@ -382,6 +382,7 @@ var BookmarkPropertiesPanel = {
     if (this._batching)
       return;
     if (PlacesUIUtils.useAsyncTransactions) {
+      this._topUndoEntry = PlacesTransactions.topUndoEntry;
       this._batchBlockingDeferred = PromiseUtils.defer();
       PlacesTransactions.batch(async () => {
         await this._batchBlockingDeferred.promise;
@@ -394,7 +395,7 @@ var BookmarkPropertiesPanel = {
 
   _endBatch() {
     if (!this._batching)
-      return;
+      return false;
 
     if (PlacesUIUtils.useAsyncTransactions) {
       this._batchBlockingDeferred.resolve();
@@ -403,6 +404,9 @@ var BookmarkPropertiesPanel = {
       PlacesUtils.transactionManager.endBatch(false);
     }
     this._batching = false;
+    let changed = this._topUndoEntry != PlacesTransactions.topUndoEntry;
+    delete this._topUndoEntry;
+    return changed;
   },
 
   // nsISupports
@@ -446,11 +450,14 @@ 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);
-    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;
   },
 

browser/components/places/tests/browser/browser.ini

@@ -21,6 +21,7 @@ support-files =
 [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/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.");
+  });
+});

browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js

@@ -64,6 +64,9 @@ add_task(async function() {
       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();
     }
   );
 

browser/components/places/tests/browser/browser_check_correct_controllers.js

@@ -5,6 +5,16 @@
 "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");