Bug 430442 - Undo and redo in the library are sometimes not correctly working, r=dietrich
authorMarco Bonardo <mak77@bonardo.net>
Thu, 11 Sep 2008 16:16:54 +0200
changeset 19158 9e6e421c6782b49effe522252e0dfe809dd3b881
parent 19157 a530d5f2f15a4938a02720e261842b70ec93250e
child 19159 90fcd51e53c808964ea7c414c2a45ef19fdd0a71
push id1982
push usermak77@bonardo.net
push dateThu, 11 Sep 2008 14:19:27 +0000
treeherderautoland@9e6e421c6782 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdietrich
bugs430442
milestone1.9.1b1pre
Bug 430442 - Undo and redo in the library are sometimes not correctly working, r=dietrich
browser/components/places/content/controller.js
browser/components/places/src/nsPlacesTransactionsService.js
browser/components/places/tests/unit/test_placesTxn.js
toolkit/components/places/src/nsNavBookmarks.cpp
toolkit/components/places/src/nsNavBookmarks.h
--- a/browser/components/places/content/controller.js
+++ b/browser/components/places/content/controller.js
@@ -858,21 +858,23 @@ PlacesController.prototype = {
 
   /**
    * Creates a set of transactions for the removal of a range of items. 
    * A range is an array of adjacent nodes in a view.
    * @param   [in] range
    *          An array of nodes to remove. Should all be adjacent. 
    * @param   [out] transactions
    *          An array of transactions.
+   * @param   [optional] removedFolders
+   *          An array of folder nodes that have already been removed.
    */
-  _removeRange: function PC__removeRange(range, transactions) {
+  _removeRange: function PC__removeRange(range, transactions, removedFolders) {
     NS_ASSERT(transactions instanceof Array, "Must pass a transactions array");
-
-    var removedFolders = [];
+    if (!removedFolders)
+      removedFolders = [];
 
     for (var i = 0; i < range.length; ++i) {
       var node = range[i];
       if (this._shouldSkipNode(node, removedFolders))
         continue;
 
       if (PlacesUtils.nodeIsFolder(node))
         removedFolders.push(node);
@@ -901,20 +903,21 @@ PlacesController.prototype = {
   /**
    * Removes the set of selected ranges from bookmarks.
    * @param   txnName
    *          See |remove|.
    */
   _removeRowsFromBookmarks: function PC__removeRowsFromBookmarks(txnName) {
     var ranges = this._view.getRemovableSelectionRanges();
     var transactions = [];
-    // Delete the selected rows. Do this by walking the selection backward, so
-    // that when undo is performed they are re-inserted in the correct order.
-    for (var i = ranges.length - 1; i >= 0 ; --i)
-      this._removeRange(ranges[i], transactions);
+    var removedFolders = [];
+
+    for (var i = 0; i < ranges.length; i++)
+      this._removeRange(ranges[i], transactions, removedFolders);
+
     if (transactions.length > 0) {
       var txn = PlacesUIUtils.ptm.aggregateTransactions(txnName, transactions);
       PlacesUIUtils.ptm.doTransaction(txn);
     }
   },
 
   /**
    * Removes the set of selected ranges from history.
--- a/browser/components/places/src/nsPlacesTransactionsService.js
+++ b/browser/components/places/src/nsPlacesTransactionsService.js
@@ -291,18 +291,21 @@ placesAggregateTransactions.prototype = 
       };
       PlacesUtils.bookmarks.runInBatchMode(callback, null);
     }
     else
       this.commit(true);
   },
 
   commit: function PAT_commit(aUndo) {
-    for (var i=0; i < this._transactions.length; ++i) {
-      var txn = this._transactions[i];
+    var transactions = this._transactions;
+    if (aUndo)
+      transactions.reverse();
+    for (var i = 0; i < transactions.length; i++) {
+      var txn = transactions[i];
       if (this.container > -1) 
         txn.wrappedJSObject.container = this.container;
       if (aUndo)
         txn.undoTransaction();
       else
         txn.doTransaction();
     }
   }
@@ -392,16 +395,17 @@ placesCreateItemTransactions.prototype =
     }
   }
 };
 
 function placesCreateSeparatorTransactions(aContainer, aIndex) {
   this._container = aContainer;
   this._index = typeof(aIndex) == "number" ? aIndex : -1;
   this._id = null;
+  this.redoTransaction = this.doTransaction;
 }
 
 placesCreateSeparatorTransactions.prototype = {
   __proto__: placesBaseTransaction.prototype,
 
   // childItemsTransaction support
   get container() { return this._container; },
   set container(val) { return this._container = val; },
@@ -445,37 +449,35 @@ placesCreateLivemarkTransactions.prototy
   undoTransaction: function PCLT_undoTransaction() {
     PlacesUtils.bookmarks.removeFolder(this._id);
   }
 };
 
 function placesMoveItemTransactions(aItemId, aNewContainer, aNewIndex) {
   this._id = aItemId;
   this._oldContainer = PlacesUtils.bookmarks.getFolderIdForItem(this._id);
-  this._oldIndex = PlacesUtils.bookmarks.getItemIndex(this._id);
   this._newContainer = aNewContainer;
   this._newIndex = aNewIndex;
   this.redoTransaction = this.doTransaction;
 }
 
 placesMoveItemTransactions.prototype = {
   __proto__: placesBaseTransaction.prototype,
 
   doTransaction: function PMIT_doTransaction() {
+    this._oldIndex = PlacesUtils.bookmarks.getItemIndex(this._id);
     PlacesUtils.bookmarks.moveItem(this._id, this._newContainer, this._newIndex);
-    // if newIndex == DEFAULT_INDEX we append, so get correct index for undo
-    if (this._newIndex == PlacesUtils.bookmarks.DEFAULT_INDEX)
-      this._newIndex = PlacesUtils.bookmarks.getItemIndex(this._id);
+    this._undoIndex = PlacesUtils.bookmarks.getItemIndex(this._id);
   },
 
   undoTransaction: function PMIT_undoTransaction() {
     // moving down in the same container takes in count removal of the item
     // so to revert positions we must move to oldIndex + 1
     if (this._newContainer == this._oldContainer &&
-        this._oldIndex > this._newIndex)
+        this._oldIndex > this._undoIndex)
       PlacesUtils.bookmarks.moveItem(this._id, this._oldContainer, this._oldIndex + 1);
     else
       PlacesUtils.bookmarks.moveItem(this._id, this._oldContainer, this._oldIndex);
   }
 };
 
 function placesRemoveItemTransaction(aItemId) {
   this.redoTransaction = this.doTransaction;
@@ -536,17 +538,17 @@ placesRemoveItemTransaction.prototype = 
     }
     else if (this._itemType == Ci.nsINavBookmarksService.TYPE_FOLDER) {
       this._removeTxn.undoTransaction();
       // Create children forwards to preserve parent-child relationships.
       for (var i = 0; i < this._transactions.length; ++i)
         this._transactions[i].undoTransaction();
     }
     else // TYPE_SEPARATOR
-      PlacesUtils.bookmarks.insertSeparator(this._oldContainer, this._oldIndex);
+      this._id = PlacesUtils.bookmarks.insertSeparator(this._oldContainer, this._oldIndex);
 
     if (this._annotations.length > 0)
       PlacesUtils.setAnnotationsForItem(this._id, this._annotations);
 
     PlacesUtils.bookmarks.setItemDateAdded(this._id, this._dateAdded);
     PlacesUtils.bookmarks.setItemLastModified(this._id, this._lastModified);
   },
 
--- a/browser/components/places/tests/unit/test_placesTxn.js
+++ b/browser/components/places/tests/unit/test_placesTxn.js
@@ -146,46 +146,74 @@ function run_test() {
               annotationService.getItemAnnotation(folderId, DESCRIPTION_ANNO));
   do_check_eq(observer._itemAddedIndex, bmStartIndex);
   do_check_eq(observer._itemAddedParent, root);
   do_check_eq(observer._itemAddedId, folderId);
   txn1.undoTransaction();
   do_check_eq(observer._itemRemovedId, folderId);
   do_check_eq(observer._itemRemovedFolder, root);
   do_check_eq(observer._itemRemovedIndex, bmStartIndex);
+  txn1.redoTransaction();
+  do_check_eq(observer._itemAddedIndex, bmStartIndex);
+  do_check_eq(observer._itemAddedParent, root);
+  do_check_eq(observer._itemAddedId, folderId);
+  txn1.undoTransaction();
+  do_check_eq(observer._itemRemovedId, folderId);
+  do_check_eq(observer._itemRemovedFolder, root);
+  do_check_eq(observer._itemRemovedIndex, bmStartIndex);
 
   // Test creating an item
   // Create to Root
   var txn2 = ptSvc.createItem(uri("http://www.example.com"), root, bmStartIndex, "Testing1");
   ptSvc.doTransaction(txn2); //Also testing doTransaction
   var b = (bmsvc.getBookmarkIdsForURI(uri("http://www.example.com"), {}))[0];
   do_check_eq(observer._itemAddedId, b);
   do_check_eq(observer._itemAddedIndex, bmStartIndex);
   do_check_true(bmsvc.isBookmarked(uri("http://www.example.com")));
   txn2.undoTransaction();
   do_check_eq(observer._itemRemovedId, b);
   do_check_eq(observer._itemRemovedIndex, bmStartIndex);
+  do_check_false(bmsvc.isBookmarked(uri("http://www.example.com")));
+  txn2.redoTransaction();
+  do_check_true(bmsvc.isBookmarked(uri("http://www.example.com")));
+  var newId = (bmsvc.getBookmarkIdsForURI(uri("http://www.example.com"), {}))[0];
+  do_check_eq(observer._itemAddedIndex, bmStartIndex);
+  do_check_eq(observer._itemAddedParent, root);
+  do_check_eq(observer._itemAddedId, newId);
+  txn2.undoTransaction();
+  do_check_eq(observer._itemRemovedId, newId);
+  do_check_eq(observer._itemRemovedFolder, root);
+  do_check_eq(observer._itemRemovedIndex, bmStartIndex);
 
-  // Create to a folder
+  // Create item to a folder
   var txn2a = ptSvc.createFolder("Folder", root, bmStartIndex);
   ptSvc.doTransaction(txn2a);
   var fldrId = bmsvc.getChildFolder(root, "Folder");
   var txn2b = ptSvc.createItem(uri("http://www.example2.com"), fldrId, bmStartIndex, "Testing1b");
   ptSvc.doTransaction(txn2b);
   var b2 = (bmsvc.getBookmarkIdsForURI(uri("http://www.example2.com"), {}))[0];
   do_check_eq(observer._itemAddedId, b2);
   do_check_eq(observer._itemAddedIndex, bmStartIndex);
   do_check_true(bmsvc.isBookmarked(uri("http://www.example2.com")));
   txn2b.undoTransaction();
   do_check_eq(observer._itemRemovedId, b2);
   do_check_eq(observer._itemRemovedIndex, bmStartIndex);
+  txn2b.redoTransaction();
+  newId = (bmsvc.getBookmarkIdsForURI(uri("http://www.example2.com"), {}))[0];
+  do_check_eq(observer._itemAddedIndex, bmStartIndex);
+  do_check_eq(observer._itemAddedParent, fldrId);
+  do_check_eq(observer._itemAddedId, newId);
+  txn2b.undoTransaction();
+  do_check_eq(observer._itemRemovedId, newId);
+  do_check_eq(observer._itemRemovedFolder, fldrId);
+  do_check_eq(observer._itemRemovedIndex, bmStartIndex);
 
   // Testing moving an item
   ptSvc.doTransaction(ptSvc.createItem(uri("http://www.example3.com"), root, -1, "Testing2"));
-  ptSvc.doTransaction(ptSvc.createItem(uri("http://www.example3.com"), root, -1, "Testing3"));   
+  ptSvc.doTransaction(ptSvc.createItem(uri("http://www.example3.com"), root, -1, "Testing3"));
   ptSvc.doTransaction(ptSvc.createItem(uri("http://www.example3.com"), fldrId, -1, "Testing4"));
   var bkmkIds = bmsvc.getBookmarkIdsForURI(uri("http://www.example3.com"), {});
   bkmkIds.sort();
   var bkmk1Id = bkmkIds[0];
   var bkmk2Id = bkmkIds[1];
   var bkmk3Id = bkmkIds[2];
 
   // Moving items between the same folder
@@ -197,100 +225,172 @@ function run_test() {
   do_check_eq(observer._itemMovedNewParent, root);
   do_check_eq(observer._itemMovedNewIndex, 2);
   txn3.undoTransaction();
   do_check_eq(observer._itemMovedId, bkmk1Id);
   do_check_eq(observer._itemMovedOldParent, root);
   do_check_eq(observer._itemMovedOldIndex, 2);
   do_check_eq(observer._itemMovedNewParent, root);
   do_check_eq(observer._itemMovedNewIndex, 1);
+  txn3.redoTransaction();
+  do_check_eq(observer._itemMovedId, bkmk1Id);
+  do_check_eq(observer._itemMovedOldParent, root);
+  do_check_eq(observer._itemMovedOldIndex, 1);
+  do_check_eq(observer._itemMovedNewParent, root);
+  do_check_eq(observer._itemMovedNewIndex, 2);
+  txn3.undoTransaction();
+  do_check_eq(observer._itemMovedId, bkmk1Id);
+  do_check_eq(observer._itemMovedOldParent, root);
+  do_check_eq(observer._itemMovedOldIndex, 2);
+  do_check_eq(observer._itemMovedNewParent, root);
+  do_check_eq(observer._itemMovedNewIndex, 1);
 
   // Moving items between different folders
   var txn3b = ptSvc.moveItem(bkmk1Id, fldrId, -1);
   txn3b.doTransaction();
   do_check_eq(observer._itemMovedId, bkmk1Id);
   do_check_eq(observer._itemMovedOldParent, root);
   do_check_eq(observer._itemMovedOldIndex, 1);
   do_check_eq(observer._itemMovedNewParent, fldrId);
   do_check_eq(observer._itemMovedNewIndex, 1);
   txn3.undoTransaction();
   do_check_eq(observer._itemMovedId, bkmk1Id);
   do_check_eq(observer._itemMovedOldParent, fldrId);
   do_check_eq(observer._itemMovedOldIndex, 1);
   do_check_eq(observer._itemMovedNewParent, root);
   do_check_eq(observer._itemMovedNewIndex, 1);
+  txn3b.redoTransaction();
+  do_check_eq(observer._itemMovedId, bkmk1Id);
+  do_check_eq(observer._itemMovedOldParent, root);
+  do_check_eq(observer._itemMovedOldIndex, 1);
+  do_check_eq(observer._itemMovedNewParent, fldrId);
+  do_check_eq(observer._itemMovedNewIndex, 1);
+  txn3.undoTransaction();
+  do_check_eq(observer._itemMovedId, bkmk1Id);
+  do_check_eq(observer._itemMovedOldParent, fldrId);
+  do_check_eq(observer._itemMovedOldIndex, 1);
+  do_check_eq(observer._itemMovedNewParent, root);
+  do_check_eq(observer._itemMovedNewIndex, 1);
 
   // Test Removing a Folder
   ptSvc.doTransaction(ptSvc.createFolder("Folder2", root, -1));
   var fldrId2 = bmsvc.getChildFolder(root, "Folder2");
   var txn4 = ptSvc.removeItem(fldrId2);
   txn4.doTransaction();
   do_check_eq(observer._itemRemovedId, fldrId2);
   do_check_eq(observer._itemRemovedFolder, root);
   do_check_eq(observer._itemRemovedIndex, 3);
   txn4.undoTransaction();
   do_check_eq(observer._itemAddedId, fldrId2);
   do_check_eq(observer._itemAddedParent, root);
   do_check_eq(observer._itemAddedIndex, 3);
+  txn4.redoTransaction();
+  do_check_eq(observer._itemRemovedId, fldrId2);
+  do_check_eq(observer._itemRemovedFolder, root);
+  do_check_eq(observer._itemRemovedIndex, 3);
+  txn4.undoTransaction();
+  do_check_eq(observer._itemAddedId, fldrId2);
+  do_check_eq(observer._itemAddedParent, root);
+  do_check_eq(observer._itemAddedIndex, 3);
 
   // Test removing an item
   var txn5 = ptSvc.removeItem(bkmk2Id);
   txn5.doTransaction();
   do_check_eq(observer._itemRemovedId, bkmk2Id);
   do_check_eq(observer._itemRemovedFolder, root);
   do_check_eq(observer._itemRemovedIndex, 2);
   txn5.undoTransaction();
+  var newbkmk2Id = observer._itemAddedId;
+  do_check_eq(observer._itemAddedParent, root);
+  do_check_eq(observer._itemAddedIndex, 2);
+  txn5.redoTransaction();
+  do_check_eq(observer._itemRemovedId, newbkmk2Id);
+  do_check_eq(observer._itemRemovedFolder, root);
+  do_check_eq(observer._itemRemovedIndex, 2);
+  txn5.undoTransaction();
   do_check_eq(observer._itemAddedParent, root);
   do_check_eq(observer._itemAddedIndex, 2);
 
   // Test creating a separator
   var txn6 = ptSvc.createSeparator(root, 1);
   txn6.doTransaction();
   var sepId = observer._itemAddedId;
   do_check_eq(observer._itemAddedIndex, 1);
   do_check_eq(observer._itemAddedParent, root);
   txn6.undoTransaction();
   do_check_eq(observer._itemRemovedId, sepId);
   do_check_eq(observer._itemRemovedFolder, root);
   do_check_eq(observer._itemRemovedIndex, 1);
+  txn6.redoTransaction();
+  var newSepId = observer._itemAddedId;
+  do_check_eq(observer._itemAddedIndex, 1);
+  do_check_eq(observer._itemAddedParent, root);
+  txn6.undoTransaction();
+  do_check_eq(observer._itemRemovedId, newSepId);
+  do_check_eq(observer._itemRemovedFolder, root);
+  do_check_eq(observer._itemRemovedIndex, 1);
 
   // Test removing a separator
   ptSvc.doTransaction(ptSvc.createSeparator(root, 1));
   var sepId2 = observer._itemAddedId;
   var txn7 = ptSvc.removeItem(sepId2);
   txn7.doTransaction();
   do_check_eq(observer._itemRemovedId, sepId2);
   do_check_eq(observer._itemRemovedFolder, root);
   do_check_eq(observer._itemRemovedIndex, 1);
   txn7.undoTransaction();
   do_check_eq(observer._itemAddedId, sepId2); //New separator created
   do_check_eq(observer._itemAddedParent, root);
   do_check_eq(observer._itemAddedIndex, 1);
+  txn7.redoTransaction();
+  do_check_eq(observer._itemRemovedId, sepId2);
+  do_check_eq(observer._itemRemovedFolder, root);
+  do_check_eq(observer._itemRemovedIndex, 1);
+  txn7.undoTransaction();
+  do_check_eq(observer._itemAddedId, sepId2); //New separator created
+  do_check_eq(observer._itemAddedParent, root);
+  do_check_eq(observer._itemAddedIndex, 1);
 
   // Test editing item title
   var txn8 = ptSvc.editItemTitle(bkmk1Id, "Testing2_mod");
   txn8.doTransaction();
   do_check_eq(observer._itemChangedId, bkmk1Id); 
   do_check_eq(observer._itemChangedProperty, "title");
   do_check_eq(observer._itemChangedValue, "Testing2_mod");
   txn8.undoTransaction();
   do_check_eq(observer._itemChangedId, bkmk1Id); 
   do_check_eq(observer._itemChangedProperty, "title");
   do_check_eq(observer._itemChangedValue, "Testing2");
+  txn8.redoTransaction();
+  do_check_eq(observer._itemChangedId, bkmk1Id); 
+  do_check_eq(observer._itemChangedProperty, "title");
+  do_check_eq(observer._itemChangedValue, "Testing2_mod");
+  txn8.undoTransaction();
+  do_check_eq(observer._itemChangedId, bkmk1Id); 
+  do_check_eq(observer._itemChangedProperty, "title");
+  do_check_eq(observer._itemChangedValue, "Testing2");
 
   // Test editing item uri
   var txn9 = ptSvc.editBookmarkURI(bkmk1Id, uri("http://newuri.com"));
   txn9.doTransaction();
   do_check_eq(observer._itemChangedId, bkmk1Id);
   do_check_eq(observer._itemChangedProperty, "uri");
   do_check_eq(observer._itemChangedValue, "http://newuri.com/");
   txn9.undoTransaction();
   do_check_eq(observer._itemChangedId, bkmk1Id);
   do_check_eq(observer._itemChangedProperty, "uri");
   do_check_eq(observer._itemChangedValue, "http://www.example3.com/");
+  txn9.redoTransaction();
+  do_check_eq(observer._itemChangedId, bkmk1Id);
+  do_check_eq(observer._itemChangedProperty, "uri");
+  do_check_eq(observer._itemChangedValue, "http://newuri.com/");
+  txn9.undoTransaction();
+  do_check_eq(observer._itemChangedId, bkmk1Id);
+  do_check_eq(observer._itemChangedProperty, "uri");
+  do_check_eq(observer._itemChangedValue, "http://www.example3.com/");
   
   // Test edit item description
   var txn10 = ptSvc.editItemDescription(bkmk1Id, "Description1");
   txn10.doTransaction();
   do_check_eq(observer._itemChangedId, bkmk1Id);
   do_check_eq(observer._itemChangedProperty, "bookmarkProperties/description");
 
   // Testing edit keyword
@@ -366,16 +466,24 @@ function run_test() {
   txn17.doTransaction();
   do_check_eq(2, bmsvc.getItemIndex(b1));
   do_check_eq(1, bmsvc.getItemIndex(b2));
   do_check_eq(0, bmsvc.getItemIndex(b3));
   txn17.undoTransaction();
   do_check_eq(0, bmsvc.getItemIndex(b1));
   do_check_eq(1, bmsvc.getItemIndex(b2));
   do_check_eq(2, bmsvc.getItemIndex(b3));
+  txn17.redoTransaction();
+  do_check_eq(2, bmsvc.getItemIndex(b1));
+  do_check_eq(1, bmsvc.getItemIndex(b2));
+  do_check_eq(0, bmsvc.getItemIndex(b3));
+  txn17.undoTransaction();
+  do_check_eq(0, bmsvc.getItemIndex(b1));
+  do_check_eq(1, bmsvc.getItemIndex(b2));
+  do_check_eq(2, bmsvc.getItemIndex(b3));
 
   // editBookmarkMicrosummary
   var tmpMs = mss.createMicrosummary(uri("http://testmicro.com"), 
                                      uri("http://dietrich.ganx4.com/mozilla/test-microsummary.xml"));
   ptSvc.doTransaction(
   ptSvc.createItem(uri("http://dietrich.ganx4.com/mozilla/test-microsummary-content.php"),
                    root, -1, "micro test", null, null, null));
   var bId = (bmsvc.getBookmarkIdsForURI(uri("http://dietrich.ganx4.com/mozilla/test-microsummary-content.php"),{}))[0];
@@ -431,9 +539,81 @@ function run_test() {
   do_check_eq(uneval(tagssvc.getTagsForURI(tagURI, { })), uneval(["bar","foo"]));
   var untagTxn = ptSvc.untagURI(tagURI, ["bar"]);
   untagTxn.doTransaction();
   do_check_eq(uneval(tagssvc.getTagsForURI(tagURI, { })), uneval(["foo"]));
   untagTxn.undoTransaction();
   do_check_eq(uneval(tagssvc.getTagsForURI(tagURI, { })), uneval(["bar","foo"]));
   untagTxn.redoTransaction();
   do_check_eq(uneval(tagssvc.getTagsForURI(tagURI, { })), uneval(["foo"]));
+
+  // Test aggregate removeItem transaction
+  var bkmk1Id = bmsvc.insertBookmark(root, uri("http://www.mozilla.org/"), 0, "Mozilla");
+  var bkmk2Id = bmsvc.insertSeparator(root, 1);
+  var bkmk3Id = bmsvc.createFolder(root, "folder", 2);
+  var bkmk3_1Id = bmsvc.insertBookmark(bkmk3Id, uri("http://www.mozilla.org/"), 0, "Mozilla");
+  var bkmk3_2Id = bmsvc.insertSeparator(bkmk3Id, 1);
+  var bkmk3_3Id = bmsvc.createFolder(bkmk3Id, "folder", 2);
+
+  var transactions = [];
+  transactions.push(ptSvc.removeItem(bkmk1Id));
+  transactions.push(ptSvc.removeItem(bkmk2Id));
+  transactions.push(ptSvc.removeItem(bkmk3Id));
+  var txn = ptSvc.aggregateTransactions("RemoveItems", transactions);
+
+  txn.doTransaction();
+  do_check_eq(bmsvc.getItemIndex(bkmk1Id), -1);
+  do_check_eq(bmsvc.getItemIndex(bkmk2Id), -1);
+  do_check_eq(bmsvc.getItemIndex(bkmk3Id), -1);
+  do_check_eq(bmsvc.getItemIndex(bkmk3_1Id), -1);
+  do_check_eq(bmsvc.getItemIndex(bkmk3_2Id), -1);
+  do_check_eq(bmsvc.getItemIndex(bkmk3_3Id), -1);
+
+  txn.undoTransaction();
+  var newBkmk1Id = bmsvc.getIdForItemAt(root, 0);
+  var newBkmk2Id = bmsvc.getIdForItemAt(root, 1);
+  var newBkmk3Id = bmsvc.getIdForItemAt(root, 2);
+  var newBkmk3_1Id = bmsvc.getIdForItemAt(newBkmk3Id, 0);
+  var newBkmk3_2Id = bmsvc.getIdForItemAt(newBkmk3Id, 1);
+  var newBkmk3_3Id = bmsvc.getIdForItemAt(newBkmk3Id, 2);
+  do_check_eq(bmsvc.getItemType(newBkmk1Id), bmsvc.TYPE_BOOKMARK);
+  do_check_eq(bmsvc.getBookmarkURI(newBkmk1Id).spec, "http://www.mozilla.org/");
+  do_check_eq(bmsvc.getItemType(newBkmk2Id), bmsvc.TYPE_SEPARATOR);
+  do_check_eq(bmsvc.getItemType(newBkmk3Id), bmsvc.TYPE_FOLDER);
+  do_check_eq(bmsvc.getItemTitle(newBkmk3Id), "folder");
+  do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_1Id), newBkmk3Id);
+  do_check_eq(bmsvc.getItemType(newBkmk3_1Id), bmsvc.TYPE_BOOKMARK);
+  do_check_eq(bmsvc.getBookmarkURI(newBkmk3_1Id).spec, "http://www.mozilla.org/");
+  do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_2Id), newBkmk3Id);
+  do_check_eq(bmsvc.getItemType(newBkmk3_2Id), bmsvc.TYPE_SEPARATOR);
+  do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_3Id), newBkmk3Id);
+  do_check_eq(bmsvc.getItemType(newBkmk3_3Id), bmsvc.TYPE_FOLDER);
+  do_check_eq(bmsvc.getItemTitle(newBkmk3_3Id), "folder");
+
+  txn.redoTransaction();
+  do_check_eq(bmsvc.getItemIndex(newBkmk1Id), -1);
+  do_check_eq(bmsvc.getItemIndex(newBkmk2Id), -1);
+  do_check_eq(bmsvc.getItemIndex(newBkmk3Id), -1);
+  do_check_eq(bmsvc.getItemIndex(newBkmk3_1Id), -1);
+  do_check_eq(bmsvc.getItemIndex(newBkmk3_2Id), -1);
+  do_check_eq(bmsvc.getItemIndex(newBkmk3_3Id), -1);
+
+  txn.undoTransaction();
+  newBkmk1Id = bmsvc.getIdForItemAt(root, 0);
+  newBkmk2Id = bmsvc.getIdForItemAt(root, 1);
+  newBkmk3Id = bmsvc.getIdForItemAt(root, 2);
+  newBkmk3_1Id = bmsvc.getIdForItemAt(newBkmk3Id, 0);
+  newBkmk3_2Id = bmsvc.getIdForItemAt(newBkmk3Id, 1);
+  newBkmk3_3Id = bmsvc.getIdForItemAt(newBkmk3Id, 2);
+  do_check_eq(bmsvc.getItemType(newBkmk1Id), bmsvc.TYPE_BOOKMARK);
+  do_check_eq(bmsvc.getBookmarkURI(newBkmk1Id).spec, "http://www.mozilla.org/");
+  do_check_eq(bmsvc.getItemType(newBkmk2Id), bmsvc.TYPE_SEPARATOR);
+  do_check_eq(bmsvc.getItemType(newBkmk3Id), bmsvc.TYPE_FOLDER);
+  do_check_eq(bmsvc.getItemTitle(newBkmk3Id), "folder");
+  do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_1Id), newBkmk3Id);
+  do_check_eq(bmsvc.getItemType(newBkmk3_1Id), bmsvc.TYPE_BOOKMARK);
+  do_check_eq(bmsvc.getBookmarkURI(newBkmk3_1Id).spec, "http://www.mozilla.org/");
+  do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_2Id), newBkmk3Id);
+  do_check_eq(bmsvc.getItemType(newBkmk3_2Id), bmsvc.TYPE_SEPARATOR);
+  do_check_eq(bmsvc.getFolderIdForItem(newBkmk3_3Id), newBkmk3Id);
+  do_check_eq(bmsvc.getItemType(newBkmk3_3Id), bmsvc.TYPE_FOLDER);
+  do_check_eq(bmsvc.getItemTitle(newBkmk3_3Id), "folder");
 }
--- a/toolkit/components/places/src/nsNavBookmarks.cpp
+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
@@ -1511,31 +1511,18 @@ nsNavBookmarks::RemoveFolder(PRInt64 aFo
 NS_IMPL_ISUPPORTS1(nsNavBookmarks::RemoveFolderTransaction, nsITransaction)
 
 NS_IMETHODIMP
 nsNavBookmarks::GetRemoveFolderTransaction(PRInt64 aFolder, nsITransaction** aResult)
 {
   // Create and initialize a RemoveFolderTransaction object that can be used to
   // recreate the folder safely later. 
 
-  nsCAutoString title;
-  nsresult rv = GetItemTitle(aFolder, title);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  PRInt64 parent;
-  PRInt32 index;
-  rv = GetParentAndIndexOfFolder(aFolder, &parent, &index);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  nsCAutoString type;
-  rv = GetFolderType(aFolder, type);
-  NS_ENSURE_SUCCESS(rv, rv);
-
   RemoveFolderTransaction* rft = 
-    new RemoveFolderTransaction(aFolder, parent, title, index, NS_ConvertUTF8toUTF16(type));
+    new RemoveFolderTransaction(aFolder);
   if (!rft)
     return NS_ERROR_OUT_OF_MEMORY;
 
   NS_ADDREF(*aResult = rft);
   return NS_OK;
 }
 
 NS_IMETHODIMP
--- a/toolkit/components/places/src/nsNavBookmarks.h
+++ b/toolkit/components/places/src/nsNavBookmarks.h
@@ -212,30 +212,34 @@ private:
   nsCOMPtr<mozIStorageStatement> mDBGetKeywordForURI;
   nsCOMPtr<mozIStorageStatement> mDBGetKeywordForBookmark;
   nsCOMPtr<mozIStorageStatement> mDBGetURIForKeyword;
 
   nsCOMPtr<nsIStringBundle> mBundle;
 
   class RemoveFolderTransaction : public nsITransaction {
   public:
-    RemoveFolderTransaction(PRInt64 aID, PRInt64 aParent, 
-                            const nsACString& aTitle, PRInt32 aIndex,
-                            const nsAString& aType) 
-                            : mID(aID),
-                              mParent(aParent),
-                              mIndex(aIndex){
-      mTitle = aTitle;
-      mType = aType;
-    }
-    
+    RemoveFolderTransaction(PRInt64 aID) : mID(aID) {}
+
     NS_DECL_ISUPPORTS
 
     NS_IMETHOD DoTransaction() {
       nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
+
+      nsresult rv = bookmarks->GetParentAndIndexOfFolder(mID, &mParent, &mIndex);
+      NS_ENSURE_SUCCESS(rv, rv);
+
+      rv = bookmarks->GetItemTitle(mID, mTitle);
+      NS_ENSURE_SUCCESS(rv, rv);
+
+      nsCAutoString type;
+      rv = bookmarks->GetFolderType(mID, type);
+      NS_ENSURE_SUCCESS(rv, rv);
+      mType = NS_ConvertUTF8toUTF16(type);
+
       return bookmarks->RemoveFolder(mID);
     }
 
     NS_IMETHOD UndoTransaction() {
       nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
       PRInt64 newFolder;
       return bookmarks->CreateContainerWithID(mID, mParent, mTitle, mType, PR_TRUE,
                                               &mIndex, &newFolder);