Bug 463513 - Tagging service could hold a fixed cache instead of an open node, r=mak77
authorDietrich Ayala <dietrich@mozilla.com>
Wed, 10 Dec 2008 11:40:13 +0100
changeset 22561 89a175c70c25df9a47519fa6967e9f4214acaa6d
parent 22560 29f1fa84f8fd739b81e39335776d64f2d8de8a89
child 22562 9952fee851dae674ede7e4a25ee667ebea43a3ff
push id4069
push usermak77@bonardo.net
push dateWed, 10 Dec 2008 10:44:37 +0000
treeherdermozilla-central@9952fee851da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak77
bugs463513
milestone1.9.2a1pre
Bug 463513 - Tagging service could hold a fixed cache instead of an open node, r=mak77
toolkit/components/places/src/nsTaggingService.js
toolkit/components/places/tests/unit/test_tagging.js
--- a/toolkit/components/places/src/nsTaggingService.js
+++ b/toolkit/components/places/src/nsTaggingService.js
@@ -34,99 +34,84 @@
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 const Cc = Components.classes;
 const Ci = Components.interfaces;
 const Cr = Components.results;
+const Cu = Components.utils;
 
-Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 const NH_CONTRACTID = "@mozilla.org/browser/nav-history-service;1";
 const BMS_CONTRACTID = "@mozilla.org/browser/nav-bookmarks-service;1";
 const IO_CONTRACTID = "@mozilla.org/network/io-service;1";
 const ANNO_CONTRACTID = "@mozilla.org/browser/annotation-service;1";
 const FAV_CONTRACTID = "@mozilla.org/browser/favicon-service;1";
 const OBSS_CONTRACTID = "@mozilla.org/observer-service;1";
 
 var gIoService = Cc[IO_CONTRACTID].getService(Ci.nsIIOService);
 
 /**
  * The Places Tagging Service
  */
 function TaggingService() {
+  this._bms = Cc[BMS_CONTRACTID].getService(Ci.nsINavBookmarksService);
+  this._bms.addObserver(this, false);
+
+  this._obss = Cc[OBSS_CONTRACTID].getService(Ci.nsIObserverService);
+  this._obss.addObserver(this, "xpcom-shutdown", false);
 }
 
 TaggingService.prototype = {
-  get _bms() {
-    if (!this.__bms)
-      this.__bms = Cc[BMS_CONTRACTID].getService(Ci.nsINavBookmarksService);
-    return this.__bms;
-  },
-
   get _history() {
     if (!this.__history)
       this.__history = Cc[NH_CONTRACTID].getService(Ci.nsINavHistoryService);
     return this.__history;
   },
 
   get _annos() {
     if (!this.__annos)
       this.__annos =  Cc[ANNO_CONTRACTID].getService(Ci.nsIAnnotationService);
     return this.__annos;
   },
 
-  get _tagsResult() {
-    if (!this.__tagsResult) {
-      var options = this._history.getNewQueryOptions();
-      var query = this._history.getNewQuery();
-      query.setFolders([this._bms.tagsFolder], 1);
-      this.__tagsResult = this._history.executeQuery(query, options);
-      this.__tagsResult.root.containerOpen = true;
-      this.__tagsResult.viewer = this;
-
-      // we need to null out the result on shutdown
-      var observerSvc = Cc[OBSS_CONTRACTID].getService(Ci.nsIObserverService);
-      observerSvc.addObserver(this, "xpcom-shutdown", false);
-    }
-    return this.__tagsResult;
-  },
-
   // Feed XPCOMUtils
   classDescription: "Places Tagging Service",
   contractID: "@mozilla.org/browser/tagging-service;1",
   classID: Components.ID("{bbc23860-2553-479d-8b78-94d9038334f7}"),
 
   // nsISupports
   QueryInterface: XPCOMUtils.generateQI([Ci.nsITaggingService,
+                                         Ci.nsINavBookmarkObserver,
                                          Ci.nsIObserver]),
 
   /**
-   * If there's no tag with the given name, null is returned;
+   * If there's no tag with the given name or id, null is returned;
    */
-  _getTagNode: function TS__getTagIndex(aTagNameOrId) {
+  _getTagResult: function TS__getTagResult(aTagNameOrId) {
     if (!aTagNameOrId)
       throw Cr.NS_ERROR_INVALID_ARG;
 
-    var nameLower = null;
+    var tagId = null;
     if (typeof(aTagNameOrId) == "string")
-      nameLower = aTagNameOrId.toLowerCase();
+      tagId = this._getItemIdForTag(aTagNameOrId);
+    else
+      tagId = aTagNameOrId;
 
-    var root = this._tagsResult.root;
-    var cc = root.childCount;
-    for (var i=0; i < cc; i++) {
-      var child = root.getChild(i);
-      if ((nameLower && child.title.toLowerCase() == nameLower) ||
-          child.itemId === aTagNameOrId)
-        return child;
-    }
+    if (tagId == -1)
+      return null;
 
-    return null;
+    var options = this._history.getNewQueryOptions();
+    var query = this._history.getNewQuery();
+    query.setFolders([tagId], 1);
+    var result = this._history.executeQuery(query, options);
+    return result;
   },
 
   /**
    * Creates a tag container under the tags-root with the given name.
    *
    * @param aName
    *        the name for the new container.
    * @returns the id of the new container.
@@ -136,115 +121,149 @@ TaggingService.prototype = {
                                   this._bms.DEFAULT_INDEX);
   },
 
   /**
    * Checks whether the given uri is tagged with the given tag.
    *
    * @param [in] aURI
    *        url to check for
-   * @param [in] aTagId
-   *        id of the folder representing the tag to check
-   * @param [out] aItemId
-   *        the id of the item found under the tag container
-   * @returns true if the given uri is tagged with the given tag, false
+   * @param [in] aTagName
+   *        the tag to check for
+   * @returns the item id if the URI is tagged with the given tag, -1
    *          otherwise.
    */
-  _isURITaggedInternal: function TS__uriTagged(aURI, aTagId, aItemId) {
+  _getItemIdForTaggedURI: function TS__getItemIdForTaggedURI(aURI, aTagName) {
+    var tagId = this._getItemIdForTag(aTagName);
+    if (tagId == -1)
+      return -1;
     var bookmarkIds = this._bms.getBookmarkIdsForURI(aURI, {});
     for (var i=0; i < bookmarkIds.length; i++) {
       var parent = this._bms.getFolderIdForItem(bookmarkIds[i]);
-      if (parent == aTagId) {
-        aItemId.value = bookmarkIds[i];
-        return true;
-      }
+      if (parent == tagId)
+        return bookmarkIds[i];
     }
-    return false;
+    return -1;
+  },
+
+  /**
+   * Returns the folder id for a tag, or -1 if not found.
+   * @param [in] aTag
+   *        string tag to search for
+   * @returns integer id for the bookmark folder for the tag
+   */
+  _getItemIdForTag: function TS_getItemIdForTag(aTagName) {
+    for (var i in this._tagFolders) {
+      if (aTagName.toLowerCase() == this._tagFolders[i].toLowerCase())
+        return parseInt(i);
+    }
+    return -1;
   },
 
   // nsITaggingService
   tagURI: function TS_tagURI(aURI, aTags) {
     if (!aURI || !aTags)
       throw Cr.NS_ERROR_INVALID_ARG;
 
     for (var i=0; i < aTags.length; i++) {
-      var tagNode = this._getTagNode(aTags[i]);
-      if (!tagNode) {
-        if (typeof(aTags[i]) == "number")
+      var tag = aTags[i];
+      var tagId = null;
+      if (typeof(tag) == "number") {
+        // is it a tag folder id?
+        if (this._tagFolders[tag]) {
+          tagId = tag;
+          tag = this._tagFolders[tagId];
+        }
+        else
           throw Cr.NS_ERROR_INVALID_ARG;
-
-        var tagId = this._createTag(aTags[i]);
-        this._bms.insertBookmark(tagId, aURI, this._bms.DEFAULT_INDEX, null);
       }
       else {
-        var tagId = tagNode.itemId;
-        if (!this._isURITaggedInternal(aURI, tagNode.itemId, {}))
-          this._bms.insertBookmark(tagId, aURI, this._bms.DEFAULT_INDEX, null);
+        tagId = this._getItemIdForTag(tag);
+        if (tagId == -1)
+          tagId = this._createTag(tag);
+      }
 
-        // _getTagNode ignores case sensitivity
-        // rename the tag container so the places view would match the
-        // user-typed values
-        if (typeof(aTags[i]) == "string" && tagNode.title != aTags[i])
-          this._bms.setItemTitle(tagNode.itemId, aTags[i]);
+      var itemId = this._getItemIdForTaggedURI(aURI, tag);
+      if (itemId == -1)
+        this._bms.insertBookmark(tagId, aURI, this._bms.DEFAULT_INDEX, null);
+
+      // Rename the tag container so the Places view would match the
+      // most-recent user-typed values.
+      var currentTagTitle = this._bms.getItemTitle(tagId);
+      if (currentTagTitle != tag) {
+        this._bms.setItemTitle(tagId, tag);
+        this._tagFolders[tagId] = tag;
       }
     }
   },
 
   /**
    * Removes the tag container from the tags-root if the given tag is empty.
    *
    * @param aTagId
    *        the item-id of the tag element under the tags root
    */
   _removeTagIfEmpty: function TS__removeTagIfEmpty(aTagId) {
-    var node = this._getTagNode(aTagId).QueryInterface(Ci.nsINavHistoryContainerResultNode);
-    var wasOpen = node.containerOpen;
-    if (!wasOpen)
-      node.containerOpen = true;
+    var result = this._getTagResult(aTagId);
+    if (!result)
+      return;
+    var node = result.root;
+    node.QueryInterface(Ci.nsINavHistoryContainerResultNode);
+    node.containerOpen = true;
     var cc = node.childCount;
-    if (wasOpen)
-      node.containerOpen = false;
-    if (cc == 0) {
+    node.containerOpen = false;
+    if (cc == 0)
       this._bms.removeFolder(node.itemId);
-    }
   },
 
   // nsITaggingService
   untagURI: function TS_untagURI(aURI, aTags) {
     if (!aURI)
       throw Cr.NS_ERROR_INVALID_ARG;
 
     if (!aTags) {
       // see IDL.
       // XXXmano: write a perf-sensitive version of this code path...
       aTags = this.getTagsForURI(aURI, { });
     }
 
     for (var i=0; i < aTags.length; i++) {
-      var tagNode = this._getTagNode(aTags[i]);
-      if (tagNode) {
-        var itemId = { };
-        if (this._isURITaggedInternal(aURI, tagNode.itemId, itemId)) {
-          this._bms.removeItem(itemId.value);
-          this._removeTagIfEmpty(tagNode.itemId);
+      var tag = aTags[i];
+      var tagId = null;
+      if (typeof(tag) == "number") {
+        // is it a tag folder id?
+        if (this._tagFolders[tag]) {
+          tagId = tag;
+          tag = this._tagFolders[tagId];
+        }
+        else
+          throw Cr.NS_ERROR_INVALID_ARG;
+      }
+      else
+        tagId = this._getItemIdForTag(tag);
+
+      if (tagId != -1) {
+        var itemId = this._getItemIdForTaggedURI(aURI, tag);
+        if (itemId != -1) {
+          this._bms.removeItem(itemId);
+          this._removeTagIfEmpty(tagId);
         }
       }
-      else if (typeof(aTags[i]) == "number")
-        throw Cr.NS_ERROR_INVALID_ARG;
     }
   },
 
   // nsITaggingService
   getURIsForTag: function TS_getURIsForTag(aTag) {
     if (aTag.length == 0)
       throw Cr.NS_ERROR_INVALID_ARG;
 
     var uris = [];
-    var tagNode = this._getTagNode(aTag);
-    if (tagNode) {
+    var tagResult = this._getTagResult(aTag);
+    if (tagResult) {
+      var tagNode = tagResult.root;
       tagNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
       tagNode.containerOpen = true;
       var cc = tagNode.childCount;
       for (var i = 0; i < cc; i++) {
         try {
           uris.push(gIoService.newURI(tagNode.getChild(i).uri, null, null));
         } catch (ex) {
           // This is an invalid node, tags should only contain valid uri nodes.
@@ -258,75 +277,97 @@ TaggingService.prototype = {
 
   // nsITaggingService
   getTagsForURI: function TS_getTagsForURI(aURI, aCount) {
     if (!aURI)
       throw Cr.NS_ERROR_INVALID_ARG;
 
     var tags = [];
     var bookmarkIds = this._bms.getBookmarkIdsForURI(aURI, {});
-    var root = this._tagsResult.root;
-    var cc = root.childCount;
     for (var i=0; i < bookmarkIds.length; i++) {
-      var parent = this._bms.getFolderIdForItem(bookmarkIds[i]);
-      for (var j=0; j < cc; j++) {
-        var child = root.getChild(j);
-        if (child.itemId == parent)
-          tags.push(child.title);
-      }
+      var folderId = this._bms.getFolderIdForItem(bookmarkIds[i]);
+      if (this._tagFolders[folderId])
+        tags.push(this._tagFolders[folderId]);
     }
 
     // sort the tag list
     tags.sort();
     aCount.value = tags.length;
     return tags;
   },
 
-  // nsITaggingService
-  _allTags: null,
-  get allTags() {
-    if (!this._allTags) {
-      this._allTags = [];
-      var root = this._tagsResult.root;
+  __tagFolders: null, 
+  get _tagFolders() {
+    if (!this.__tagFolders) {
+      this.__tagFolders = [];
+      var options = this._history.getNewQueryOptions();
+      options.resultType = Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY;
+      options.expandQueries = 0;
+      var query = this._history.getNewQuery();
+      var tagsResult = this._history.executeQuery(query, options);
+      var root = tagsResult.root;
+      root.containerOpen = true;
       var cc = root.childCount;
-      for (var j=0; j < cc; j++) {
-        var child = root.getChild(j);
-        this._allTags.push(child.title);
+      for (var i=0; i < cc; i++) {
+        var child = root.getChild(i);
+        this.__tagFolders[child.itemId] = child.title;
       }
+      root.containerOpen = false;
+    }
 
-      // sort the tag list
-      this.allTags.sort();
-    }
-    return this._allTags;
+    return this.__tagFolders;
+  },
+
+  // nsITaggingService
+  get allTags() {
+    var allTags = [];
+    for (var i in this._tagFolders)
+      allTags.push(this._tagFolders[i]);
+    // sort the tag list
+    allTags.sort();
+    return allTags;
   },
 
   // nsIObserver
   observe: function TS_observe(aSubject, aTopic, aData) {
     if (aTopic == "xpcom-shutdown") {
-      this.__tagsResult.root.containerOpen = false;
-      this.__tagsResult.viewer = null;
-      this.__tagsResult = null;
-      var observerSvc = Cc[OBSS_CONTRACTID].getService(Ci.nsIObserverService);
-      observerSvc.removeObserver(this, "xpcom-shutdown");
+      this._bms.removeObserver(this);
+      this._obss.removeObserver(this, "xpcom-shutdown");
     }
   },
 
-  // nsINavHistoryResultViewer
-  // Used to invalidate the cached tag list
-  itemInserted: function() this._allTags = null,
-  itemRemoved: function() this._allTags = null,
-  itemMoved: function() {},
-  itemChanged: function() {},
-  itemReplaced: function() {},
-  containerOpened: function() {},
-  containerClosed: function() {},
-  invalidateContainer: function() this._allTags = null,
-  invalidateAll: function() this._allTags = null,
-  sortingChanged: function() {},
-  result: null
+  // boolean to indicate if we're in a batch
+  _inBatch: false,
+
+  // nsINavBookmarkObserver
+  onBeginUpdateBatch: function() {
+    this._inBatch = true;
+  },
+  onEndUpdateBatch: function() {
+    this._inBatch = false;
+  },
+  onItemAdded: function(aItemId, aFolderId, aIndex) {
+    if (aFolderId == this._bms.tagsFolder &&
+        this._bms.getItemType(aItemId) == this._bms.TYPE_FOLDER)
+      this._tagFolders[aItemId] = this._bms.getItemTitle(aItemId);
+  },
+  onItemRemoved: function(aItemId, aFolderId, aIndex){
+    if (aFolderId == this._bms.tagsFolder && this._tagFolders[aItemId])
+      delete this._tagFolders[aItemId];
+  },
+  onItemChanged: function(aItemId, aProperty, aIsAnnotationProperty, aValue){
+    if (this._tagFolders[aItemId])
+      this._tagFolders[aItemId] = this._bms.getItemTitle(aItemId);
+  },
+  onItemVisited: function(aItemId, aVisitID, time){},
+  onItemMoved: function(aItemId, aOldParent, aOldIndex, aNewParent, aNewIndex){
+    if (this._tagFolders[aItemId] && this._bms.tagFolder == aOldParent &&
+        this._bms.tagFolder != aNewParent)
+      delete this._tagFolders[aItemId];
+  }
 };
 
 // Implements nsIAutoCompleteResult
 function TagAutoCompleteResult(searchString, searchResult,
                                defaultIndex, errorDescription,
                                results, comments) {
   this._searchString = searchString;
   this._searchResult = searchResult;
--- a/toolkit/components/places/tests/unit/test_tagging.js
+++ b/toolkit/components/places/tests/unit/test_tagging.js
@@ -85,17 +85,17 @@ 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 (or even trice!) with the same tag should be a
+  // Tagging the same url twice (or even thrice!) with the same tag should be a
   // no-op
   tagssvc.tagURI(uri1, ["tag 1"]);
   do_check_eq(tag1node.childCount, 2);
   tagssvc.tagURI(uri1, [tag1itemId]);
   do_check_eq(tag1node.childCount, 2);
   do_check_eq(tagRoot.childCount, 1);
 
   // also tests bug 407575
@@ -127,9 +127,35 @@ function run_test() {
   // test untagging
   tagssvc.untagURI(uri1, ["tag 1"]);
   do_check_eq(tag1node.childCount, 1);
 
   // removing the last uri from a tag should remove the tag-container
   tagssvc.untagURI(uri2, ["tag 1"]);
   do_check_eq(tagRoot.childCount, 1);
   
+  // get array of tag folder ids => title
+  // for testing tagging with mixed folder ids and tags
+  var options = histsvc.getNewQueryOptions();
+  var query = histsvc.getNewQuery();
+  query.setFolders([bmsvc.tagsFolder], 1);
+  var result = histsvc.executeQuery(query, options);
+  var tagRoot = result.root;
+  tagRoot.containerOpen = true;
+  var tagFolders = [];
+  var child = tagRoot.getChild(0);
+  var tagId = child.itemId;
+  var tagTitle = child.title;
+
+  // test mixed id/name tagging
+  // as well as non-id numeric tags
+  var uri3 = uri("http://testuri/3");
+  tagssvc.tagURI(uri3, [tagId, "tag 3", "456"]);
+  var tags = tagssvc.getTagsForURI(uri3, {});
+  do_check_true(tags.indexOf(tagTitle) != -1);
+  do_check_true(tags.indexOf("tag 3") != -1);
+  do_check_true(tags.indexOf("456") != -1);
+
+  // test mixed id/name tagging
+  tagssvc.untagURI(uri3, [tagId, "tag 3", "456"]);
+  tags = tagssvc.getTagsForURI(uri3, {});
+  do_check_eq(tags.length, 0);
 }