Closed Bug 383803 Opened 12 years ago Closed 12 years ago

Places Tagging Back-end (nsITaggingService)

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3 alpha6

People

(Reporter: mano, Assigned: mano)

Details

Attachments

(1 file, 5 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
TBD: tests
Attachment #267785 - Flags: review?(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.
Attachment #267785 - Flags: review?(dietrich) → review+
Attached patch v1.1, with tests (obsolete) — Splinter Review
Attachment #267785 - Attachment is obsolete: true
Attachment #267915 - Flags: review?(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.
Attachment #267915 - Attachment is obsolete: true
Attachment #267917 - Attachment is obsolete: true
Attachment #267998 - Flags: review?(dietrich)
Attachment #267915 - Flags: review?(dietrich)
*gah*
Attachment #267998 - Attachment is obsolete: true
Attachment #268000 - Flags: review?(dietrich)
Attachment #267998 - Flags: review?(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.
Attachment #268000 - Flags: review?(dietrich) → review+
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
Filed:
 * Bug 384226 - comprehensive tests for complex places queries.
 * Bug 384227 - Support tagging observers.
 * Bug 384228 - multiple-folders in complex queries are ignored.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.