Bug 373504 - Duplicating (pasting/dropping) a bookmark item does not copy over its annotations. r=dietrich.

browser/components/places/content/bookmarkProperties.js

@@ -47,9 +47,8 @@
  *           @ loadBookmarkInSidebar - optional, the default state for the
  *             "Load this bookmark in the sidebar" field.
  *         - "folder"
- *         - "folder with items"
+ *           @ URIList (Array of nsIURI objects) - optional, list of uris to
- *           @ URIList (Array of nsIURI objects)- list of uris to be bookmarked
+ *             be bookmarked under the new folder.
- *             under the new folder.
  *         - "livemark"
  *       @ uri (nsIURI object) - optional, the default uri for the new item.
  *         The property is not used for the "folder with items" type.
@@ -62,8 +61,9 @@
  *      Notes:
  *        1) If |uri| is set for a bookmark/livemark item and |title| isn't,
  *           the dialog will query the history tables for the title associated
- *           with the given uri. For "folder with items" folder, a default
+ *           with the given uri. If the dialog is set to adding a folder with
- *           static title is used ("[Folder Name]").
+ *           bookmark items under it (see URIList), a default static title is
+ *           used ("[Folder Name]").
  *        2) The index field of the the default insertion point is ignored if
  *           the folder picker is shown.
  *     - "edit" - for editing a bookmark item or a folder.
@@ -99,14 +99,6 @@ const LIVEMARK_CONTAINER = 2;
 
 const ACTION_EDIT = 0;
 const ACTION_ADD = 1;
-const ACTION_ADD_WITH_ITEMS = 2;
-
-/**
- * Supported options:
- * BOOKMARK_ITEM : ACTION_EDIT, ACTION_ADD
- * BOOKMARK_FOLDER : ACTION_ADD, ACTION_EDIT, ADD_WITH_ITEMS
- * LIVEMARK_CONTAINER : ACTION_ADD, ACTION_EDIT
- */
 
 var BookmarkPropertiesPanel = {
 
@@ -139,6 +131,7 @@ var BookmarkPropertiesPanel = {
   _itemTitle: "",
   _itemDescription: "",
   _microsummaries: null,
+  _URIList: null,
 
   // sizeToContent is not usable due to bug 90276, so we'll use resizeTo
   // instead and cache the bookmarks tree view size. See WSucks in the legacy
@@ -153,11 +146,12 @@ var BookmarkPropertiesPanel = {
    * button based on the variant of the dialog.
    */
   _getAcceptLabel: function BPP__getAcceptLabel() {
-    if (this._action == ACTION_ADD)
+    if (this._action == ACTION_ADD) {
-      return this._strings.getString("dialogAcceptLabelAddItem");
+      if (this._URIList)
-    if (this._action == ACTION_ADD_WITH_ITEMS)
+        return this._strings.getString("dialogAcceptLabelAddMulti");
-      return this._strings.getString("dialogAcceptLabelAddMulti");
 
+      return this._strings.getString("dialogAcceptLabelAddItem");
+    }
     return this._strings.getString("dialogAcceptLabelEdit");
   },
 
@@ -174,10 +168,11 @@ var BookmarkPropertiesPanel = {
 
       // folder
       NS_ASSERT(this._itemType == BOOKMARK_FOLDER, "bogus item type");
+      if (this._URIList)
+        return this._strings.getString("dialogTitleAddMulti");
+
       return this._strings.getString("dialogTitleAddFolder");
-    }
+    } 
-    if (this._action == ACTION_ADD_WITH_ITEMS)
-      return this._strings.getString("dialogTitleAddMulti");
     if (this._action == ACTION_EDIT) {
       return this._strings
                  .getFormattedString("dialogTitleEdit", [this._itemTitle]);
@@ -235,18 +230,14 @@ var BookmarkPropertiesPanel = {
         case "folder":
           this._action = ACTION_ADD;
           this._itemType = BOOKMARK_FOLDER;
-          if (!this._itemTitle)
-            this._itemTitle = this._strings.getString("newFolderDefault");
-          break;
-        case "folder with items":
-          NS_ASSERT("URIList" in dialogInfo,
-                    "missing URLList property for 'folder with items' action");
-          this._action = ACTION_ADD_WITH_ITEMS
-          this._itemType = BOOKMARK_FOLDER;
-          this._URIList = dialogInfo.URIList;
           if (!this._itemTitle) {
-            this._itemTitle =
+            if ("URIList" in dialogInfo) {
-              this._strings.getString("bookmarkAllTabsDefault");
+              this._itemTitle =
+                this._strings.getString("bookmarkAllTabsDefault");
+              this._URIList = dialogInfo.URIList;
+            }
+            else
+              this._itemTitle = this._strings.getString("newFolderDefault");
           }
           break;
         case "livemark":
@@ -651,7 +642,10 @@ var BookmarkPropertiesPanel = {
   },
 
   onDialogAccept: function BPP_onDialogAccept() {
-    this._saveChanges();
+    if (this._action == ACTION_ADD)
+      this._createNewItem();
+    else
+      this._saveChanges();
   },
 
   /**
@@ -720,101 +714,133 @@ var BookmarkPropertiesPanel = {
   },
 
   /**
-   * Get a create-item transaction for the item added in the dialog
+   * XXXmano todo:
+   *  1. Make setAnnotationsForURI unset a given annotation if the value field
+   *     is not set.
+   *  2. Replace PlacesEditItemDescriptionTransaction and
+   *     PlacesSetLoadInSidebarTransaction transaction with a generic
+   *     transaction to set/unset an annotation object.
+   *  3. Use the two helpers below with this new generic transaction in
+   *     _saveChanges.
    */
-  _getCreateItemTransaction: function() {
-    NS_ASSERT(this._action != ACTION_EDIT,
-              "_getCreateItemTransaction called when editing an item");
 
-    var containerId, indexInContainer = -1;
+  /**
-    if (isElementVisible(this._folderMenuList))
+   * Returns an object which could then be used to set/unset the
-      containerId = this._getFolderIdFromMenuList();
+   * description annotation for an item (any type).
-    else {
+   *
-      containerId = this._defaultInsertionPoint.folderId;
+   * @param aDescription
-      indexInContainer = this._defaultInsertionPoint.index;
+   *        The description of the item.
-    }
+   * @returns an object representing the annotation which could then be used
+   *          with get/setAnnotationsForURI of PlacesUtils.
+   */
+  _getDescriptionAnnotation:
+  function BPP__getDescriptionAnnotation(aDescription) {
+    var anno = { name: DESCRIPTION_ANNO,
+                 type: Ci.mozIStorageValueArray.VALUE_TYPE_TEXT,
+                 flags: 0,
+                 value: aDescription,
+                 expires: Ci.nsIAnnotationService.EXPIRE_NEVER };
 
-    if (this._itemType == BOOKMARK_ITEM) {
+    /**
-      var uri = PlacesUtils._uri(this._element("editURLBar").value);
+     * See todo note above
-      NS_ASSERT(uri, "cannot create an item without a uri");
+     * if (aDescription)
-      return new
+     *   anno.value = aDescription;
-        PlacesCreateItemTransaction(uri, containerId, indexInContainer);
+     */
-    }
+    return anno;
-    else if (this._itemType == LIVEMARK_CONTAINER) {
-      var feedURIString = this._element("feedLocationTextfield").value;
-      var feedURI = PlacesUtils._uri(feedURIString);
-
-      var siteURIString = this._element("feedSiteLocationTextfield").value;
-      var siteURI = null;
-      if (siteURIString)
-        siteURI = PlacesUtils._uri(siteURIString);
-
-      var name = this._element("namePicker").value;
-      return new PlacesCreateLivemarkTransaction(feedURI, siteURI,
-                                                 name, containerId,
-                                                 indexInContainer);
-    }
-    else if (this._itemType == BOOKMARK_FOLDER) { // folder
-      var name = this._element("namePicker").value;
-      return new PlacesCreateFolderTransaction(name, containerId,
-                                               indexInContainer);
-    }
   },
 
   /**
+   * Returns an object which could then be used to set/unset the
+   * load-in-sidebar annotation for a bookmark item.
+   *
+   * @param aLoadInSidebar
+   *        Whether to load the bookmark item in the sidebar in default
+   *        conditions.
+   * @returns an object representing the annotation which could then be used
+   *          with get/setAnnotationsForURI of PlacesUtils.
+   */
+  _getLoadInSidebarAnnotation:
+  function BPP__getLoadInSidebarAnnotation(aLoadInSidebar) {
+    var anno = { name: LOAD_IN_SIDEBAR_ANNO,
+                 type: Ci.mozIStorageValueArray.VALUE_TYPE_INTEGER,
+                 flags: 0,
+                 value: aLoadInSidebar,
+                 expires: Ci.nsIAnnotationService.EXPIRE_NEVER };
+
+    /**
+     * See todo note above
+     * if (anno)
+     *   anno.value = aLoadInSidebar;
+     */
+    return anno;
+  },
+
+  /**
+   * Dialog-accept code path when editing an item (any type).
+   *
    * Save any changes that might have been made while the properties dialog
    * was open.
    */
   _saveChanges: function BPP__saveChanges() {
-    var transactions = [];
+    var itemId;
-    var childItemsTransactions = [];
+    if (this._itemType == BOOKMARK_ITEM)
+      itemId = this._bookmarkId;
+    else
+      itemId = this._folderId;
 
-    // -1 is used when creating a new item (for child transactions).
+    var transactions = [];
-    var itemId = -1;
+
-    if (this._action == ACTION_EDIT) {
+    // title
-      if (this._itemType == BOOKMARK_ITEM)
+    var newTitle = this._element("userEnteredName").label;
-        itemId = this._bookmarkId;
+    if (newTitle != this._itemTitle)
-      else
+      transactions.push(this._getEditTitleTransaction(itemId, newTitle));
-        itemId = this._folderId;
+
+    // description
+    var description = this._element("descriptionTextfield").value;
+    if (description != this._itemDescription) {
+      transactions.push(new PlacesEditItemDescriptionTransaction(
+        itemId, description, this._itemType != BOOKMARK_ITEM));
     }
 
-    // for ACTION_ADD, the uri is set via insertItem
+    if (this._itemType == BOOKMARK_ITEM) {
-    if (this._action == ACTION_EDIT && this._itemType == BOOKMARK_ITEM) {
+      // location
       var url = PlacesUtils._uri(this._element("editURLBar").value);
       if (!this._bookmarkURI.equals(url))
         transactions.push(new PlacesEditBookmarkURITransaction(itemId, url));
-    }
 
-    // title transaction
+      // keyword transactions
-    // XXXmano: this isn't necessary for new folders. We should probably
-    // make insertItem take a title too (like insertFolder)
-    var newTitle = this._element("userEnteredName").label;
-    if (this._action != ACTION_EDIT || newTitle != this._itemTitle)
-      transactions.push(this._getEditTitleTransaction(itemId, newTitle));
-
-    // keyword transactions
-    if (this._itemType == BOOKMARK_ITEM) {
       var newKeyword = this._element("keywordTextfield").value;
-      if (this._action != ACTION_EDIT || newKeyword != this._bookmarkKeyword) {
+      if (newKeyword != this._bookmarkKeyword) {
         transactions.push(
           new PlacesEditBookmarkKeywordTransaction(itemId, newKeyword));
       }
-    }
 
-    // items under a new folder
+      // microsummaries
-    if (this._action == ACTION_ADD_WITH_ITEMS) {
+      var namePicker = this._element("namePicker");
-      for (var i = 0; i < this._URIList.length; ++i) {
+      var newMicrosummary = namePicker.selectedItem.microsummary;
-        var uri = this._URIList[i];
+
-        var title = this._getURITitleFromHistory(uri);
+      // Only add a microsummary update to the transaction if the
-        var txn = new PlacesCreateItemTransaction(uri, -1, -1);
+      // microsummary has actually changed, i.e. the user selected no
-        txn.childTransactions.push(
+      // microsummary, but the bookmark previously had one, or the user
-          new PlacesEditItemTitleTransaction(-1, title));
+      // selected a microsummary which is not the one the bookmark previously
-        childItemsTransactions.push(txn);
+      // had.
+      var placeURI = PlacesUtils.bookmarks.getItemURI(itemId);
+      if ((newMicrosummary == null && this._mss.hasMicrosummary(placeURI)) ||
+          (newMicrosummary != null &&
+           !this._mss.isMicrosummary(placeURI, newMicrosummary))) {
+        transactions.push(
+          new PlacesEditBookmarkMicrosummaryTransaction(itemId,
+                                                        newMicrosummary));
+      }
+
+      // load in sidebar
+      var loadInSidebarChecked = this._element("loadInSidebarCheckbox").checked;
+      if (loadInSidebarChecked != this._loadBookmarkInSidebar) {
+        transactions.push(
+          new PlacesSetLoadInSidebarTransaction(itemId, loadInSidebarChecked));
       }
     }
-
+    else if (this._itemType == LIVEMARK_CONTAINER) {
-    // See _getCreateItemTransaction for the add action case
-    if (this._action == ACTION_EDIT && this._itemType == LIVEMARK_CONTAINER) {
       var feedURIString = this._element("feedLocationTextfield").value;
       var feedURI = PlacesUtils._uri(feedURIString);
       if (!this._feedURI.equals(feedURI)) {
@@ -835,95 +861,143 @@ var BookmarkPropertiesPanel = {
       }
     }
 
-    // microsummaries
-    if (this._itemType == BOOKMARK_ITEM) {
-      var namePicker = this._element("namePicker");
-
-      // Something should always be selected in the microsummary menu,
-      // but if nothing is selected, then conservatively assume we should
-      // just display the bookmark title.
-      if (namePicker.selectedIndex == -1)
-        namePicker.selectedIndex = 0;
-
-      // This will set microsummary == undefined if the user selected
-      // the "don't display a microsummary" item.
-      var newMicrosummary = namePicker.selectedItem.microsummary;
-
-      if (this._action == ACTION_ADD && newMicrosummary) {
-        transactions.push(
-          new PlacesEditBookmarkMicrosummaryTransaction(itemId,
-                                                        newMicrosummary));
-      }
-      else if (this._action == ACTION_EDIT) {
-        NS_ASSERT(itemId != -1, "should have had a real bookmark id");
-
-        // Only add a microsummary update to the transaction if the
-        // microsummary has actually changed, i.e. the user selected no
-        // microsummary, but the bookmark previously had one, or the user
-        // selected a microsummary which is not the one the bookmark previously
-        // had.
-        var placeURI = PlacesUtils.bookmarks.getItemURI(itemId);
-        if ((newMicrosummary == null && this._mss.hasMicrosummary(placeURI)) ||
-            (newMicrosummary != null &&
-             !this._mss.isMicrosummary(placeURI, newMicrosummary))) {
-          transactions.push(
-            new PlacesEditBookmarkMicrosummaryTransaction(itemId,
-                                                          newMicrosummary));
-        }
-      }
-    }
-
-    // load in sidebar
-    if (this._itemType == BOOKMARK_ITEM) {
-      var checked = this._element("loadInSidebarCheckbox").checked;
-      if (this._action == ACTION_ADD ||
-          checked != this._loadBookmarkInSidebar) {
-        transactions.push(
-          new PlacesSetLoadInSidebarTransaction(itemId, checked));
-      }
-    }
-
-    // description
-    var description = this._element("descriptionTextfield").value;
-    if ((this._action != ACTION_EDIT && description) ||
-        (description != this._itemDescription)) {
-      var isFolder = this._itemType != BOOKMARK_ITEM;
-      transactions.push(new PlacesEditItemDescriptionTransaction(
-        itemId, description, this._itemType != BOOKMARK_ITEM));
-    }
-
     // If we have any changes to perform, do them via the
     // transaction manager passed by the opener so they can be undone.
     if (transactions.length > 0) {
       window.arguments[0].performed = true;
-
+      var aggregate =
-      if (this._action != ACTION_EDIT) {
+        new PlacesAggregateTransaction(this._getDialogTitle(), transactions);
-        var createTxn = this._getCreateItemTransaction();
+      this._tm.doTransaction(aggregate);
-        NS_ASSERT(createTxn, "failed to get a create-item transaction");
-
-        // Mark the containing folder as recently-used
-        this._markFolderAsRecentlyUsed(createTxn.container);
-
-        // use child transactions if we're creating a new item
-        createTxn.childTransactions =
-          createTxn.childTransactions.concat(transactions);
-
-        if (this._action == ACTION_ADD_WITH_ITEMS) {
-          // use child-items transactions for creating items under the root item
-          createTxn.childItemsTransactions =
-            createTxn.childItemsTransactions.concat(childItemsTransactions);
-        }
-        this._tm.doTransaction(createTxn);
-      }
-      else {
-        // just aggregate otherwise
-        var aggregate =
-          new PlacesAggregateTransaction(this._getDialogTitle(), transactions);
-        this._tm.doTransaction(aggregate);
-      }
     }
   },
 
+  /**
+   * [New Item Mode] Get the insertion point details for the new item, given
+   * dialog state and opening arguments.
+   *
+   * The container-identifier and insertion-index are returned separately in
+   * the form of [containerIdentifier, insertionIndex]
+   */
+  _getInsertionPointDetails: function BPP__getInsertionPointDetails() {
+    var containerId, indexInContainer = -1;
+    if (isElementVisible(this._folderMenuList))
+      containerId = this._getFolderIdFromMenuList();
+    else {
+      containerId = this._defaultInsertionPoint.folderId;
+      indexInContainer = this._defaultInsertionPoint.index;
+    }
+
+    return [containerId, indexInContainer];
+  },
+
+  /**
+   * Returns a transaction for creating a new bookmark item representing the
+   * various fields and opening arguments of the dialog.
+   */
+  _getCreateNewBookmarkTransaction:
+  function BPP__getCreateNewBookmarkTransaction() {
+    var uri = PlacesUtils._uri(this._element("editURLBar").value);
+    var title = this._element("userEnteredName").label;
+    var keyword = this._element("keywordTextfield").value;
+    var annotations = [];
+    var description = this._element("descriptionTextfield").value;
+    if (description)
+      annotations.push(this._getDescriptionAnnotation(description));
+
+    var loadInSidebar = this._element("loadInSidebarCheckbox").checked;
+    if (loadInSidebar)
+      annotations.push(this._getLoadInSidebarAnnotation(true));
+
+    var childTransactions = [];
+    var microsummary = this._element("namePicker").selectedItem.microsummary;
+    if (microsummary) {
+      childTransactions.push(
+        new PlacesEditBookmarkMicrosummaryTransaction(-1, microsummary));
+    }
+
+    var [container, index] = this._getInsertionPointDetails();
+    return new PlacesCreateItemTransaction(uri, container, index,
+                                           title, keyword, annotations,
+                                           childTransactions);
+  },
+
+  /**
+   * Returns a childItems-transactions array representing the URIList with
+   * which the dialog has been opened.
+   */
+  _getTransactionsForURIList: function BPP__getTransactionsForURIList() {
+    var transactions = [];
+    for (var i = 0; i < this._URIList.length; ++i) {
+      var uri = this._URIList[i];
+      var title = this._getURITitleFromHistory(uri);
+      transactions.push(new PlacesCreateItemTransaction(uri, -1, -1, title));
+    }
+    return transactions; 
+  },
+
+  /**
+   * Returns a transaction for creating a new folder item representing the
+   * various fields and opening arguments of the dialog.
+   */
+  _getCreateNewFolderTransaction:
+  function BPP__getCreateNewFolderTransaction() {
+    var folderName = this._element("namePicker").value;
+    var annotations = [];
+    var childItemsTransactions;
+    if (this._URIList)
+      childItemsTransactions = this._getTransactionsForURIList();
+    var description = this._element("descriptionTextfield").value;
+    if (description)
+      annotations.push(this._getDescriptionAnnotation(description));
+
+    var [container, index] = this._getInsertionPointDetails();
+    return new PlacesCreateFolderTransaction(folderName, container, index,
+                                             annotations,
+                                             childItemsTransactions);
+  },
+
+  /**
+   * Returns a transaction for creating a new live-bookmark item representing
+   * the various fields and opening arguments of the dialog.
+   */
+  _getCreateNewLivemarkTransaction:
+  function BPP__getCreateNewLivemarkTransaction() {
+    var [containerId, indexInContainer] = this._getInsertionPointDetails();
+    var feedURIString = this._element("feedLocationTextfield").value;
+    var feedURI = PlacesUtils._uri(feedURIString);
+
+    var siteURIString = this._element("feedSiteLocationTextfield").value;
+    var siteURI = null;
+    if (siteURIString)
+      siteURI = PlacesUtils._uri(siteURIString);
+
+    var name = this._element("namePicker").value;
+    return new PlacesCreateLivemarkTransaction(feedURI, siteURI,
+                                                name, containerId,
+                                                indexInContainer);
+  },
+
+  /**
+   * Dialog-accept code-path for creating a new item (any type)
+   */
+  _createNewItem: function BPP__getCreateItemTransaction() {
+    var createTxn;
+    if (this._itemType == BOOKMARK_FOLDER)
+      createTxn = this._getCreateNewFolderTransaction();
+    else if (this._itemType == LIVEMARK_CONTAINER)
+      createTxn = this._getCreateNewLivemarkTransaction();
+    else // BOOKMARK_ITEM
+      createTxn = this._getCreateNewBookmarkTransaction();
+
+    // Mark the containing folder as recently-used
+    this._markFolderAsRecentlyUsed(createTxn.container);
+
+    // perfrom our transaction do via the transaction manager passed by the
+    // opener so it can be undone.
+    window.arguments[0].performed = true;
+    this._tm.doTransaction(createTxn);
+  },
+
   onNamePickerInput: function BPP_onNamePickerInput() {
     this._element("userEnteredName").label = this._element("namePicker").value;
   },

browser/components/places/content/controller.js

@@ -1610,144 +1610,208 @@ PlacesAggregateTransaction.prototype = {
 
 
 /**
- * Create a new Folder
+ * Transaction for creating a new folder item.
+ *
+ * @param aName
+ *        the name of the new folder
+ * @param aContainer
+ *        the identifier of the folder in which the new folder should be
+ *        added.
+ * @param [optional] aIndex
+ *        the index of the item in aContainer, pass -1 or nothing to create
+ *        the item at the end of aContainer.
+ * @param [optional] aAnnotations
+ *        the annotations to set for the new folder.
+ * @param [optional] aChildItemsTransactions
+ *        array of transactions for items to be created under the new folder.
  */
-function PlacesCreateFolderTransaction(name, container, index) {
+function PlacesCreateFolderTransaction(aName, aContainer, aIndex,
-  NS_ASSERT(index >= -1, "invalid insertion index");
+                                       aAnnotations, aChildItemsTransaction) {
-  this._name = name;
+  this._name = aName;
-  this.container = container;
+  this._container = aContainer;
-  this._index = index;
+  this._index = typeof(aIndex) == "number" ? aIndex : -1;
+  this._annotations = aAnnotations;
   this._id = null;
-  this.childItemsTransactions = [];
+  this._childItemsTransactions = aChildItemsTransactions || [];
-  this.childTransactions = [];
   this.redoTransaction = this.doTransaction;
 }
 PlacesCreateFolderTransaction.prototype = {
   __proto__: PlacesBaseTransaction.prototype,
 
+  // childItemsTransaction support
+  get container() { return this._container; },
+  set container(val) { return this._container = val; },
+
   doTransaction: function PCFT_doTransaction() {
-    this.LOG("Create Folder: " + this._name + " in: " + this.container + "," + this._index);
+    var bookmarks = this.utils.bookmarks;
-    this._id = this.bookmarks.createFolder(this.container, this._name, this._index);
+    this._id = bookmarks.createFolder(this._container, this._name, this._index);
-    for (var i = 0; i < this.childItemsTransactions.length; ++i) {
+    if (this._annotations.length > 0) {
-      var txn = this.childItemsTransactions[i];
+      var placeURI = bookmarks.getFolderURI(this._id);
-      txn.container = this._id;
+      this.utils.setAnnotationsForURI(placeURI, this._annotations);
-      txn.doTransaction();
     }
-    for (var i = 0; i < this.childTransactions.length; ++i) {
+    for (var i = 0; i < this._childItemsTransactions.length; ++i) {
-      var txn = this.childTransactions[i];
+      var txn = this._childItemsTransactions[i];
-      txn.id = this._id;
+      txn.container = this._id;
       txn.doTransaction();
     }
   },
 
   undoTransaction: function PCFT_undoTransaction() {
-    this.LOG("UNCreate Folder: " + this._name + " from: " + this.container + "," + this._index);
     this.bookmarks.removeFolder(this._id);
-    for (var i = 0; i < this.childItemsTransactions.length; ++i) {
+    for (var i = 0; i < this._childItemsTransactions.length; ++i) {
       var txn = this.childItemsTransactions[i];
       txn.undoTransaction();
     }
-    for (var i = 0; i < this.childTransactions.length; ++i) {
-      var txn = this.childTransactions[i];
-      txn.undoTransaction();
-    }
   }
 };
 
 /**
- * Create a new Item
+ * Transaction for creating a new bookmark item
+ *
+ * @param aURI
+ *        the uri of the new bookmark (nsIURI)
+ * @param aContainer
+ *        the identifier of the folder in which the bookmark should be added.
+ * @param [optional] aIndex
+ *        the index of the item in aContainer, pass -1 or nothing to create
+ *        the item at the end of aContainer.
+ * @param [optional] aTitle
+ *        the title of the new bookmark.
+ * @param [optional] aKeyword
+ *        the keyword of the new bookmark.
+ * @param [optional] aAnnotations
+ *        the annotations to set for the new bookmark.
+ * @param [optional] aChildTransactions
+ *        child transactions to commit after creating the bookmark. Prefer
+ *        using any of the arguments above if possible. In general, a child
+ *        transations should be used only if the change it does has to be
+ *        reverted manually when removing the bookmark item.
+ *        a child transaction must support setting its bookmark-item
+ *        identifier via an "id" js setter.
  */
-function PlacesCreateItemTransaction(uri, container, index) {
+function PlacesCreateItemTransaction(aURI, aContainer, aIndex, aTitle,
-  this.LOG("PlacesCreateItemTransaction(" + uri.spec + ", " + container + ", " + index + ")");
+                                     aKeyword, aAnnotations,
-  NS_ASSERT(index >= -1, "invalid insertion index");
+                                     aChildTransactions) {
-  this._id = null;
+  this._uri = aURI;
-  this._uri = uri;
+  this._container = aContainer;
-  this.container = container;
+  this._index = typeof(aIndex) == "number" ? aIndex : -1;
-  this._index = index;
+  this._title = aTitle;
-  this.childTransactions = [];
+  this._keyword = aKeyword;
+  this._annotations = aAnnotations;
+  this._childTransactions = aChildTransactions || [];
   this.redoTransaction = this.doTransaction;
 }
 PlacesCreateItemTransaction.prototype = {
   __proto__: PlacesBaseTransaction.prototype,
 
+  // childItemsTransactions support for the create-folder transaction
+  get container() { return this._container; },
+  set container(val) { return this._container = val; },
+
   doTransaction: function PCIT_doTransaction() {
-    this.LOG("Create Item: " + this._uri.spec + " in: " + this.container + "," + this._index);
+    var bookmarks = this.utils.bookmarks;
-    this._id = this.bookmarks.insertItem(this.container, this._uri, this._index);
+    this._id = bookmarks.insertItem(this.container, this._uri, this._index);
-    for (var i = 0; i < this.childTransactions.length; ++i) {
+    if (this._title)
-      var txn = this.childTransactions[i];
+      bookmarks.setItemTitle(this._id, this._title);
+    if (this._keyword)
+      bookmarks.setKeywordForBookmark(this._id, this._keyword);
+    if (this._annotations && this._annotations.length > 0) {
+      var placeURI = bookmarks.getItemURI(this._id);
+      this.utils.setAnnotationsForURI(placeURI, this._annotations);
+    }
+
+    for (var i = 0; i < this._childTransactions.length; ++i) {
+      var txn = this._childTransactions[i];
       txn.id = this._id;
       txn.doTransaction();
     }
   },
 
   undoTransaction: function PCIT_undoTransaction() {
-    this.LOG("UNCreate Item: bookmark " + this._id + " for uri " + this._uri.spec + " from: " + this.container + "," + this._index);
+    this.utils.bookmarks.removeItem(this._id);
-    this.bookmarks.removeItem(this._id);
+    for (var i = 0; i < this._childTransactions.length; ++i) {
-    for (var i = 0; i < this.childTransactions.length; ++i) {
+      var txn = this._childTransactions[i];
-      var txn = this.childTransactions[i];
       txn.undoTransaction();
     }
   }
 };
 
 /**
- * Create a new Separator
+ * Transaction for creating a new separator item
+ *
+ * @param aContainer
+ *        the identifier of the folder in which the separator should be
+ *        added.
+ * @param [optional] aIndex
+ *        the index of the item in aContainer, pass -1 or nothing to create
+ *        the separator at the end of aContainer.
  */
-function PlacesCreateSeparatorTransaction(container, index) {
+function PlacesCreateSeparatorTransaction(aContainer, aIndex) {
-  NS_ASSERT(index >= -1, "invalid insertion index");
+  this._container = aContainer;
-  this.container = container;
+  this._index = typeof(aIndex) == "number" ? aIndex : -1;
-  this._index = index;
   this._id = null;
 }
 PlacesCreateSeparatorTransaction.prototype = {
   __proto__: PlacesBaseTransaction.prototype,
 
-  doTransaction: function PIST_doTransaction() {
+  // childItemsTransaction support
+  get container() { return this._container; },
+  set container(val) { return this._container = val; },
+
+  doTransaction: function PCST_doTransaction() {
     this.LOG("Create separator in: " + this.container + "," + this._index);
     this._id = this.bookmarks.insertSeparator(this.container, this._index);
   },
 
-  undoTransaction: function PIST_undoTransaction() {
+  undoTransaction: function PCST_undoTransaction() {
     this.LOG("UNCreate separator from: " + this.container + "," + this._index);
     this.bookmarks.removeChildAt(this.container, this._index);
   }
 };
 
 /**
- * Create a new live bookmark
+ * Transaction for creating a new live-bookmark item.
+ *
+ * @see nsILivemarksService::createLivemark for documentation regarding the
+ * first three arguments.
+ *
+ * @param aContainer
+ *        the identifier of the folder in which the live-bookmark should be
+ *        added.
+ * @param [optional] aIndex
+ *        the index of the item in aContainer, pass -1 or nothing to create
+ *        the item at the end of aContainer.
+ * @param [optional] aAnnotations
+ *        the annotations to set for the new live-bookmark.
  */
 function PlacesCreateLivemarkTransaction(aFeedURI, aSiteURI, aName,
-                                         aContainer, aIndexInContainer) {
+                                         aContainer, aIndex, aAnnotations) {
   this._feedURI = aFeedURI;
   this._siteURI = aSiteURI;
   this._name = aName;
   this._container = aContainer;
-  this._indexInContainer = aIndexInContainer;
+  this._index = typeof(aIndex) == "number" ? aIndex : -1;
-  this.childTransactions = [];
+  this._annotations = aAnnotations;
 }
 PlacesCreateLivemarkTransaction.prototype = {
   __proto__: PlacesBaseTransaction.prototype,
 
+  // childItemsTransaction support
+  get container() { return this._container; },
+  set container(val) { return this._container = val; },
+
   doTransaction: function PCLT_doTransaction() {
-    this._id = this.livemarks.createLivemark(this._container,
+    this._id = this.utils.livemarks
-                                             this._name,
+                   .createLivemark(this._container, this._name, this._siteURI,
-                                             this._siteURI,
+                                   this._feedURI, this._index);
-                                             this._feedURI,
+    if (this._annotations)  {
-                                             this._indexInContainer);
+      var placeURI = this.utils.bookmarks.getItemURI(this._id);
-    for (var i = 0; i < this.childTransactions.length; ++i) {
+      this.utils.setAnnotationsForURI(placeURI, this._annotations);
-      var txn = this.childTransactions[i];
-      txn.id = this._id;
-      txn.doTransaction();
     }
   },
 
   undoTransaction: function PCLT_undoTransaction() {
     this.bookmarks.removeFolder(this._id);
-
-    for (var i = 0; i < this.childTransactions.length; ++i) {
-      var txn = this.childTransactions[i];
-      txn.undoTransaction();
-    }
   }
 };
 
@@ -1953,7 +2017,7 @@ PlacesRemoveSeparatorTransaction.prototype = {
  * Edit a bookmark's title.
  */
 function PlacesEditItemTitleTransaction(id, newTitle) {
-  this.id = id;
+  this._id = id;
   this._newTitle = newTitle;
   this._oldTitle = "";
   this.redoTransaction = this.doTransaction;
@@ -1962,12 +2026,12 @@ PlacesEditItemTitleTransaction.prototype = {
   __proto__: PlacesBaseTransaction.prototype,
 
   doTransaction: function PEITT_doTransaction() {
-    this._oldTitle = this.bookmarks.getItemTitle(this.id);
+    this._oldTitle = this.bookmarks.getItemTitle(this._id);
-    this.bookmarks.setItemTitle(this.id, this._newTitle);
+    this.bookmarks.setItemTitle(this._id, this._newTitle);
   },
 
   undoTransaction: function PEITT_undoTransaction() {
-    this.bookmarks.setItemTitle(this.id, this._oldTitle);
+    this.bookmarks.setItemTitle(this._id, this._oldTitle);
   }
 };
 
@@ -1975,7 +2039,7 @@ PlacesEditItemTitleTransaction.prototype = {
  * Edit a bookmark's uri.
  */
 function PlacesEditBookmarkURITransaction(aBookmarkId, aNewURI) {
-  this.id = aBookmarkId;
+  this._id = aBookmarkId;
   this._newURI = aNewURI;
   this.redoTransaction = this.doTransaction;
 }
@@ -1983,12 +2047,12 @@ PlacesEditBookmarkURITransaction.prototype = {
   __proto__: PlacesBaseTransaction.prototype,
 
   doTransaction: function PEBUT_doTransaction() {
-    this._oldURI = this.bookmarks.getBookmarkURI(this.id);
+    this._oldURI = this.bookmarks.getBookmarkURI(this._id);
-    this.bookmarks.changeBookmarkURI(this.id, this._newURI);
+    this.bookmarks.changeBookmarkURI(this._id, this._newURI);
   },
 
   undoTransaction: function PEBUT_undoTransaction() {
-    this.bookmarks.changeBookmarkURI(this.id, this._oldURI);
+    this.bookmarks.changeBookmarkURI(this._id, this._oldURI);
   }
 };
 

browser/components/places/content/utils.js

@@ -476,11 +476,8 @@ var PlacesUtils = {
    * @returns A nsITransaction object that performs the copy.
    */
   _getURIItemCopyTransaction: function (aURI, aContainer, aIndex) {
-    var itemTitle = this.history.getPageTitle(aURI);
+    var title = this.history.getPageTitle(aURI);
-    var createTxn = new PlacesCreateItemTransaction(aURI, aContainer, aIndex);
+    return new PlacesCreateItemTransaction(aURI, aContainer, aIndex, title);
-    createTxn.childTransactions.push(
-      new PlacesEditItemTitleTransaction(-1, itemTitle));
-    return new PlacesAggregateTransaction("ItemCopy", [createTxn]);
   },
 
   /**
@@ -494,13 +491,16 @@ var PlacesUtils = {
    *          The index within the container the item is copied to
    * @returns A nsITransaction object that performs the copy.
    */
-  _getBookmarkItemCopyTransaction: function (aID, aContainer, aIndex) {
+  _getBookmarkItemCopyTransaction: function (aId, aContainer, aIndex) {
-    var itemURL = this.bookmarks.getBookmarkURI(aID);
+    var bookmarks = this.bookmarks;
-    var itemTitle = this.bookmarks.getItemTitle(aID);
+    var itemURL = bookmarks.getBookmarkURI(aId);
-    var createTxn = new PlacesCreateItemTransaction(itemURL, aContainer, aIndex);
+    var itemTitle = bookmarks.getItemTitle(aId);
-    createTxn.childTransactions.push(
+    var keyword = bookmarks.getKeywordForBookmark(aId);
-      new PlacesEditItemTitleTransaction(-1, itemTitle));
+    var annos = this.getAnnotationsForURI(bookmarks.getItemURI(aId));
-    return new PlacesAggregateTransaction("ItemCopy", [createTxn]);
+    var createTxn =
+      new PlacesCreateItemTransaction(itemURL, aContainer, aIndex, itemTitle,
+                                      keyword, annos);
+    return createTxn;
   },
 
   /**
@@ -517,40 +517,47 @@ var PlacesUtils = {
   _getFolderCopyTransaction:
   function PU__getFolderCopyTransaction(aData, aContainer, aIndex) {
     var self = this;
-    function getChildTransactions(folderId) {
+    function getChildItemsTransactions(aFolderId) {
-      var childTransactions = [];
+      var childItemsTransactions = [];
-      var children = self.getFolderContents(folderId, false, false);
+      var children = self.getFolderContents(aFolderId, false, false);
       var cc = children.childCount;
-      var txn = null;
       for (var i = 0; i < cc; ++i) {
+        var txn = null;
         var node = children.getChild(i);
         if (self.nodeIsFolder(node)) {
           var nodeFolderId = asFolder(node).folderId;
           var title = self.bookmarks.getFolderTitle(nodeFolderId);
-          txn = new PlacesCreateFolderTransaction(title, -1, aIndex);
+          var annos = self.getAnnotationsForURI(self._uri(node.uri));
-          txn.childTransactions = getChildTransactions(nodeFolderId);
+          var folderItemsTransactions =
+            getChildItemsTransactions(nodeFolderId);
+          txn = new PlacesCreateFolderTransaction(title, -1, aIndex, annos,
+                                                  folderItemsTransactions);
         }
         else if (self.nodeIsBookmark(node)) {
-          txn = self._getBookmarkItemCopyTransaction(self._uri(node.uri), -1,
+          txn = self._getBookmarkItemCopyTransaction(node.bookmarkId, -1,
                                                      aIndex);
         }
         else if (self.nodeIsURI(node) || self.nodeIsQuery(node)) {
+          // XXXmano: can this ^ ever happen?
           txn = self._getURIItemCopyTransaction(self._uri(node.uri), -1,
                                                 aIndex);
         }
-        else if (self.nodeIsSeparator(node)) {
+        else if (self.nodeIsSeparator(node))
           txn = new PlacesCreateSeparatorTransaction(-1, aIndex);
-        }
+
-        childTransactions.push(txn);
+        NS_ASSERT(txn, "Unexpected item under a bookmarks folder");
+        if (txn)
+          childItemsTransactions.push(txn);
       }
-      return childTransactions;
+      return childItemsTransactions;
     }
 
     var title = this.bookmarks.getFolderTitle(aData.id);
+    var annos =
+      this.getAnnotationsForURI(this.bookmarks.getFolderURI(aData.id));
     var createTxn =
-      new PlacesCreateFolderTransaction(title, aContainer, aIndex);
+      new PlacesCreateFolderTransaction(title, aContainer, aIndex, annos,
-    createTxn.childTransactions =
+                                        getChildItemsTransactions(aData.id));
-      getChildTransactions(aData.id, createTxn);
     return createTxn;
   },
 
@@ -922,7 +929,7 @@ var PlacesUtils = {
               "showAddMultiBookmarkUI expects a list of nsIURI objects");
     var info = {
       action: "add",
-      type: "folder with items",
+      type: "folder",
       hiddenRows: ["description"],
       URIList: aURIList
     };
@@ -930,7 +937,7 @@ var PlacesUtils = {
   },
 
   /**
-   * Opens the bookmark properties panel for a given bookmark idnetifier.
+   * Opens the bookmark properties panel for a given bookmark identifier.
    *
    * @param aId
    *        bookmark identifier for which the properties are to be shown