399799 - Provide better options to query for tags

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

a hasTag option, similar to our hasAnnotation option, would be very useful.

Comment 1

[Asaf Romano (gone)]

What's the use case for this for which isBookmarked isn't as suitable?

Comment 2

[Dietrich Ayala (:dietrich)]

how does isBookmarked help at all in this case?

the use-case i'm talking about is if you want to query for all URIs tagged with "foo" for example, and easily use that in our places views, or save a search for it. we're going to need to add "has tag" as an option in the query builder, and this would make that implementation cleaner.

currently it requires knowledge of our backend tag implementation, which you then have to work around. also, if you wanted to save that query, it'd basically be like hard-coding an id that isn't guaranteed to remain static.

however, being able to specify a tag in the query allows callers to access tag data in the way they conceptualize it (a property of a uri), instead of having to think of a tag as a bookmark folder, which is counter-intuitive.

Comment 3

[Asaf Romano (gone)]

Sold (somehow I read this as "isTagged").

Comment 4

[Dietrich Ayala (:dietrich)]

This will not block the 3.1 release. Would definitely take a patch though.

Comment 5

[Dietrich Ayala (:dietrich)]

This would be awesome for extension developers who use tags. Would also allow smart folders for tags. Marking wanted because it's really wanted. Bad.

Comment 6

[Drew Willcoxon :adw]

I've taken a look at the code and think I can take this on, but I want to be clear on what we're doing.  hasTag?  hasTags?  hasAllTags?  hasSomeTags?

hasTag (only one tag): not so useful.
hasTags (more than one tag): better, but what does this mean?  Has all tags specified or only some?
hasAllTags: useful.
hasSomeTags: also useful.

We could be fancy and do what TagSifter :) does -- arbitrary boolean arithmetic on tags, e.g., (tag1 AND tag2) OR ((NOT tag3) AND tag4).

But since multiple queries are OR'ed together, maybe it would be best to go with hasAllTags.  That way you could have fancy arithmetic; it would not be pushed to one query but spread across multiple queries.  On the other hand, hasAllTags would make a simple search like (tag1 OR tag2 OR tag3) kind of a pain.  You'd have to make three identical queries with different hasAllTags options.  (On the other other hand, hasSomeTags would make (tag1 AND tag2 AND tag3) impossible, which is a useful search.)

Thoughts?

Comment 7

[Marco Bonardo [:mak]]

i think:
hasTag=mozilla (has tag mozilla)
hasTag=mozilla&hasTag=firefox (has both mozilla and firefox)

Comment 8

[Igor Velkov]

(hasTag=mozilla|hasTag=mail)&~hasTag=thunderbird (has mozillla or mail, but not thunderbird)

Comment 9

[Drew Willcoxon :adw]

The more I think about this the more it seems like any kind of simple hasTags flag is too... simple.  It would be nice to be able to do a query like ((tag1 OR tag2) AND (tag3 OR tag4)), or even just ((NOT tag1) AND tag2).  Right?  How would you do either of those with only a list of tags to search for, regardless of whether all tags in the list must be present or only some?

Do we even want to support such arbitrary queries?  I would argue yes; IMHO tagging is handicapped and no different from general search terms unless you can combine tags in such a precise and ad hoc way.  And as an extension developer it would have been very useful for me.

This points to a tag query API apart from the current history query API, an API that would let you build up a tag query with operations like intersection, union, and complement.  Or more simply a set of related methods added to the current API.

Serialization could be done in postfix, e.g., ((tag1 OR tag2) AND (tag3 OR tag4)) would serialize to something like "tagQuery=#101#102|#103#104|&", where # marks a tag, tags are written as their moz_bookmarks IDs, and | and & are operators.

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> Serialization could be done in postfix, e.g., ((tag1 OR tag2) AND (tag3 OR
> tag4)) would serialize to something like "tagQuery=#101#102|#103#104|&", where
> # marks a tag, tags are written as their moz_bookmarks IDs, and | and & are
> operators.

only a minor note, we clearly need a team talk on all of this. But for sure we want "hasTag" also to abstract the backend implementation, that we want to move away from moz_bookmarks ids. A tag will in future probably be recognized by itself in the table schema too.

Comment 11

[Dietrich Ayala (:dietrich)]

this doesn't need to be wizard class. it needs to be simple and usable for common cases. i think inclusion and exclusion options are required. everything else can be done via post-query filtering in extension-land. trying to push every permutation into the back-end complicates the apis for everyone, to the benefit of very few. i'm fine with those few doing a little bit of legwork. 

i like hasTags(). however, it's negation is difficult.

maybe tagged() and nottagged() ?

Comment 12

[Drew Willcoxon :adw]

Attachment: WIP 1

Adds getTags(), setTags(), and tagCount to nsINavHistoryQuery.  I followed the lead of getFolders() et al. in declaring the new methods and attribute.  Thoughts on these additions are welcome.  The patch includes a test, so you can see how they're being used.  A history item with *any* of the tags passed to setTags() will match.  I still need to add a way to specify "not these tags."  I'm thinking it will be a boolean attribute "tagsAreNot" (similar to annotationIsNot).  Also, I'm not sure if there's a separate path for hooking up tags to bookmark queries, so I'll check that out.

This is the most XPCOM programming I've done so far, so critiques on the code at this stage are appreciated.

Comment 14

[Drew Willcoxon :adw]

Attachment: for review v1

I think I've updated every place I need to.

I'm not so wild about the names of the new methods and attribute in the IDL, but I think it's more important to be consistent with the rest of the API (setFolders(), annotationIsNot, etc.).

Query URIs look like this (again trying to be consistent):

1. place:tag=foo&tag=bar
2. place:tag=baz&tag=qux&!tags=1

1 says "get all items that are tagged with either foo or bar."
2 says "get all items that are not tagged with any of baz and qux."

Comment 15

[Marco Bonardo [:mak]]

(In reply to comment #14)
> 1 says "get all items that are tagged with either foo or bar."

I have a doubt about this.
For folders the action is clearly an OR because makes no sense asking "give me all bookmarks that are in both folder A and B". In this case is not so straight forward.

Talking about tags functionality we add, what is more useful?
"give me all bookmarks tagged with tag1 OR tag2"
"give me all bookmarks tagged with tag1 AND tag2"

I think the default action we implement should be the most common action our API users will look for, in this case i would expect a filter action.
I think is more common to ask for a bookmark that has 2 tags (for example "bug" and "crash") rather than asking to merge all bookmarks from 2 tags (give me all "bug" and all "crash").
And we already provide a way to merge (or) results from 2 queries while we don't provide a way to AND them:
"You can pass one or more nsINavHistoryQuery objects to executeQueries(). Within one query object, all parameters are ANDed together. The conditions for different query objects are then ORed together."
So, also looking at documentation, original implementation points out that all parameters of a query are ANDed.

I think we should discuss this later in #places with Dietrich.

Comment 16

[Drew Willcoxon :adw]

Attachment: for review v2

As we discussed on IRC, the semantics of setTags() is now "get all items that are tagged with all given tags."  Beefed up the test to check multiple ORed queries.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 371793 [details] [diff] [review]
for review v2

>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl

you need to change the nsINavHistoryQuery uuid

>   /**
>+   * Limit results to items that are tagged with all of the given tags.  To
>+   * search for items that are tagged with any given tags, multiple queries
>+   * may be passed to nsINavHistoryService.executeQueries(). If 'tagsAreNot'
>+   * is true, the results are instead limited to items that are not tagged
>+   * with any of the given tags.
>+   */

Move each sentence to a new line, this is too much crowded and referring to 3 different situations. When writing mdc docs we could also add some bool logic example to better clarify how this works.

>+  void setTags([const,array, size_is(count)] in wstring tags,
>+               in unsigned long count);
>+  void getTags(out unsigned long count,
>+               [retval,array,size_is(count)] out wstring tags);
>+  readonly attribute unsigned long tagCount;
>+  attribute boolean tagsAreNot;

according to this page
http://benjamin.smedbergs.us/blog/2008-09-26/allocated-memory-and-shared-library-boundaries
wstring in idl is obsolete and we should use AString.
Unluckily you can't pass in an array of those. wstring works but is easy to leak it.
you can also look at http://weblogs.mozillazine.org/weirdal/archives/019761.html
In this case i can't see a better solution that using wstring though, maybe you could ask some more xpcom experienced dev.
In tagging service we in an nsIVariant and out a wstring array, looks like we have no convention.

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>@@ -5885,16 +5891,42 @@ nsNavHistory::QueryToSelectClause(nsNavH
>          "JOIN moz_anno_attributes annoname "
>            "ON anno.anno_attribute_id = annoname.id "
>          "WHERE anno.place_id = h.id "
>            "AND annoname.name = ").Param(":anno").Str(")");
>     // annotation-based queries don't get the common conditions, so you get
>     // all URLs with that annotation
>   }
> 
>+  // tags
>+  const nsTArray<nsString> &tags = aQuery->Tags();
>+  if (tags.Length() > 0) {
>+    clause.Condition("h.id");
>+    if (aQuery->TagsAreNot())
>+      clause.Str("NOT");
>+    clause.Str(
>+      "IN "
>+        "(SELECT bms.fk "
>+         "FROM moz_bookmarks bms "
>+         "JOIN moz_bookmarks tags ON bms.parent = tags.id "
>+         "WHERE tags.parent =").
>+           Param(":tags_folder").
>+           Str("AND tags.title IN (");
>+    for (PRUint32 i = 0; i < tags.Length(); ++i) {
>+      nsPrintfCString param(":tag%d_", i);

did you mean :tag_%d?

>+      clause.Param(param.get());
>+      if (i < tags.Length() - 1)
>+        clause.Str(",");
>+    }
>+    clause.Str(")");
>+    if (!aQuery->TagsAreNot())
>+      clause.Str("GROUP BY bms.fk HAVING count(*) >=").Param(":tag_count");

awesome! but why >=, should not this be a simple =?

>@@ -6021,16 +6053,37 @@ nsNavHistory::BindQueryClauseParameters(
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+    nsNavBookmarks *bookmarks = nsNavBookmarks::GetBookmarksService();
>+    NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>+    PRInt64 tagsFolder;
>+    bookmarks->GetTagsFolder(&tagsFolder);

you can directly use PRInt64 tagsFolder = GetTagsFolder() helper (nsNavHistory::GetTagsFolder())

>diff --git a/toolkit/components/places/src/nsNavHistoryQuery.cpp b/toolkit/components/places/src/nsNavHistoryQuery.cpp

>@@ -360,16 +363,17 @@ nsNavHistory::QueriesToQueryString(nsINa
>                                    nsACString& aQueryString)
> {
>   nsCOMPtr<nsNavHistoryQueryOptions> options = do_QueryInterface(aOptions);
>   NS_ENSURE_TRUE(options, NS_ERROR_INVALID_ARG);
> 
>   nsCAutoString queryString;
>   for (PRUint32 queryIndex = 0; queryIndex < aQueryCount;  queryIndex ++) {
>     nsINavHistoryQuery* query = ;
>+    nsCOMPtr<nsNavHistoryQuery> queryPtr = do_QueryInterface(query);

there should be nothing bad in directly doing
nsCOMPtr<nsNavHistoryQuery> query = do_QueryInterface(aQueries[queryIndex]);
and using query below

>@@ -1108,16 +1153,86 @@ NS_IMETHODIMP nsNavHistoryQuery::SetAnno
>   return NS_OK;
> }
> NS_IMETHODIMP nsNavHistoryQuery::GetHasAnnotation(PRBool* aHasIt)
> {
>   *aHasIt = ! mAnnotation.IsEmpty();
>   return NS_OK;
> }
> 
>+/* void getTags (out unsigned long count,
>+                 [array, size_is (count), retval] out wstring tags); */
>+NS_IMETHODIMP nsNavHistoryQuery::GetTags(PRUint32 *aCount, PRUnichar ***aTags)
>+{
>+  PRUint32 count = mTags.Length();
>+  PRUnichar **tags = nsnull;
>+  if (count > 0) {
>+    tags = static_cast<PRUnichar**>(nsMemory::Alloc(count * sizeof(PRUnichar*)));

NS_Alloc(size)

>+    if (!tags) {
>+      *aCount = 0;
>+      *aTags = nsnull;
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    for (PRUint32 i = 0; i < count; ++i) {
>+      tags[i] = ToNewUnicode(mTags[i]);
>+      if (!tags[i]) {
>+        for (PRUint32 j = 0; j < i; ++j) {
>+          nsMemory::Free(tags[j]);

NS_Free(tags[j])

>+        }
>+        nsMemory::Free(tags);
>+        *aCount = 0;
>+        *aTags = nsnull;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+    }
>+  }
>+  *aCount = count;
>+  *aTags = tags;
>+  return NS_OK;
>+}

the problem with these wstring implementation is that this alloc space does not have clear life bounds defined.

r+ valid as a first-pass, passing this to dietrich since i can't approve interface changes. Btw i think we need some hint from an experienced xpcom dev to evaluate these array of wstrings.

Test is quite big, so i'll review that apart later.

Comment 18

[Drew Willcoxon :adw]

(In reply to comment #17)
> Move each sentence to a new line, this is too much crowded and referring to 3
> different situations. When writing mdc docs we could also add some bool logic
> example to better clarify how this works.

OK.  FWIW, some existing comments in the IDL are like this.

> according to this page
> http://benjamin.smedbergs.us/blog/2008-09-26/allocated-memory-and-shared-library-boundaries

Cool, thanks for linking to those.  Would be nice if the MDC strings page mentioned these things.

> In tagging service we in an nsIVariant and out a wstring array, looks like we
> have no convention.

No convention at all.

> did you mean :tag_%d?

The reason the underscore is needed is to separate tag indices from query indices.  When you OR multiple queries together, the IndexGetter helper class used to construct the SQL appends a query index to each parameter name for query indices > 0.  In the :tag%d_ case, without the underscore, the tag index would run into the query index.  Then we couldn't be sure whether :tag11 means "tag index 11 in query 0" or "tag index 1 in query 1," and as a result the wrong arguments are bound to the wrong parameters.  It's a subtle problem I ran into in the test.

> awesome! but why >=, should not this be a simple =?

Because there can be multiple tags with the same name (which is a whole other sucky problem).  One of the tests actually checks this.

> the problem with these wstring implementation is that this alloc space does not
> have clear life bounds defined.

For both GetTags() and SetTags(), the caller owns them, right?  Do you mean wstrings in general or in this case?

> interface changes. Btw i think we need some hint from an experienced xpcom dev
> to evaluate these array of wstrings.

Do you know anyone in particular?  I will take a look at those pages you linked to.

Comment 19

[Marco Bonardo [:mak]]

About life limits i meant wstrings in general when returned by our function that allocs them.

Well i think Neil Rashbrook (NeilAway) has a good experience with both xpcom and places code, or you could try with benjamin, if he has some time to share.
I'm not here from a long time so it's hard for me to point you to the right persons.

Comment 20

[Shawn Wilsher :sdwilsh]

Pretty sure you want to use NS_Alloc and NS_Free here.  The caller of this method will own the strings, so you do not have to worry about them.

Comment 21

[Drew Willcoxon :adw]

Attachment: for review v3

Now with attribute nsIVariant tags.  Addressed Marco's comments where still applicable.

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 373247 [details] [diff] [review]
for review v3

>diff --git a/toolkit/components/places/tests/queries/test_tags.js b/toolkit/components/places/tests/queries/test_tags.js

>+function doWithBookmark(aTags, aCallback) {
>+  var pURI = uri("http://example.com/");

could you please move the uri to a const and use that everywhere?

>+function doWithVisit(aTags, aCallback) {
>+  var pURI = uri("http://example.com/");

same here and all around.

>+/**
>+ * Ensures that the arrays contain the same elements.
>+ */
>+function setsAreEqual(aArr1, aArr2) {
>+  do_check_eq(aArr1.length, aArr2.length);
>+  aArr1.forEach(function (u) do_check_true(aArr2.indexOf(u) >= 0));
>+  aArr2.forEach(function (u) do_check_true(aArr1.indexOf(u) >= 0));

would make sense to compare the position too if we know how tags are returned (alphabetically and case insensitive, i suppose)?

r=me as first-pass to the tests

Comment 23

[Drew Willcoxon :adw]

(In reply to comment #22)
> would make sense to compare the position too if we know how tags are returned
> (alphabetically and case insensitive, i suppose)?

That's a good point.  Is the order in which tags are returned important?  Right now it's the order the caller added them in.  Alphabetically/lexicographically is the most obvious, but what about non-ascii tags?  Are there XPCOM utilities for string sorting?

Comment 24

[Drew Willcoxon :adw]

Attachment: for review v3.1

Small changes: addresses comment 22 and sorts the tags array when it's set.

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 373681 [details] [diff] [review]
for review v3.1

some comments, not done yet.

>@@ -846,16 +847,43 @@ interface nsINavHistoryQuery : nsISuppor
>    * annotation. This will only work for RESULTS_AS_URI since there will be
>    * no visits for these items.
>    */
>   attribute boolean annotationIsNot;
>   attribute AUTF8String annotation;
>   readonly attribute boolean hasAnnotation;
> 
>   /**
>+   * Limit results to items that are tagged with all of the given tags.  This
>+   * attribute should be set to an array of strings.  When called as a

"should" is not ok here. it *is* a string array.

>+   * getter it will return an array of strings sorted ascending in
>+   * lexicographical order.  The array may be empty in either case.  Duplicate
>+   * tags may be specified when setting the attribute, but on get only unique
>+   * tags will be returned.

"but on get" is awkward. maybe "the getter returns only unique tags" or something to that effect.

>+   *
>+   * To search for items that are tagged with any given tags rather than all,
>+   * multiple queries may be passed to nsINavHistoryService.executeQueries().
>+   */
>+  attribute nsIVariant tags;
>+
>+  /**
>+   * The number of unique tags contained in the 'tags' attribute.  If you
>+   * need only the number of tags and not the tags themselves, this attribute
>+   * is preferred over using the 'tags' attribute since it is more efficient.
>+   */
>+  readonly attribute unsigned long tagCount;

what's the use case for this?

>@@ -6037,16 +6069,34 @@ nsNavHistory::BindQueryClauseParameters(
> 
>   // annotation
>   if (NS_SUCCEEDED(aQuery->GetHasAnnotation(&hasIt)) && hasIt) {
>     rv = statement->BindUTF8StringParameter(index.For(":anno"), 
>                                             aQuery->Annotation());
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>+  // tags
>+  const nsTArray<nsString> &tags = aQuery->Tags();
>+  if (tags.Length() > 0) {
>+    for (PRUint32 i = 0; i < tags.Length(); ++i) {
>+      nsPrintfCString param(":tag%d_", i);
>+      nsCString tag = NS_ConvertUTF16toUTF8(tags[i]);
>+      rv = statement->BindUTF8StringParameter(index.For(param.get()), tag);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }

should be able to use nsCAutoString instead of nsCString here

https://developer.mozilla.org/en/String_Quick_Reference#Stack-based_strings

Comment 26

[Shawn Wilsher :sdwilsh]

Actually, NS_ConvertUTF16toUTF8 is a class in itself, so there's not need to store it in yet another string (it's inefficient).  You really just want
NS_ConvertUTF16toUTF8 tag(tags[i]);

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 373681 [details] [diff] [review]
for review v3.1

>+  // Convert the nsIVariant to an array.  We own the resulting buffer and its
>+  // elements.
>+  nsresult rv = aTags->GetAsArray(&eltType, &eltIID, &arrayLen, &array);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // If element type is not wstring, thanks a lot.  Your memory die now.
>+  if (eltType != nsIDataType::VTYPE_WCHAR_STR) {
>+    switch (eltType) {
>+    case nsIDataType::VTYPE_ID:
>+    case nsIDataType::VTYPE_CHAR_STR:
>+      {
>+        char **charArray = reinterpret_cast<char **>(array);
>+        for (PRUint32 i = 0; i < arrayLen; ++i) {
>+          if (charArray[i])
>+            nsMemory::Free(charArray[i]);
>+        }
>+      }
>+      break;
>+    case nsIDataType::VTYPE_INTERFACE:
>+    case nsIDataType::VTYPE_INTERFACE_IS:
>+      {
>+        nsISupports **supportsArray = reinterpret_cast<nsISupports **>(array);
>+        for (PRUint32 i = 0; i < arrayLen; ++i) {
>+          NS_IF_RELEASE(supportsArray[i]);
>+        }
>+      }
>+      break;
>+    // The other types are primitives that do not need to be freed.
>+    }
>+    nsMemory::Free(array);
>+    return NS_ERROR_ILLEGAL_VALUE;
>+  }

reading this block made me gag.

>+  PRUnichar **tags = reinterpret_cast<PRUnichar **>(array);
>+  mTags.Clear();
>+
>+  // Finally, add each passed-in tag to our mTags array and then sort it.
>+  for (PRUint32 i = 0; i < arrayLen; ++i) {
>+
>+    // Don't allow nulls.
>+    if (!tags[i]) {
>+      nsMemory::Free(tags);
>+      return NS_ERROR_ILLEGAL_VALUE;
>+    }

why returning early? why not keep the non-null tags and move on?

>+
>+    nsDependentString tag(tags[i]);
>+
>+    // Don't store duplicate tags.  This isn't just to save memory or to be
>+    // fancy; the SQL that's built from the tags relies on no dupes.
>+    if (!mTags.Contains(tag) && !mTags.AppendElement(tag)) {
>+      nsMemory::Free(tags[i]);
>+      nsMemory::Free(tags);
>+      return NS_ERROR_OUT_OF_MEMORY;

ditto

Comment 28

[Drew Willcoxon :adw]

(In reply to comment #25)
> >+  readonly attribute unsigned long tagCount;
> 
> what's the use case for this?

It's a holdover from an earlier patch when I was doing get/setTags() to be consistent with get/setFolders() and folderCount.  So the answer is, no good use case that I can think of -- it should go.

(In reply to comment #27)
> reading this block made me gag.

Heh.  If you want to make hasTag as easy as possible to use from JS, nsIVariant is the best way I know of, and if I'm using nsIVariant, I need code like that to prevent leaks.  Ideally it would be abstracted into some nsIVariant utils somewhere.  If you're thinking it's unlikely that a caller would pass in an nsISupports or something else that requires being freed, I agree, but IMO that's not a good reason to allow leaks, and I think a dozen lines of pretty readable code is hardly going out of our way to check those cases.

> why returning early? why not keep the non-null tags and move on?

I didn't view this case any differently from passing in a mixture of tags and numbers or tags and any other kind of non-tags.  In those cases I return illegal value.  If we filter out nulls, should we filter out other non-tags, too?  Or do you think callers will often end up passing in nulls but not other non-tags?

> >+    // Don't store duplicate tags.  This isn't just to save memory or to be
> >+    // fancy; the SQL that's built from the tags relies on no dupes.
> >+    if (!mTags.Contains(tag) && !mTags.AppendElement(tag)) {
> >+      nsMemory::Free(tags[i]);
> >+      nsMemory::Free(tags);
> >+      return NS_ERROR_OUT_OF_MEMORY;
> 
> ditto

Here I'm actually filtering out dupes and returning only if there's a low-level error appending a tag to the list.  I should make this more readable by breaking out a separate nested conditional for AppendElement().

Comment 29

[Dietrich Ayala (:dietrich)]

Comment on attachment 373681 [details] [diff] [review]
for review v3.1

looks good, aside from the few questions posed above. once those are resolved, this should be ready to go.

Comment 30

[Dietrich Ayala (:dietrich)]

(In reply to comment #28)
> 
> > why returning early? why not keep the non-null tags and move on?
> 
> I didn't view this case any differently from passing in a mixture of tags and
> numbers or tags and any other kind of non-tags.  In those cases I return
> illegal value.  If we filter out nulls, should we filter out other non-tags,
> too?  Or do you think callers will often end up passing in nulls but not other
> non-tags?

hrm, let's leave it as-is.

> > >+    // Don't store duplicate tags.  This isn't just to save memory or to be
> > >+    // fancy; the SQL that's built from the tags relies on no dupes.
> > >+    if (!mTags.Contains(tag) && !mTags.AppendElement(tag)) {
> > >+      nsMemory::Free(tags[i]);
> > >+      nsMemory::Free(tags);
> > >+      return NS_ERROR_OUT_OF_MEMORY;
> > 
> > ditto
> 
> Here I'm actually filtering out dupes and returning only if there's a low-level
> error appending a tag to the list.  I should make this more readable by
> breaking out a separate nested conditional for AppendElement().

ok nm, was my oversight.

Comment 31

[Shawn Wilsher :sdwilsh]

Use NS_Free please instead of nsMemory::Free

Comment 32

[Drew Willcoxon :adw]

(In reply to comment #31)
> Use NS_Free please instead of nsMemory::Free

You told me, I remember, will do. :)

Comment 33

[Drew Willcoxon :adw]

Attachment: for checkin

Addressed the various comments above.

Comment 34

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/bf9632b84142

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on trunk by check-in in mozilla-central. Tests are included too so marking in-testsuite flag.

Dietrich, still possible to get this into 1.9.1? Or is it too late for?

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> Dietrich, still possible to get this into 1.9.1? Or is it too late for?
This is an API change, so it won't be able to land on 1.9.1.

Comment 37

[Eric Shepherd [:sheppy]]

Documentation is here: https://developer.mozilla.org/en/nsINavHistoryQuery#Attributes

Comment 38

[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