419731 - Changing bookmark title in places should be reflected in tag containers

Status: VERIFIED FIXED

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

Description

[Recall]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022612 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022612 Minefield/3.0b4pre

I am very anal, and like to keep my bookmarks in folders, with custom titles as well as tagging them.

Currently if I make a custom title in places for a bookmark, it saves. However the tag does not retain this custom title. So it means I have to rename the bookmark in two different places.

I would like the names to be consistent, so if I name in places, it keeps the same title for tags.

Reproducible: Always

Steps to Reproduce:
1.Open places
2.Rename a bookmark and tag

Actual Results:  
Title is changed for bookmark, but not on the tagged bookmark.

Expected Results:  
Renaming a normal bookmark, should also rename the tag.

Comment 1

[Damian Shaw [Quan]]

Can confirm this, doesn't even work when restarting Firefox...

By "tagged bookmarks" I assume you mean tags that show up in queries under the default Smart bookmarks folder?

Comment 2

[Recall]

Yep, that is what I mean. Also they do not rename in places library under the tags section. Thanks for confirming.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Mano, is this because of the way we use tagcontainers? I think we should always be referencing through the URI item to get the place title.

Comment 4

[Marco Bonardo [:mak]]

tag containers are actually folders, so the bookmark is duplicated into it and is a different one from the original. tags should be mantained in a separate table with a cross reference one. but since we are late in the dev cycle we could mantain titles in sync, i suggest changing nsNavBookmarks::SetItemTitle so that it will not only update a single bookmark based on the id, but it will update all bookmarks which point to the same place_id.

I dont' think that a user could want to rename differently the same bookmark having it into different places (like toolbar and menu). If we allow them to have different titles for toolbar and menu we cannot choose what title to use into tags, so sync appear the only possible way.

if that's what is wanted i can fix this.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch

this implements comment #4 behaviour

Comment 6

[Asaf Romano (gone)]

No, this isn't something we could do at this point. There were reasons we chose to revert the url-centric model from Places.

Comment 7

[Marco Bonardo [:mak]]

the only usecase i see is if a user wants to have bookmarks without title on the toolbar and with title in other places. in that case we cannot sync. Still in that case how can we choose what title we should show in tags?

Comment 8

[Asaf Romano (gone)]

Then you didn't follow old places bugs...  there were *a lot* of issues with the one-title-concept, esp. for import from Fx2. See also http://wiki.mozilla.org/Places:BookmarksComments

Anyway, this is a no-go at this point.

Comment 9

[Marco Bonardo [:mak]]

you're right, we should update only the tag for the bookmark that has generated it, but since tag creation is url-centric (we don't have a reference to the bookmark that has created the tag, instead the tag is directly linked to all bookmarks with the same url, and that is mainly because a tag is a folder now) we cannot do that (the minimum thing would be update only tag title with the latest changed bookmark title if that's not empty, but that appear as a bad workaround rather than a solution).

Comment 10

[Asaf Romano (gone)]

But, tagged items has a null title, _not_ a static title (though you could get into that state).

We should change the code which selects the history title for this case, to select the title for the most recent bookmark (which has non-null title).

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch

this appear to be enough for the Library to show the correct title. Clearly this works until tags bookmarks have null titles, detecting real tags children could be slow (will require at least an addition join on moz_bookmarks)

Comment 12

[Marco Bonardo [:mak]]

why was this marked as a dataloss? it's a select problem, the bookmark retain its title and we are not loosing anything. I'm removing that, restore it if i'm wrong please.

Comment 13

[Ondrej Brablc]

There is a relation to bug 420003. Please fix in the very same way statement mDBBookmarkToUrlResult in nsNavHistory.cpp. Perhaps with some shared SQL fragment (view would be even better).

This statement is used when an item is added to the tag container. This happens when sorting items or when a new link is added to the tag container. In both cases the links have null titles after operation and this is shown of front-end.

Comment 14

[Marco Bonardo [:mak]]

thank you ondrej, will check your comments there

Comment 15

[Ondrej Brablc]

I'm afraid that the penalty would be too big.

I did some performance measurement and it takes 128% of the original time to select a single item. Unfortunately regardless whether the item read was in tag container or in bookmark folder. Reading a folder with 4 links inside takes 198% of the original time.

We should probably use this only when we know we are in the tag container.

Comment 16

[Marco Bonardo [:mak]]

most probably that's because we don't have an index on lastModified?

Comment 17

[Marco Bonardo [:mak]]

i can confirm most of the time is spent ordering by lastModified

Comment 18

[Marco Bonardo [:mak]]

if you try with this index the problems is gone (you loose about 40ms, or less), now we should try to get a similar result without having to create a new index

CREATE INDEX moz_bookmarks_fklmindex ON moz_bookmarks(fk, lastModified);

Comment 19

[Marco Bonardo [:mak]]

actually this is the best i have found:

SELECT h.id, h.url, COALESCE(b.title,
      (
        SELECT title
        FROM moz_bookmarks
        WHERE fk = h.id
        AND lastModified =
            (SELECT MAX(lastModified) FROM moz_bookmarks
             WHERE title NOT NULL AND fk = h.id)
        LIMIT 1
        ),
      h.title), h.rev_host, h.visit_count,
      null
      , f.url, null, b.id,
      b.dateAdded, b.lastModified,
      b.position, b.type, b.fk
     FROM moz_bookmarks b
     LEFT JOIN moz_places h ON b.fk = h.id
     LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id
     ORDER BY b.position

(notice this is not the real query, is lacking max visit date and a where clause, it's only for perf testing on a big bookmarks db)

Original query is taking 220ms, this is taking 350ms, Ondrej got similar results from testing (in percentage). The other query (order by one) was taking HUGE timings worts than these (4900ms on mine).

with the index the ORDER BY query is almost fast like the actual query (but we will have a new index, and it's always better having less than more)

So we have 3 options actually:

1. get my latest query, that is (from Ondrej calculation) about 55% slower than actual

2. create a new index, will loose a 5-10% on perf, and we will have another index (imho good only if we are going use that elsewhere)

3. on update of a bookmark title we update title for all tags referring to the same url (tags have no more null titles, we dont' touch other bookmark's titles)

Comment 20

[Marco Bonardo [:mak]]

Attachment: impl. 1

for testing, this implements point 1

Comment 21

[Marco Bonardo [:mak]]

cons:
- point 1 will slowdown (by some ms) all bookmark view queries, if we cannot title bookmarks into tags and we don't really need a new index, than is the way to go.

- point 2 will increase db size and *could* affect performance globally (a new index has to be maintained by the db and make it grow in size). if we will do many selects based on lastModified to support DB sync, then this is the way to go.

- point 3 is only applicable if we don't need having null titles on bookmarks in tags, but most likely will not have any perf impact. If we can set titles and don't need a new index to speed up syncing based on lastModified, then this is the way to go.

Dietrich, Mano?

Comment 22

[Ondrej Brablc]

(In reply to comment #21)
> - point 3 is only applicable if we don't need having null titles on bookmarks
> in tags, but most likely will not have any perf impact. If we can set titles
> and don't need a new index to speed up syncing based on lastModified, then this
> is the way to go.

I'm afraid that there will be some performance penalty too. Every call to setItemTitle will additionally need to get list of all tags and update their titles including calling of observers. With bug 392193 import should no more call setItemTitle, so this will only be problem for manually inserted items, may be for livemark items and may be for pages changing their title using JavaScript (I'm really not sure here).

But I think that this is the best choice anyway.

Comment 23

[Marco Bonardo [:mak]]

ok after some testing (thanks to ondrej and dietrich for support) here is the final situation:

1. not applicable, if you have a LOT of bookmarks pointing to the same website, this is going to kill perf. tested with hugebookmarks file, lost 46s on a full db query!

3. this appear complex, but the main problem is that setting a title on tags will actually cause a big increase in db size. db size is mostly due to strings, rather than to ints. So duplicating title into tag if you end up having a lot of tagged items will cause a bad increase in db size (and that will again affect performance badly, and complain from the users in extreme cases)

the only remaining option is 2. I'm going modifying the query and creating a new index, that will also be useful when we will sync. Tested with 10500 bookmarks, a hundred tags (increasing them does not appear making things much worse), 4000 bookmarks pointing to the same uri, timings are good (lost 70ms on a full db query).

Comment 24

[Marco Bonardo [:mak]]

after applying the new index patch with my biggish bookmark set i see more lag in the UI, especially in bookmarks menu and in the Library. 
We can't make those perf worst to fix a less important problem.
So i'll try to see if i can come with a frontend only patch for tag containers.

This problem is a pain, and is mostly due to current tag implementation as folders (that causes duplication of data).

Comment 25

[Marco Bonardo [:mak]]

Attachment: patch

This should be better, no the cleanest solution but i can't think anything better.
My perf problems are mostly due to the size of bookmarks (with 2400 bookmarks in the menu i cannot even scroll the right pane of the Library when the menu folder is selected:( and that is with current nightly.), i'm selecting the query on the fly based on the result type, if it's RESULTS_AS_TAG_QUERY we use the new query.

mDBBookmarkToUrlResult is fairly less used, so there the different query should not create any embarassing situation.

Comment 26

[Ondrej Brablc]

Comment on attachment 307705 [details] [diff] [review]
patch

Is the same code used when viewing tag containers from places library? In this case you should probably check whether grand parent is not tags folder.

Comment 27

[Marco Bonardo [:mak]]

but if we  add other checks we will loose the advantage of changing the query dinamically with one check on something that we already know. Then we could directly use the new query always since it would most probably have similar slowdowns. Yes we could have a minimal slowdown on tag containers (real minimal since they are not duped).

Comment 28

[Marco Bonardo [:mak]]

Attachment: patch v2

ok, previous approach could not work because in the right pane of the Library we don't have a RESULTS_AS_TAG_QUERY parent, so we need a way to "decorate" this tagged uris.

So let's move to another solution, add a new RESULTS_AS_TAGGED_URI to results deriving from a RESULTS_AS_TAG_QUERY type, tag folders become queries (no more folder shortcuts) and are served by the query builder, so we use a different query for them that will catch the correct title. Then we have to replace the Tags query in the left pane of the Library to use RESULTS_AS_TAG_QUERY, to do this i need to include the stringBundle from the toolkit, because we have passed string freeze and we don't have a correct string fot Tags Folder in the frontend.

This patch does everything BUT replacing old library left pane query, so to test  you will need a new profile. Still have to see if we should recreate the full left pane queries or only replace the old query with the new one on idle.

Ondrej could you take a look at this, since it involves changes to your code

Comment 29

[Ondrej Brablc]

We will have a problem when tagging a link while there is an observer on the tag containers - they force refresh. I have added 14 tags on one link (sic) and had to kill Firefox after 20 minutes. The statements with title selection seemed extremely slow with my 6000 bookmarks. I will finish the review tomorrow. 

But my feeling is that this should not be a blocker.

Comment 30

[Marco Bonardo [:mak]]

will try your STR, i'm testing with more than 10 000 bookmarks, should see that

Comment 31

[Asaf Romano (gone)]

getItemTitle(PlacesUtils. tagsFolderId) works too.

why did you set exapndQueries on the left pane?

Comment 32

[Asaf Romano (gone)]

<Mano>	here's what we should do then (going to paste that on the bug):
<Mano>	1. Make Tags a query
<Mano>	2. unset exapndQueries
<Mano>	3. make node's hasChildren honor excludeItem for uri queries - return false if it's set.
<Mano>	4. Make HasChildren honor Tag queries - return false if there are no tag folders

Comment 33

[Marco Bonardo [:mak]]

Mano, i've implemented comment 32 and is working, still i have to check where is the perf problem about comment 29, i can reproduce that slowdown

Comment 34

[Marco Bonardo [:mak]]

Attachment: patch v3

the slowdown is caused by a Refresh() loop not by a query slowlyness, when i add a new tag or i remove a tag i get a bunch of calls to nsNavHistoryQueryResultNode:Refresh() that are slowing down the full thing in a bad way.
Also in the library right pane when i double click a tag the place: address has been rewritten so we don't find to the correct tag container in the left pane

need help debugging this

Comment 35

[Marco Bonardo [:mak]]

Attachment: patch v4

this should solve the perf problem, having bookmark queries child of a bookmark query was calling refresh in loop... i've added a fake batching to avoid recursive call and that worked...

This also includes an experiment, since we should drop all bookmarks (there are other bugs that have done changes to all bookmarks, making it read only, so it could be a good moment to drop it and recreate) we could replace actual history query with a RESULTS_AS_DATE_QUERY, since large treeviews are not usable now (they are too slow) and history will be huge, we could split it with this query (query then should be changed to use more good values than 6 days ago).

Testable with a new profile.

Comment 36

[Marco Bonardo [:mak]]

Attachment: screenshot: actual view with splitted history and tags

Comment 37

[Ondrej Brablc]

Comment on attachment 310894 [details] [diff] [review]
patch v4

Nice, I would like to see two more functions CanAlwaysExpand() and ResultTypeIsURI(). The batch code in Refresh() should be explained - why we have it there.

Comment 38

[Recall]

Hi, if this patch makes it in will it allow json to keep the custom names? Currently when I restore from a json backup, it does not retain the names or does this require a seperate bug?

Comment 39

[Marco Bonardo [:mak]]

Karl. this is only to show changed bookmarks names in tag containers, nothing more than that, will not affect import/export in any way

Comment 40

[Recall]

Marco. Thanks, nice to see this bug is looking like getting fixed. I will make a separate bug for json problem.

Comment 41

[Marco Bonardo [:mak]]

Attachment: patch v5

this is another approach... previous was good but not completely solving the problem.

We should return the real bookmarks in a tag container, not our "shortcut" to the bookmark... this way the title is correct and the user can edit the tag child and see changes on the real bookmark...
So when viewing a tag child you are viewing the lastModified bookmark for that uri. 
This patch does that, plus fixes container icons to use tag icons (we should add a history container icon).

Ondrej please, take a look at my approach and if you have some time give it some  test about those perf problems you were seeing.

Comment 42

[Ondrej Brablc]

Comment on attachment 311601 [details] [diff] [review]
patch v5

It behaves somehow nonstandard, so I believe it is not because I have broken build:

1) Recent Tags has tag icon, it should stay with query icon.
2a) Recent Tags in places library should not be expandable (or at least it was not before).
2b) When clicking on a tag container in the library, it behaved as a shortcut to the "Tags" folder.
3) Ordering does not seem to work for items in "Recent Tags", the Added and Last Modified columns are empty. Tagging an item by the least recently used tag does not push it to the top.
4) It is no more possible to sort content of tag container.
5) Change of item title seems to be visible in Recent Tags folder, but not in Tags folder. I even do not have the same content for some tags.

Comment 43

[Marco Bonardo [:mak]]

(In reply to comment #42)
> 1) Recent Tags has tag icon, it should stay with query icon.

why can't we decorate a tag query with a tag icon? Hwv not a big deal, it's an easy change.

> 2a) Recent Tags in places library should not be expandable (or at least it was
> not before).

this is going to be expandable if we are making query-containing-queries always expand and is what we are doing from the first. But clearly it should not show bookmarks inside.

> 2b) When clicking on a tag container in the library, it behaved as a shortcut
> to the "Tags" folder.

can you explain better? when double clicking on a tag container the respective tag folder is opened under Tags... it works in the same way in current trunk (apart that recent tags is not expandable), the search in the tree takes the first item with the same uri.

> 3) Ordering does not seem to work for items in "Recent Tags", the Added and
> Last Modified columns are empty. Tagging an item by the least recently used tag does not push it to the top.

i've not changed the queyr. so i should check why we are loosing them for tag containers, they were folder shortcuts, while now are queries so i suppose is due to this

> 4) It is no more possible to sort content of tag container.

We can't retain all functionality moving from a folder to a query container. That's a query we should sort contents based on some specified criteria, like by lastModified or by title, letting the user sort a tag container by title does not make much sense, let's sort it by default then.

> 5) Change of item title seems to be visible in Recent Tags folder, but not in
> Tags folder. I even do not have the same content for some tags.

i see in both places, and i see same content, is that with a new profile? do you have STR please?

Comment 44

[Marco Bonardo [:mak]]

Attachment: v6

So, thank you Ondrej for following me here, your testing is really appreciated!
This version should bring some major improvement over the previous, i've removed history splitting, should be done in another bug.

I've fixed many of the problems you reported, still you can't sort content of tag containers (selecting sort by name from the context menu), imo this is a wontfix, they are sorted by title by default.

Plan is to do this for new profiles only, History splitting for new profiles in another bug. Then a new bug should provide versioning for the left pane, so we can upgrade old profiles easily.

Comment 45

[Marco Bonardo [:mak]]

some annotations from Ondrej's testing (Thank you, again):

- the expand/compact glyph sometimes is not drawn near Tags container in the Library. Don't know much about this, could be a problem in my code for hasChildren.

- deleting a bookmark inside a tag deletes the bookmark, should be catched somewhere, probably the user is expecting that removing a bookmark under a tag will only remove the tag, not the bookmark itself.

- need to check if we can have null titled bookmarks that are not tag children, since those would be filtered out. I'd expect having empty string titled bookmarks, not null titles, checking grandparent would be better but *could* be slow (need to measure)

- need to check if we can end up with null lastModified. It should always be at least = dateAdded, are we allowing this?

- Tags icon in the Library is a query icon now, should that be a normal folder icon? Do we need a new icon for it? The same icon will be used in all saved searches with the same result type (TAG_QUERY). Need UX team ideas on this

Comment 46

[Ondrej Brablc]

Comment on attachment 311760 [details] [diff] [review]
v6

The last patch was much better, however it should be noted, that with this patch following changes:

- It is no more possible to change order of tags in the Library->Tags
- It is no more possible to change order inside of individual tags.

This is a side effect and should be discussed (it will make the bug 424648 invalid), I do not know if it is on some test plan, but people tend to sort the tag containers. May be they valuate this more than titles sync.

Comment 47

[Marco Bonardo [:mak]]

(In reply to comment #46)
> - It is no more possible to change order of tags in the Library->Tags
> - It is no more possible to change order inside of individual tags.

true, still you can sort them using library columns. i'm not sure that ordering tags or tag contents "by hand" has a big advantage, in fact we have a bug asking to have them sorted by name rather than by position.

Instead this adds some interesting possibility:
- titles are always correct
- you can directly edit bookmarks properties inside a tag container
- bookmarks in tag containers have valid properties from the original bookmark (lastModified for example) so sorting works better and as expected
- you can directly copy or drag a bookmark from a tag container to the toolbar or menu for example and see it moving really, while now you are dragging an empty titled shortcut
- you can no more tag an history item or create a folder/separator inside a tag container (these are other 2 bugs filled that should be fixed)
- will allow us to change tag backed easily in future (for example moving them to a separate table)

Comment 48

[Marco Bonardo [:mak]]

Attachment: patch v7

after chat with Mano and Dietrich we think that we can go on, loosing sorting is not so blocking until we can sort in the library (we could provide a way to create a most used tag smart bookmark)

> - the expand/compact glyph sometimes is not drawn near Tags container in the
> Library. Don't know much about this, could be a problem in my code for
> hasChildren.

Should work better, i had to check mValidContent

> - deleting a bookmark inside a tag deletes the bookmark, should be catched
> somewhere, probably the user is expecting that removing a bookmark under a tag
> will only remove the tag, not the bookmark itself.

fixed using transaction manager, so should work in a realiable manner

> - need to check if we can have null titled bookmarks that are not tag children

fixed the query, so now i'm filtering based on grandparent

> - need to check if we can end up with null lastModified. It should always be at
> least = dateAdded, are we allowing this?

fixed for newly created items, old items will be fixed on idle so we can't have items with a NULL lastModified (should be at least = dateAdded)

> - Tags icon in the Library is a query icon now, should that be a normal folder
> icon? Do we need a new icon for it? The same icon will be used in all saved
> searches with the same result type (TAG_QUERY). Need UX team ideas on this

fixed tags and history icons based on Faaborg's mockup (and chat with him). Still lacking an icon for day containers, but bits added to css for future.


Actual issues:

- You can't sort items inside tag container (they inherit the sorting from the parent, so in Library they are sorted by title, in Recent tags are sorted by last dateAdded)

- You can't drop an item into a tag container to add a tag, imho is not a so blocking feature so it could be addressed in a followup

- You need a new profile

Mano, could you take a first review on this?

Comment 49

[Asaf Romano (gone)]

Comment on attachment 312306 [details] [diff] [review]
patch v7

<MaK77>	so we always have lastModified filled
<Mano>	that's pretty bad
<Mano>	i don't want last modified visible for new bookmarks
<Mano>	but we can fix that in the front end
<Mano>	add to treeview.js please
<Mano>	if == dateAdded return "" etc.

<Mano>	+ // parent parameter
+ if (aQuery->Folders().Length() == 1) {
+ rv = statement->BindInt64Parameter(index.For(":parent"),
+ aQuery->Folders()[0]);
+ NS_ENSURE_SUCCESS(rv, rv);
+ }
+
<Mano>	we could have multiple folder set(!)
<Mano>	this is done post-query elsewhere
<MaK77>	multiple folder on a tag container?
<Mano>	what would a place: string look like?
<MaK77>	resultType=7&folder=X&other_options
<MaK77>	folder=X points to the real tag folder
<Mano>	can you comment above the 7 constant that parnet= works differently in that mode
<Mano>	sorry, above parent;
<Mano>	sorry, above setFolders
<Mano>	that 1. It's not recursive 2. It must be a tag folder 3. Multiple folders are ignored

<Mano>	+ // if we are child of an ExcludeItems root, we should not expand
+ if (mResult && mResult->mRootNode->mOptions->ExcludeItems())
+ return PR_FALSE;
+
<Mano>	are all queries just-itesms now?
<Mano>	you need to check the resulttype there, i think
<MaK77>	in the library left pane we have TAG_QUERY 2 times
<Mano>	could be either RESULT_TYPE_URI or the new one, right?
<MaK77>	one for the TAGS container, one for the smart bookmarks
<Mano>	i know what this is for
<Mano>	but I'm pretty sure we've got some queries that result in containers
<MaK77>	but excluditems is ignored for those queries
<Mano>	where do you ignore it?
<MaK77>	CanExpand calls CanAlwaysExpand
<Mano>	_ah_
<MaK77>	and haschildren check if they have some container inside
<Mano>	ok, comment on that in CanExapnd.
<Mano> 	that CanAlwaysExapnd handles container-queries
...
<Mano> maybe just rename canalwyasexapnd to IsContainersQuery


<Mano>	for container queries,
<Mano>	is mChildren valid even when the container is closed?
<MaK77>	mh! i have to open it probably before.. that could explain why i was not immediately getting the expand glyph
<MaK77>	so i open the container and then check mChildre.count(), right?
<Mano>	I'm not sure if that wouldn't break the view...
<Mano>	you need to 1. If it's open, just check
<Mano>	2. If it's not. Null the view (and keep it in a var), open the container, check, close the container, reset the view
<MaK77>	this code previously was doing
<MaK77>	if (mContentsValid) {
<MaK77>	*aHasChildren = (mChildren.Count() > 0);
<MaK77>	so if mContentsValid the container could be already open
<Mano>	right
<Mano>	and then we should the twisty till you try to open the query
<Mano>	you could keep it as is, and just handle the _tags_ query some other way
<Mano>	(check if any tags exist)
<Mano>	it's the only visible containers-query that we care of that much
<MaK77>	but then, what will happen when doing the same for splitting history?
<Mano>	we'll special case that too (which is even much easier)
...
<Mano>	we cannot just open the query as a general solution

<Mano>	and we can fall back to the general solution above otherwise.
<Mano>	as long as you only do so for containers-queries.
<Mano>	for items queries _without_ excludeItems set, I'm fine with showing the twisty.
<Mano>	you can do that in a follow up, in this one i would just continue to return true unless it's the tags query

Comment 50

[Marco Bonardo [:mak]]

Attachment: patch v8

Additional fixes from previous version:

- addressed Mano comments
- drag & drop from/to tag containers works again
- dropping an history item into a tag container adds it to unfiled bookmarks (this should solve bug 410196)
- lastModified is no longer required on all items (db size saving), it was bogus since was dependant on the order in which sqlite was executing the queries (so if lastModified item was contained in tag we could had hidden items). Now results are post-filtered in FilterResultSet, so they should be consistent.
- disabled inserting into tag containers (context menu fix, inserted items would be hidden). Should be fixed adding a new bookmark in unfiled bookmarks, then taggin it, but that's an edge case imo.
- dragging from a tag container should result in a copy op to avoid moving the real bookmark.
- removed all styling bits for icons, Mano is fixing them in another blocker

Actual issues:

- You can't permanently sort items inside tag container, nor sorting tag containers, still can sort them in the Library.
- You need a new profile (bug 425161 will solve this)
- needs a unit test to ensure that TAGGED_URI is returning correct titles (can be done later though)

Comment 51

[Asaf Romano (gone)]

Can we rename RESULTS_AS_TAGGED_URI to something like RESULTS_AS_TAG_CONTAINER_ CONTENTS and explain in the interface what do you mean by "special title". Please also move the additional setFolders documentation to the IDL.

Comment 52

[Asaf Romano (gone)]

Comment on attachment 314336 [details] [diff] [review]
patch v8

And:

>Index: toolkit/components/places/src/utils.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v
>retrieving revision 1.13
>diff -u -8 -p -r1.13 utils.js
>--- toolkit/components/places/src/utils.js	7 Apr 2008 15:59:30 -0000	1.13
>+++ toolkit/components/places/src/utils.js	8 Apr 2008 14:00:36 -0000
>@@ -250,16 +250,18 @@ var PlacesUtils = {
>    * they cannot be removed depending on the circumstance)
>    * @param   aNode
>    *          A result node
>    * @returns true if the node is readonly, false otherwise
>    */
>   nodeIsReadOnly: function PU_nodeIsReadOnly(aNode) {
>     if (this.nodeIsFolder(aNode))
>       return this.bookmarks.getFolderReadonly(asQuery(aNode).folderItemId);
>+    if (this.nodeIsTagQuery(aNode))
>+      return false;
>     if (this.nodeIsQuery(aNode))
>       return asQuery(aNode).childrenReadOnly;

&& ! just_check_the_type_run_as_query_once ;)


>    * Determines whether or not a ResultNode is a host container.
>    * @param   aNode
>@@ -284,16 +286,28 @@ var PlacesUtils = {
>     return aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY &&
>            aNode.parent &&
>            ((resultType = asQuery(aNode.parent).queryOptions.resultType) ==
>                Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY ||
>              resultType == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY);
>   },
> 
>   /**
>+   * Determines whether or not a ResultNode is a tag container.

nit: result-node

>+   * @param   node
>+   *          A NavHistoryResultNode

ditto.

>+   * @returns true if the node is a tag container, false otherwise
>+   */
>+  nodeIsTagQuery: function PU_nodeIsTagQuery(aNode) {
>+    return aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY &&
>+           asQuery(aNode).queryOptions.resultType ==
>+             Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAGGED_URI;
>+  },
>+

why do we need to check both?


>   getConcreteItemId: function PU_getConcreteItemId(aNode) {
>     if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT)
>       return asQuery(aNode).folderItemId;
>+    else if (PlacesUtils.nodeIsQuery(aNode)) {
>+      // Some queries (i.e. RESULTS_AS_TAGGED_URI) are similar to folder
>+      // shortcuts, so we can still get the concrete itemId for them.
>+      var queries = asQuery(aNode).getQueries({});
>+      if (queries.length == 1) {
>+        var folders = queries[0].getFolders({});
>+          if (folders.length == 1)
>+            return folders[0];
>+      }
>+    }

I don't like messing with getConcreteItemId for this, or anything which pretends to support this "globally". For starters, when the result type is not RESULTS_AS_TAGGED_URI, options.folders is recursive, thus this doesn't apply at all.

>Index: browser/components/places/content/controller.js
>===================================================================

>   _canInsert: function PC__canInsert() {
>-    return this._view.insertionPoint != null;
>+    // don't allow inserting into Tag Containers
>+    var ip = this._view.insertionPoint;
>+    return ip != null &&
>+           PlacesUtils.bookmarks.getFolderIdForItem(ip.itemId) !=

We *must* avoid running statements like this during command update, cannot you check the parent's result type?

>       if (PlacesUtils.nodeIsFolder(node))
>         removedFolders.push(node);
>+      else if (PlacesUtils.nodeIsTagQuery(node.parent)) {
>+        var queries = asQuery(node.parent).getQueries({});
>+        if (queries.length == 1) {
>+          var folders = queries[0].getFolders({});
>+          if (folders.length == 1) {
>+            var uri = PlacesUtils._uri(node.uri);
>+            // extract tag title from the generating query
>+            var tag = PlacesUtils.bookmarks.getItemTitle(folders[0]);
>+            // if tag is empty we would delete all tags from an uri
>+            if (tag)
>+              transactions.push(PlacesUIUtils.ptm.untagURI(uri, tag));

untagURI can take the itemId.

>+          }
>+        }
>+        continue;
>+      }
> 
>       transactions.push(PlacesUIUtils.ptm.removeItem(node.itemId));
>     }
>   },
> 
>   /**
>    * Removes the set of selected ranges from bookmarks.
>    * @param   txnName
>@@ -1332,19 +1351,38 @@ var PlacesControllerDragHelper = {
>       // drag multiple elts upward: need to increment index or each successive
>       // elt will be inserted at the same index, each above the previous.
>       if ((index != -1) && ((index < unwrapped.index) ||
>                            (unwrapped.folder && (index < unwrapped.folder.index)))) {
>         index = index + movedCount;
>         movedCount++;
>       }
> 
>-      transactions.push(PlacesUIUtils.makeTransaction(unwrapped,
>-                        flavor.value, insertionPoint.itemId,
>-                        index, copy));
>+      // if dragging over a tag container we should tag the item
>+      var parent = PlacesUtils.bookmarks.getFolderIdForItem(insertionPoint.itemId);
>+      if (parent == PlacesUtils.tagsFolderId) {
>+        var uri = PlacesUtils._uri(unwrapped.uri);
>+        // if uri is not bookmarked we should add it to unsorted bookmarks
>+        var bmkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(uri, {});
>+        if (bmkIds.length == 0) {

use PlacesUtils.getMostRecentBookmarkForURI

>+          transactions.push(PlacesUIUtils.ptm.createItem(uri,
>+                                                         PlacesUtils.unfiledBookmarksFolderId,
>+                                                         PlacesUtils.bookmarks.DEFAULT_INDEX,
>+                                                         unwrapped.title));
>+        }
>+        var tag = PlacesUtils.bookmarks.getItemTitle(insertionPoint.itemId);
>+        // don't try to add an empty tag
>+        if (tag)
>+          transactions.push(PlacesUIUtils.ptm.tagURI(uri, tag));

tagURI can do so too.

>+      }
>+      else {
>+        transactions.push(PlacesUIUtils.makeTransaction(unwrapped,
>+                          flavor.value, insertionPoint.itemId,
>+                          index, copy));
>+      }

>Index: browser/components/places/content/menu.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/menu.xml,v
>retrieving revision 1.127
>diff -u -8 -p -r1.127 menu.xml
>--- browser/components/places/content/menu.xml	6 Apr 2008 19:41:08 -0000	1.127
>+++ browser/components/places/content/menu.xml	8 Apr 2008 14:00:39 -0000
>@@ -184,22 +184,24 @@
>         ]]></body>
>       </method>
> 
>       <method name="onDragStart">
>         <parameter name="aEvent"/>
>         <parameter name="aXferData"/>
>         <parameter name="aDragAction"/>
>         <body><![CDATA[
>-          if (aEvent.ctrlKey)
>+          if (aEvent.ctrlKey ||
>+              PlacesUtils.nodeIsTagQuery(aEvent.target.node.parent))
>             aDragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
> 

nodeIsQuery makes better sense here, I think.

>Index: browser/components/places/content/tree.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/tree.xml,v
>retrieving revision 1.111
>diff -u -8 -p -r1.111 tree.xml
>--- browser/components/places/content/tree.xml	14 Mar 2008 00:52:21 -0000	1.111
>+++ browser/components/places/content/tree.xml	8 Apr 2008 14:00:40 -0000
>@@ -198,16 +198,18 @@
>             nodesURIChecked.push(containerURI);
> 
>             var wasOpen = container.containerOpen;
>             if (!wasOpen)
>               container.containerOpen = true;
>             for (var i = 0; i < container.childCount; ++i) {
>               var child = container.getChild(i);
>               var childURI = child.uri;
>+              // remove excludeitems option
>+              childURI = childURI.replace("&excludeItems=1", "");

*ew*, why? I would rather unset in the caller, preferebally on a concrete query object.

>Index: browser/components/places/content/treeView.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v
>retrieving revision 1.61
>diff -u -8 -p -r1.61 treeView.js
>--- browser/components/places/content/treeView.js	8 Apr 2008 05:51:23 -0000	1.61
>+++ browser/components/places/content/treeView.js	8 Apr 2008 14:00:42 -0000
>@@ -989,28 +989,38 @@ PlacesTreeView.prototype = {
>       // The user cannot drop an item into itself or a read-only container
>       var dragService =  Cc["@mozilla.org/widget/dragservice;1"].
>                          getService(Ci.nsIDragService);
>       var dragSession = dragService.getCurrentSession();
>       var elt = dragSession.sourceNode.parentNode;
>       if (elt.localName == "tree" && elt.view == this &&
>           this.selection.isSelected(aRow))
>         return false;
>+      if (PlacesUtils.nodeIsTagQuery(node))
>+        return true;
>       if (node.parent && PlacesUtils.nodeIsReadOnly(node.parent))
>         return false;
>     }
>   
>     var ip = this._t(aRow, aOrientation);
>     return ip && PlacesControllerDragHelper.canDrop(ip);

Shouldn't _getInsertionPoint handle that?

Comment 53

[Dietrich Ayala (:dietrich)]

Comment on attachment 314336 [details] [diff] [review]
patch v8

a few drive-by comments:

>@@ -4983,16 +5034,23 @@ nsNavHistory::BindQueryClauseParameters(
> 
>   // annotation
>   if (NS_SUCCEEDED(aQuery->GetHasAnnotation(&hasIt)) && hasIt) {
>     rv = statement->BindUTF8StringParameter(index.For(":anno"), 
>                                             aQuery->Annotation());
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>+  // parent parameter
>+  if (aQuery->Folders().Length() == 1) {
>+    rv = statement->BindInt64Parameter(index.For(":parent"),
>+                                            aQuery->Folders()[0]);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+

fix indent

>@@ -5249,16 +5310,24 @@ nsNavHistory::FilterResultSet(nsNavHisto
>       }
> 
>       // Skip if we don't match all terms in the title, url or tag
>       if (!matchAll)
>         continue;
> 
>       appendNode = PR_TRUE;
>     }
>+
>+    // RESULTS_AS_TAGGED_URI returns a set ordered by place_id and lastModified,
>+    // so, to remove duplicates, we can retain the first result for each uri.
>+    if (aOptions->ResultType() == nsINavHistoryQueryOptions::RESULTS_AS_TAGGED_URI) {
>+      if (nodeIndex > 0 && aSet[nodeIndex]->mURI == aSet[nodeIndex-1]->mURI)
>+        appendNode = PR_FALSE;
>+    }
>+

you could get the result type outside the loop, instead of querying the options object each iteration.

also, is there a reason to not just continue in this case?

>@@ -5961,16 +6038,21 @@ GetSimpleBookmarksQueryFolder(const nsCO
>   if (hasIt)
>     return 0;
>   (void)query->GetHasSearchTerms(&hasIt);
>   if (hasIt)
>     return 0;
>   if (aOptions->MaxResults() > 0)
>     return 0;
> 
>+  // RESULTS_AS_TAGGED_URI is quite similar to a folder shortcut, but we must
>+  // avoid threating it like that, since we need to retain all query options.

s/threating/treating/

>Index: toolkit/components/places/src/nsNavHistoryQuery.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v
>retrieving revision 1.37
>diff -u -8 -p -r1.37 nsNavHistoryQuery.cpp
>--- toolkit/components/places/src/nsNavHistoryQuery.cpp	15 Mar 2008 19:39:04 -0000	1.37
>+++ toolkit/components/places/src/nsNavHistoryQuery.cpp	8 Apr 2008 14:00:29 -0000
>@@ -1124,16 +1124,19 @@ NS_IMETHODIMP nsNavHistoryQuery::GetFold
> }
> 
> NS_IMETHODIMP nsNavHistoryQuery::GetFolderCount(PRUint32 *aCount)
> {
>   *aCount = mFolders.Length();
>   return NS_OK;
> }
> 
>+// For special result type RESULTS_AS_TAGGED_URI we can define only one folder
>+// that must be a tag folder. This is not recursive so results wll be catched

s/wll be catched/will be cached/

Comment 54

[Marco Bonardo [:mak]]

Attachment: patch V9

new name is RESULTS_AS_TAG_CONTENTS
should have fixed all comments from Mano and Dietrich, thank you for support!

Actual issues:
- You can't permanently sort items inside tag container, nor sorting tag
containers, still can sort them in the Library.
- You need a new profile (bug 425161 will solve this)
- needs a unit test to ensure that TAGGED_URI is returning correct titles (can
be done later though)

Comment 55

[Asaf Romano (gone)]

Comment on attachment 314715 [details] [diff] [review]
patch V9

Please update menu.xml and toolbar.xml too to set isTag on their insertionpoint objects.

Other than that:

>Index: toolkit/components/places/src/nsNavHistoryResult.cpp
>===================================================================

> NS_IMETHODIMP
> nsNavHistoryQueryResultNode::GetHasChildren(PRBool* aHasChildren)
> {
>-  if (! CanExpand()) {
>+  if (!CanExpand()) {
>     *aHasChildren = PR_FALSE;
>     return NS_OK;
>   }
>+
>+  PRUint16 resultType = mOptions->ResultType();
>+  // For tag containers query we must check if we have any tag
>+  if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY) {
>+    nsresult rv;
>+    nsCOMPtr<nsITaggingService> tagsvc =
>+      do_GetService("@mozilla.org/browser/tagging-service;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsIVariant *tags;
>+    tagsvc->GetAllTags(&tags);
>+    *aHasChildren = ((nsTArray<PRUnichar*>*) tags)->Length() > 0;
>+    return NS_OK;

This leaks the tags array, doesn't it? I think it would be better to run a sql query here anyway.


>Index: browser/components/places/content/controller.js
>===================================================================

>    * Determines whether or not nodes can be inserted relative to the selection.
>    */
>   _canInsert: function PC__canInsert() {
>-    return this._view.insertionPoint != null;
>+    var ip = this._view.insertionPoint;
>+    return ip != null && !ip.isTag;

It's optional ("undefined" in certain place, so please make that

 var ip = this._view.insertionPoint;
 return ip && ip.isTag != true;

>+      // if dragging over a tag container we should tag the item
>+      var parent = PlacesUtils.bookmarks.getFolderIdForItem(insertionPoint.itemId);
>+      if (parent == PlacesUtils.tagsFolderId) {
>+        var uri = PlacesUtils._uri(unwrapped.uri);
>+        var tagItemId = insertionPoint.itemId;
>+        transactions.push(PlacesUIUtils.ptm.tagURI(uri,[tagItemId]));
>+      }

Could we use isTag here too?

>Index: browser/components/places/content/tree.xml
>===================================================================

>+          // allow dropping into single folder queries (like Tag containers)
>+          if (PlacesUtils.nodeIsQuery(aContainer)) {
>+            var queries = asQuery(aContainer).getQueries({});
>+            if (queries.length == 1) {
>+              var folders = queries[0].getFolders({});
>+              return folders.length != 1;
>+            }
>+          }


tag-query, not any query (ditto on treeView.js).

Comment 56

[Marco Bonardo [:mak]]

Attachment: patch v10

we're near the light...
Added the unit test, fixed a minor problem with a failing unit tests, addressed comments.

Known issues for this version:
- You can't permanently sort items inside tag containers, nor sorting tag
containers, still can sort them in the Library.
- You need a new profile (bug 425161)

Comment 57

[Asaf Romano (gone)]

Comment on attachment 314826 [details] [diff] [review]
patch v10


>Index: toolkit/components/places/src/nsNavHistoryResult.cpp
>===================================================================

>+    nsCOMPtr<mozIStorageStatement> hasTagsStatement;
>+    nsresult rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
>+        "SELECT id FROM moz_bookmarks WHERE parent = "
>+        "(SELECT folder_id FROM moz_bookmarks_roots WHERE root_name = 'tags') "
>+        "LIMIT 1"),
>+      getter_AddRefs(hasTagsStatement));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+

let's don't make this complicated, we've the tags root cached in few places.

>+    return hasTagsStatement->ExecuteStep(aHasChildren);
>+  }
>+
>+  //XXX: For history containers query we must check if we have any history
>+  if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY ||
>+      resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_SITE_QUERY ||
>+      resultType == nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY) {
>+    nsNavHistory* history = nsNavHistory::GetHistoryService();
>+    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
>+    history->GetHasHistoryEntries(aHasChildren);
>+    return NS_OK;

make that |return history->GetHasHistoryEntries(aHasChildren);|



>Index: toolkit/components/places/src/utils.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v
>retrieving revision 1.14
>diff -u -8 -p -r1.14 utils.js
>--- toolkit/components/places/src/utils.js	8 Apr 2008 18:41:05 -0000	1.14
>+++ toolkit/components/places/src/utils.js	10 Apr 2008 11:50:48 -0000
>@@ -250,17 +250,19 @@ var PlacesUtils = {
>    * they cannot be removed depending on the circumstance)
>    * @param   aNode
>    *          A result node
>    * @returns true if the node is readonly, false otherwise
>    */
>   nodeIsReadOnly: function PU_nodeIsReadOnly(aNode) {
>     if (this.nodeIsFolder(aNode))
>       return this.bookmarks.getFolderReadonly(asQuery(aNode).folderItemId);
>-    if (this.nodeIsQuery(aNode))
>+    if (this.nodeIsQuery(aNode) &&
>+        asQuery(aNode).queryOptions.resultType !=
>+          Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS)
>       return asQuery(aNode).childrenReadOnly;

This can be return |aNode.childrenReadOnly;| now.
>     return false;
>   },
> 
>   /**
>    * Determines whether or not a ResultNode is a host container.
>    * @param   aNode
>    *          A result node
>@@ -284,16 +286,28 @@ var PlacesUtils = {
>     return aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY &&
>            aNode.parent &&
>            ((resultType = asQuery(aNode.parent).queryOptions.resultType) ==
>                Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY ||
>              resultType == Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_SITE_QUERY);
>   },
> 
>   /**
>+   * Determines whether or not a result-node is a tag container.
>+   * @param   node

aNode

>+  /**
>    * Determines whether or not a ResultNode is a container.
>    * @param   aNode
>    *          A result node
>    * @returns true if the node is a container item, false otherwise
>    */
>   containerTypes: [Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER,
>                    Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT,
>                    Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY,
>@@ -371,16 +385,26 @@ var PlacesUtils = {
> 
>   /**
>    * Gets the concrete item-id for the given node. Generally, this is just
>    * node.itemId, but for folder-shortcuts that's node.folderItemId.
>    */
>   getConcreteItemId: function PU_getConcreteItemId(aNode) {
>     if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT)
>       return asQuery(aNode).folderItemId;
>+    else if (PlacesUtils.nodeIsTagQuery(aNode)) {
>+      // RESULTS_AS_TAG_CONTENTS queries are similar to folder shortcuts
>+      // so we can still get the concrete itemId for them.
>+      var queries = asQuery(aNode).getQueries({});

You don't need QI here either, nodeIsTagQuery does that for you.

>+      if (queries.length == 1) {
>+        var folders = queries[0].getFolders({});
>+          if (folders.length == 1)
>+            return folders[0];

I wouldn't bother checking the lengths.

In which case RESULTS_AS_TAG_CONTENTS can result in multiple queries anyway?


>Index: browser/components/places/content/tree.xml
>===================================================================

>       <method name="_disallowInsertion">
>         <parameter name="aContainer"/>
>         <body><![CDATA[
>+          // allow dropping into like Tag containers
>+          if (PlacesUtils.nodeIsTagQuery(aContainer)) {
>+            var queries = asQuery(aContainer).getQueries({});
>+            if (queries.length == 1) {
>+              var folders = queries[0].getFolders({});
>+              return folders.length != 1;
>+            }

see previous comments.


>Index: browser/components/places/content/treeView.js
>===================================================================

>   _disallowInsertion: function PTV__disallowInsertion(aContainer) {
>+    // allow dropping into Tag containers
>+    if (PlacesUtils.nodeIsTagQuery(aContainer)) {
>+      var queries = asQuery(aContainer).getQueries({});
>+      if (queries.length == 1) {
>+        var folders = queries[0].getFolders({});
>+        return folders.length != 1;
>+      }
>+    }

ditto.

r=mano otherwise. Many thanks for this awesome work.

Comment 58

[Marco Bonardo [:mak]]

Attachment: patch v11

drivers: this patch is needed to handle properly tag contents, and will fix other tag containers blockers

Comment 59

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315106 [details] [diff] [review]
patch v11

a=beltzner, please watch for regressions and omgzorz yay!

Comment 60

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.225; previous revision: 1.224
done
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.128; previous revision: 1.127
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.150; previous revision: 1.149
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.112; previous revision: 1.111
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.63; previous revision: 1.62
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.134; previous revision: 1.133
done
Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.40; previous revision: 1.39
done
Checking in toolkit/components/places/public/nsINavHistoryService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v  <--  nsINavHistoryService.idl
new revision: 1.82; previous revision: 1.81
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.160; previous revision: 1.159
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.290; previous revision: 1.289
done
Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v  <--  nsNavHistoryQuery.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.140; previous revision: 1.139
done
Checking in toolkit/components/places/src/nsNavHistoryResult.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v  <--  nsNavHistoryResult.h
new revision: 1.56; previous revision: 1.55
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.15; previous revision: 1.14
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_419731.js,v
done
Checking in toolkit/components/places/tests/unit/test_419731.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_419731.js,v  <--  test_419731.js
initial revision: 1.1
done

Comment 61

[Ronny Perinke]

+  // For history containers query we must check if we have any history
+  if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY ||
+      resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_SITE_QUERY ||
+      resultType == nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY) {
+    nsNavHistory* history = nsNavHistory::GetHistoryService();
+    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
+    return history->GetHasHistoryEntries(aHasChildren);
+  }

I would also use the IsContainersQuery() function there, so have one thing less to change, if there's a new container query type.

Comment 62

[Marco Bonardo [:mak]]

IsContainersQuery() includes (an could potentially future-include) also non history queries...

Comment 63

[Tracy Walker [:tracy]]

verified with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041804 Minefield/3.0pre

Comment 64

[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