383803 - Places Tagging Back-end (nsITaggingService)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Asaf Romano (gone)]

Comment 1

[Asaf Romano (gone)]

Attachment: v1

TBD: tests

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 267785 [details] [diff] [review]
v1


>+  /**
>+   * Retrieves all URLs tagged with the given tag.
>+   *
>+   * @param aTag
>+   *        Array of tags to unset.
>+   */
>+  nsISimpleEnumerator getURIsForTag(in AString aTag);

nit: fix param comment

also, was there a conclusion in the irc discussion about whether there's a more js-consumer-friendly way of doing this?


>@@ -3322,19 +3322,31 @@ nsNavHistory::QueryToSelectClause(nsNavH
>   if (aQuery->MaxVisits() >= 0) {
>     if (! aClause->IsEmpty())
>       *aClause += NS_LITERAL_CSTRING(" AND ");
>     parameterString(aStartParameter + *aParamCount, paramString);
>     *aClause += NS_LITERAL_CSTRING("h.visit_count <= ") + paramString;
>     (*aParamCount) ++;
>   }
> 
>-  // only bookmarked, has no affect on bookmarks-only queries
>-  if (aOptions->QueryType() != nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS &&
>-      aQuery->OnlyBookmarked()) {
>+  
>+  if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) {
>+    // Folders has only affect on bookmark queries

nit: s/has only/only have an/

>+
>+
>+/**
>+ * The Places Tagging Service
>+ */
>+function TaggingService() {
>+  dump("@In New TaggingService\n");

remove before checkin? (for the others also)

>+
>+  /**
>+   * If there's no tag with the given name, -1 is returned.
>+   */
>+  _getTagIndex: function TS__getTagIndex(aName) {
>+    for (var i=0; i < this._tags.length; i++) {
>+      if (this._tags[i].name == aName)
>+        return i;
>+    }
>+
>+    return -1;
>+  },

check aName.length?


>+  _createTag: function TS__createTag(aName) {
>+    var id = this._bms.createFolder(this._bms.tagRoot, aName,
>+                                    this._bms.DEFAULT_INDEX);
>+    this._tags.push({ itemId: id, name: aName});
>+
>+    // Set the favicon
>+    var faviconService = Cc[FAV_CONTRACTID].getService(Ci.nsIFaviconService);
>+    var uri = this._bms.getFolderURI(id);
>+    faviconService.setFaviconUrlForPage(uri, this._tagContainerIcon);
>+  
>+    return id;
>+  },

should check aName.length?

>+
>+  _isURITaggedInternal: function TS__uriTagged(aURI, aTagId, aItemId) {
>+    var options = this._history.getNewQueryOptions();
>+    options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
>+    var query = this._history.getNewQuery();
>+    query.setFolders([aTagId], 1);
>+    query.uri = aURI;
>+    var result = this._history.executeQuery(query, options);
>+    var rootNode = result.root;
>+    rootNode.containerOpen = true;
>+    if (rootNode.childCount != 0) {
>+      aItemId.value = rootNode.getChild(0).itemId;
>+      return true;
>+    }
>+    return false;
>+  },

should check that aURI is an nsIURI

can you please jsdoc the private methods, and label each of the interface methods as such?

also, trying to make this a boolean return w/ the aItemId object parameter seems awkward. maybe simpler as _getIdForTaggedURI that returns the id or -1 (also consistent w/ _getTagIndex). If nothing else, aItemId should be named and/or documented better.

>+
>+  // nsITaggingService
>+  tagURI: function TS_tagURI(aURI, aTags, aCount) {
>+    dump(aTags + "\n" + aTags[0] + "\n");
>+    for (var i=0; i < aTags.length; i++) {
>+      var tagIndex = this._getTagIndex(aTags[i], true);
>+      if (tagIndex == -1) {
>+        var tagId = this._createTag(aTags[i]);
>+        this._bms.insertBookmark(tagId, aURI, this._bms.DEFAULT_INDEX, "");
>+      }
>+      else {
>+        var tagId = this._tags[tagIndex].itemId;
>+        if (!this._isURITaggedInternal(aURI, tagId, {}))
>+          this._bms.insertBookmark(tagId, aURI, this._bms.DEFAULT_INDEX, "");
>+      }
>+    }
>+  },

should check aURI is nsIURI, and also for the other apis following that also take a uri parameter.


a couple of other things:

- the tagging service should be added to the packages-static files.

- do we need to add nsITaggingObserver? we might be able to rig it through the bookmarks observer, but it might be better to have tag-specific notifications (clearer api, less noise for consumers that are tag- or bookmark-specific).

- in addition to the tests for the tagging service, please also add tests for the onlyBookmarked/QUERY_TYPE_BOOKMARKS changes.

- file a followup bug for supporting multiple folders in bookmarks queries.

r=me given these changes and follow-ups, etc.

Comment 3

[Asaf Romano (gone)]

Attachment: v1.1, with tests

Comment 4

[Asaf Romano (gone)]

Attachment: packages-static

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #3)
> Created an attachment (id=267915) [details]
> v1.1, with tests
> 

(In reply to comment #4)
> Created an attachment (id=267917) [details]
> packages-static
> 

i think you attached the wrong files.

Comment 6

[Asaf Romano (gone)]

Attachment: v1.1, with tests & installer changes

Comment 7

[Asaf Romano (gone)]

Attachment: v1.1, with tests & installer changes

*gah*

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 268000 [details] [diff] [review]
v1.1, with tests & installer changes


>Index: browser/installer/windows/packages-static
>===================================================================
>RCS file: /cvsroot/mozilla/browser/installer/windows/packages-static,v
>retrieving revision 1.111
>diff -u -p -8 -r1.111 packages-static
>--- browser/installer/windows/packages-static	23 May 2007 03:29:47 -0000	1.111
>+++ browser/installer/windows/packages-static	11 Jun 2007 19:15:52 -0000
>@@ -221,16 +221,17 @@ bin\components\nsSessionStartup.js
> bin\components\nsSessionStore.js
> bin\components\sessionstore.xpt
> bin\components\nsURLFormatter.js
> bin\components\urlformatter.xpt
> bin\components\browserdirprovider.dll
> bin\components\brwsrcmp.dll
> bin\components\txEXSLTRegExFunctions.js
> bin\components\nsLivemarkService.js
>+bin/components\nsTaggingService.js
> bin\components\nsDefaultCLH.js
> 

please fix the slashes here.

Also, is there a reason why we can't use the nsIVariant pattern for getTagsForURI as well? It would make the API more consistent, and would be one less instance where XPConnect's eccentricities are exposed to extension developers.

r=me, either way.

Comment 9

[Asaf Romano (gone)]

Attachment: for checkin

Comment 10

[Asaf Romano (gone)]

mozilla/toolkit/components/places/public/Makefile.in 1.9
mozilla/toolkit/components/places/public/nsITaggingService.id initial revision: 1.1
mozilla/toolkit/components/places/src/Makefile.in 1.25
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.131
mozilla/toolkit/components/places/src/nsNavHistory.h 1.81
mozilla/toolkit/components/places/src/nsTaggingService.js initial revision: 1.1
mozilla/toolkit/components/places/tests/unit/test_tagging.js initial revision: 1.1
mozilla/toolkit/themes/pinstripe/mozapps/jar.mn 1.19
mozilla/toolkit/themes/pinstripe/mozapps/places/tagContainerIcon.png initial revision: 1.1
mozilla/toolkit/themes/winstripe/mozapps/jar.mn 1.17
mozilla/toolkit/themes/winstripe/mozapps/places/tagContainerIcon.png initial revision: 1.1
mozilla/browser/installer/unix/packages-static 1.101
mozilla/browser/installer/windows/packages-static 1.113

Comment 11

[Asaf Romano (gone)]

Filed:
 * Bug 384226 - comprehensive tests for complex places queries.
 * Bug 384227 - Support tagging observers.
 * Bug 384228 - multiple-folders in complex queries are ignored.

Comment 12

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv