Bug 411088 - when deleting a tagged bookmark from the places organizer, the tag remains. r=dietrich, a=beltzner.

browser/components/places/src/nsPlacesTransactionsService.js

@@ -97,6 +97,19 @@ placesTransactionsService.prototype = {
   },
 
   removeItem: function placesRmItem(id) {
+    if (id == PlacesUtils.tagsFolderId || id == PlacesUtils.placesRootId ||
+        id == PlacesUtils.bookmarksMenuFolderId ||
+        id == PlacesUtils.toolbarFolderId)
+      throw Cr.NS_ERROR_INVALID_ARG;
+
+    // if the item lives within a tag container, use the tagging transactions
+    var parent = PlacesUtils.bookmarks.getFolderIdForItem(id);
+    var grandparent = PlacesUtils.bookmarks.getFolderIdForItem(parent);
+    if (grandparent == PlacesUtils.tagsFolderId) {
+      var uri = PlacesUtils.bookmarks.getBookmarkURI(id);
+      return this.untagURI(uri, [parent]);
+    }
+
     return new placesRemoveItemTransaction(id);
   },
 
@@ -866,6 +879,7 @@ placesSortFolderByNameTransactions.prototype = {
 function placesTagURITransaction(aURI, aTags) {
   this._uri = aURI;
   this._tags = aTags;
+  this._unfiledItemId = -1;
   this.redoTransaction = this.doTransaction;
 }
 
@@ -873,10 +887,22 @@ placesTagURITransaction.prototype = {
   __proto__: placesBaseTransaction.prototype,
 
   doTransaction: function PTU_doTransaction() {
+    if (PlacesUtils.getBookmarksForURI(this._uri).length == 0) {
+      // Force an unfiled bookmark first
+      this.__unfiledItemId =
+        PlacesUtils.bookmarks
+                   .insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                   this._uri, -1,
+                                   PlacesUtils.history.getPageTitle(this._uri));
+    }
     PlacesUtils.tagging.tagURI(this._uri, this._tags);
   },
 
   undoTransaction: function PTU_undoTransaction() {
+    if (this.__unfiledItemId != -1) {
+      PlacesUtils.bookmarks.removeItem(this.__unfiledItemId);
+      this.__unfiledItemId = -1;
+    }
     PlacesUtils.tagging.untagURI(this._uri, this._tags);
   }
 };

toolkit/components/places/public/nsITaggingService.idl

@@ -52,7 +52,9 @@ interface nsITaggingService : nsISupports
    * @param aURI
    *        the URL to tag.
    * @param aTags
-   *        Array of tags to set for the given URL.
+   *        Array of tags to set for the given URL. Each element within the
+   *        array can be either a tag name or a concrete itemId of a tag
+   *        container.
    */
   void tagURI(in nsIURI aURI, in nsIVariant aTags);
 
@@ -64,7 +66,8 @@ interface nsITaggingService : nsISupports
    *        the URL to un-tag.
    * @param aTags
    *        Array of tags to unset. pass null to remove all tags from the given
-   *        url.
+   *        url. Each element within the array can be either a tag name or a
+   *        concrete itemId of a tag container.
    */
   void untagURI(in nsIURI aURI, in nsIVariant aTags);
 

toolkit/components/places/src/nsTaggingService.js

@@ -103,13 +103,20 @@ TaggingService.prototype = {
   /**
    * If there's no tag with the given name, null is returned;
    */
-  _getTagNode: function TS__getTagIndex(aName) {
-    var nameLower = aName.toLowerCase();
+  _getTagNode: function TS__getTagIndex(aTagNameOrId) {
+    if (!aTagNameOrId)
+      throw Cr.NS_ERROR_INVALID_ARG;
+
+    var nameLower = null;
+    if (typeof(aTagNameOrId) == "string")
+      nameLower = aTagNameOrId.toLowerCase();
+
     var root = this._tagsResult.root;
     var cc = root.childCount;
     for (var i=0; i < cc; i++) {
       var child = root.getChild(i);
-      if (child.title.toLowerCase() == nameLower)
+      if ((nameLower && child.title.toLowerCase() == nameLower) ||
+          child.itemId === aTagNameOrId)
         return child;
     }
 
@@ -158,11 +165,11 @@ TaggingService.prototype = {
       throw Cr.NS_ERROR_INVALID_ARG;
 
     for (var i=0; i < aTags.length; i++) {
-      if (aTags[i].length == 0)
-        throw Cr.NS_ERROR_INVALID_ARG;
-
       var tagNode = this._getTagNode(aTags[i]);
       if (!tagNode) {
+        if (typeof(aTags[i]) == "number")
+          throw Cr.NS_ERROR_INVALID_ARG;
+
         var tagId = this._createTag(aTags[i]);
         this._bms.insertBookmark(tagId, aURI, this._bms.DEFAULT_INDEX, null);
       }
@@ -174,7 +181,7 @@ TaggingService.prototype = {
         // _getTagNode ignores case sensitivity
         // rename the tag container so the places view would match the
         // user-typed values
-        if (tagNode.title != aTags[i])
+        if (typeof(aTags[i]) == "string" && tagNode.title != aTags[i])
           this._bms.setItemTitle(tagNode.itemId, aTags[i]);
       }
     }
@@ -209,9 +216,6 @@ TaggingService.prototype = {
     }
 
     for (var i=0; i < aTags.length; i++) {
-      if (aTags[i].length == 0)
-        throw Cr.NS_ERROR_INVALID_ARG;
-
       var tagNode = this._getTagNode(aTags[i]);
       if (tagNode) {
         var itemId = { };
@@ -220,6 +224,8 @@ TaggingService.prototype = {
           this._removeTagIfEmpty(tagNode.itemId);
         }
       }
+      else if (typeof(aTags[i]) == "number")
+        throw Cr.NS_ERROR_INVALID_ARG;
     }
   },
 

toolkit/components/places/tests/unit/test_tagging.js

@@ -84,19 +84,24 @@ function run_test() {
 
   var tag1node = tagRoot.getChild(0)
                         .QueryInterface(Ci.nsINavHistoryContainerResultNode);
+  var tag1itemId = tag1node.itemId;
+
   do_check_eq(tag1node.title, "tag 1");
   tag1node.containerOpen = true;
   do_check_eq(tag1node.childCount, 2);
 
-  // Tagging the same url twice with the same tag should be a no-op
+  // Tagging the same url twice (or even trice!) with the same tag should be a
+  // no-op
   tagssvc.tagURI(uri1, ["tag 1"]);
   do_check_eq(tag1node.childCount, 2);
-
-  // the former should be ignored.
+  tagssvc.tagURI(uri1, [tag1itemId]);
+  do_check_eq(tag1node.childCount, 2);
   do_check_eq(tagRoot.childCount, 1);
+
   // also tests bug 407575
-  tagssvc.tagURI(uri1, ["tag 1", "tag 2", "Tag 1", "Tag 2"]);
+  tagssvc.tagURI(uri1, [tag1itemId, "tag 1", "tag 2", "Tag 1", "Tag 2"]);
   do_check_eq(tagRoot.childCount, 2);
+  do_check_eq(tag1node.childCount, 2);
 
   // test getTagsForURI
   var uri1tags = tagssvc.getTagsForURI(uri1, {});
@@ -126,4 +131,5 @@ function run_test() {
   // removing the last uri from a tag should remove the tag-container
   tagssvc.untagURI(uri2, ["tag 1"]);
   do_check_eq(tagRoot.childCount, 1);
+  
 }