RESULTS_AS_TAG_QUERY allows to be used with a QUERY_TYPE_HISTORY, in this case it adds a useless history observer and if it is a root of a treeView we try to delete tags as they would be history containers (see for example the Library right pane) also RESULTS_AS_TAG_CONTENTS allows to be used with a QUERY_TYPE_HISTORY, in this case we are doing the right thing, defining a queryType=QUERY_TYPE_BOOKMARKS, but we don't force third party users to do the same, so they could finish up with bogus tag containers (place:resultType=7&folder=X), filtered in the wrong way. I suggest forcing the queryType to QUERY_TYPE_BOOKMARKS when one of these resultType is choosen, since they are bookmarks containers.

Comment on attachment 343962 [details] [diff] [review] patch >diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js >--- a/browser/components/places/content/controller.js >+++ b/browser/components/places/content/controller.js >@@ -884,18 +884,18 @@ PlacesController.prototype = { > transactions.push(PlacesUIUtils.ptm.untagURI(uri, [tagItemId])); > continue; > } > else if (PlacesUtils.nodeIsTagQuery(node) && node.parent && > PlacesUtils.nodeIsQuery(node.parent) && > asQuery(node.parent).queryOptions.resultType == > Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) { > // Untag all URIs tagged with this tag only if the tag container is >- // child of the "Tags" query in the library, in all other places we >- // must only remove the query node. >+ // child of the "Tags" query in the Library or is in the right pane. >+ // In all other places we must only remove the query node. I don't understand this comment change - the original comment is true regardless if in the right or left pane. >+ // Always filter bookmarks queries to avoid the inclusion of query nodes, >+ // but RESULTS AS TAG QUERY does never need to be filtered. >+ if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS && >+ aOptions->ResultType() != nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY) > return PR_TRUE; grammar nit: "never needs" or "doesn't ever" how is this change related to the reported bug? one problem with this change is that we'll need to undo this change to properly fix bug 399799, to allow for complex tag queries.

(In reply to comment #1) > > // Untag all URIs tagged with this tag only if the tag container is > >- // child of the "Tags" query in the library, in all other places we > >- // must only remove the query node. > >+ // child of the "Tags" query in the Library or is in the right pane. > >+ // In all other places we must only remove the query node. > > I don't understand this comment change - the original comment is true > regardless if in the right or left pane. true > >+ // Always filter bookmarks queries to avoid the inclusion of query nodes, > >+ // but RESULTS AS TAG QUERY does never need to be filtered. > >+ if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS && > >+ aOptions->ResultType() != nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY) > > return PR_TRUE; > > grammar nit: "never needs" or "doesn't ever" > > how is this change related to the reported bug? is needed, results as tag query would now be a bookmarks query so it would pass through the bookmarks filters, but it does not need to. > one problem with this change is that we'll need to undo this change to properly fix bug 399799, to allow for complex tag queries. we can't go much complex with current query implementation, hwv why we should undo, hasTag would be an additional option, and ideally it should not require defining a resultType=6 or resultType=7 (even if probably hasTag should completely replace resultType=7, so code would have to be changed in every case)

Comment on attachment 343962 [details] [diff] [review] patch ok, r=me w/ nits fixed, thanks.

pushed to trunk http://hg.mozilla.org/mozilla-central/rev/d9c12c924970

Comment on attachment 349852 [details] [diff] [review] for checkin asking approval for 1.9.1 This is probably cause of other bugs, since we actually have a wrong observer on all tag containers. Medium risk, but unit tests are pass and manual tag tests i've done appear correct.

Comment on attachment 349852 [details] [diff] [review] for checkin a191=beltzner

pushed to 1.9.1 branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9a9a5a31d80a

Verified fixed on trunk and 1.9.1 branch based on given changesets.

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