314553 - Convert bookmarks to use mozStorage

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Brian Ryner (not reading)]

Comment 1

[Brian Ryner (not reading)]

Attachment: first cut

This first cut basically works from the storage point-of-view, but there's no bookmarks.html import yet, or UI.  I want to get this into the tree so that other people can help work on it.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Do you think it would be a better idea to do this as browser/components/bookmarks2, and just switch which dir is built in the makefile?  I'd like to avoid mingling this code with the current bookmarks code lest it pick up cooties..

Comment 3

[Brian Ryner (not reading)]

I could go either way on that... it would make the makefiles cleaner, but we'd be stuck with the "2" after everything's fully switched over.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Eh, I'm ok with that (being stuck with the 2).. if we want we can try to invent some new name or something, but I'd rather just go with bookmarks2 and move on.

Comment 5

[Brett Wilson]

Comment on attachment 201461 [details] [diff] [review]
first cut

>+  /**
>+   * A unique id for this node.
>+   */
>+  readonly attribute unsigned long id;

Is there any reason to expose this value?

One thing just occurred to me: What if we also stored some kind of "persistent" sorting for a container. So the user could say "Keep sorted by name" or date, or whatever.

For containers, where are we planning on storing extra information, for example, the URL for RDF data sources? Bookmark categories won't have URLs, so we can't look them up in the annotation service.


>+  /**
>+   * The provider responsible for this container's children.
>+   * This can be be used by observers to quickly determine whether
>+   * a notification requires additional actions specific to the container type.
>+   */
>+  readonly attribute ACString providerType;

Did we decide on the value of this? Just some random string? Authors could probably use some naming guidance.

>+/**
>+ * An observer for changes on all bookmarks in a given service's hierarchy.
>+ */

For resorts, we're sending Add then remove notifications, right? That should probably be made clear here. Also, for add and deletes, you should make clear that the observer is responsible for assuming that all other bookmarks shift by one.

>+  /* convenience to avoid having to call getKnownBookmark(BookmarksRoot) */
>+  readonly attribute mozIBookmarkContainer bookmarksRoot;

Is there some reason they all aren't exposed like this? I don't even see why we need getKnownBookmark(). The advantage seems to be that we can add new roots without changing the interface, but it seems that these are pretty stable at this point and this wouldn't be a big deal.

>+  mozIBookmarkContainer createContainer(in boolean readOnly,
>+                                        in boolean temporary,
>+                                        in AString provider,
>+                                        in boolean containerReadOnly);

Could this be moved to the container interface? That way, we would avoid having dangling containers if somebody creates one but never inserts it. This brings up another point: Do containers have exactly one parent? We know that a container named "Cool" may not be unique, but is it OK for the same "Cool" container to be two different places? I think the UI would get confusing and it's better off if we enforce one parent per container. But then we need to decide where/how this gets enforced, and what the preferred method for "moving" containers is.

>+[scriptable, uuid(3196c132-c8c4-4621-a23b-b5251ec7b095)]
>+interface mozIBookmarkImporter : nsISupports
>+{
>+  /**
>+   * Read all bookmarks from the given location and insert them using the
>+   * given bookmark service object.
>+   */
>+  void importFile(in nsIFile file, in mozIBookmarkService bmservice);
>+};

Will this work for importing IE bookmarks that live as separate files in a special folder?


I'll look at the implementation later...

Comment 6

[Brian Ryner (not reading)]

(In reply to comment #5)
> (From update of attachment 201461 [details] [diff] [review] [edit])
> >+  /**
> >+   * A unique id for this node.
> >+   */
> >+  readonly attribute unsigned long id;
> 
> Is there any reason to expose this value?

If nothing else, I'm currently using it in the unit test to verify that one BookmarkNode is equivalent to another BookmarkNode.  There's probably a better way to do this.

> For containers, where are we planning on storing extra information, for
> example, the URL for RDF data sources? Bookmark categories won't have URLs, so
> we can't look them up in the annotation service.
> 

We could store them in the containers table, I guess...

> Did we decide on the value of this? Just some random string? Authors could
> probably use some naming guidance.
> 

I'd been using "local" in my testing, but maybe it makes sense for this to be a contract-id type string?

> For resorts, we're sending Add then remove notifications, right? That should
> probably be made clear here. Also, for add and deletes, you should make clear
> that the observer is responsible for assuming that all other bookmarks shift by
> one.

I think we're sending remove then add for a node move.

> Is there some reason they all aren't exposed like this? I don't even see why we
> need getKnownBookmark(). The advantage seems to be that we can add new roots
> without changing the interface, but it seems that these are pretty stable at
> this point and this wouldn't be a big deal.
> 

I'd be ok with this change.

> >+  mozIBookmarkContainer createContainer(in boolean readOnly,
> >+                                        in boolean temporary,
> >+                                        in AString provider,
> >+                                        in boolean containerReadOnly);
> 
> Could this be moved to the container interface? That way, we would avoid having
> dangling containers if somebody creates one but never inserts it. This brings
> up another point: Do containers have exactly one parent? We know that a
> container named "Cool" may not be unique, but is it OK for the same "Cool"
> container to be two different places? I think the UI would get confusing and
> it's better off if we enforce one parent per container. But then we need to
> decide where/how this gets enforced, and what the preferred method for "moving"
> containers is.

We could do it either way.  It seems easier to let containers work like items and let dangling containers happen, if someone misuses the API.  They should get garbage-collected at some point, like we discussed.

> 
> >+  void importFile(in nsIFile file, in mozIBookmarkService bmservice);
> >+};
> 
> Will this work for importing IE bookmarks that live as separate files in a
> special folder?

I thought those were referenced via some special notation from bookmarks.html, but I admit to not having looked too closely at this.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #6)
> (In reply to comment #5)
> > 
> > >+  void importFile(in nsIFile file, in mozIBookmarkService bmservice);
> > >+};
> > 
> > Will this work for importing IE bookmarks that live as separate files in a
> > special folder?
> 
> I thought those were referenced via some special notation from bookmarks.html,
> but I admit to not having looked too closely at this.
> 

I wouldn't bother with importFile right now; you can sort of reference those via some system bookmarks RDF entry right now, but it never really worked for IE (e.g. it doesn't provide a live view of the bookmarks).  The separate importers know how to get each platform's bookmarks data and the like, we'll just need to fix the importers.

Review of the rest later tonight..

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 201461 [details] [diff] [review]
first cut

(I didn't realize you guys were going to go ahead and start implementing the bookmarks stuff yet.. I have some code [based on the older plan] where I started implementing a lot of this stuff a few months ago, would've put it up first. I put up the interface IDL at http://www.vlad1.com/~vladimir/misc/mozIBookmarks.idl at least.)

One of the main problems that I see with this implementation is that it limits the kind of data that can be stored in a bookmark.  Any bookmark metadata would have to be stored in separate tables.  I originally wanted a bookmark node/container to just be a chunk of metadata that would be interpreted by the node/container provider -- this would give a natural place for container providers such as query-based containers, or remote server containers to store their data (the query string, the server information, etc.).  For nodes, even with dropping the idea of different providers for nodes, it's still useful to be able to have metadata that's just for that node (RSS feed information/metadata that went along with the feed entry/etc.)  That's not possible with the API presented here.  In the IDL I put up, it's assumed that mozIBookmarkNode implements nsIPropertyBag/nsIWritablePropertyBag.

The difficulty I ran into was with deciding when to save/load data.  It was pretty clear that we'd have to load the data into the node/container structures in memory and not be able to operate directly from the database; what wasn't as clear was how to decide when to write data to the database.  My eventual solution was to allow a bookmark service user to begin a transaction on a node (which translated directly to a database transaction) and then to modify as many properties of that node and they wanted and then commit.  If the commit didn't succeed, the node would be reloaded from disk (though you could also checkpoint the node's properties before a transaction start).  There was all sorts of muddiness here though that I never fully worked through and was planning on stepping back and figuring out where the data is actually supposed to go and when.

>+[scriptable, uuid(be2823f2-79b4-4835-8a27-68efe378659f)]
>+interface mozIBookmarkNode : nsISupports {
>+  /**
>+   * A unique id for this node.
>+   */
>+  readonly attribute unsigned long id;

One of the reasons that I had this as a GUID originally was to more easily support synchronization/remote access to bookmarks, because the same GUID could be used everywhere.. if it's a simple increasing id, then whatever implements any kind of remote bookmarks will have to map between these ids and whatever is being used server-side.  This may be ok, as having this be an integer will probably speed up client side access some (but I'm not sure by how much, especially given an index on the id string).

>+  /**
>+   * Indicates whether this node is temporary. Temporary nodes will be removed
>+   * from the bookmarks store as soon as their container is no longer open.
>+   */
>+  readonly attribute boolean temporary;

Do we want a hard guarantee that the node will be removed as soon as the container closes?  I'm not even sure how we'd track that in some UI cases.  Maybe say that it's only around for the current session?  If a container provider wants to change its nodes every time someone opens the container, it's still free to do so, but other containers might want to only periodically update but still have the data be nuked on shutdown/restart.

>+};

I'd add in nsAString getProperty(string propName)/void setProprety(string propName, nsAString propValue) to mozIBookmarkNode here, or have a requirement that nodes implement nsIPropertyBag (and nsIWritablePropertyBag if they're not read-only).

>+/**
>+ * An observer for changes on all bookmarks in a given service's hierarchy.
>+ */
>+[scriptable, uuid(75c53908-7aa0-45ee-a68a-f2db90374742)]
>+interface mozIBookmarkObserver : nsISupports {

>+  /**
>+   * Notify this observer that the bookmark was added.
>+   * Called after the actual add took place.
>+   *
>+   * @param bookmark   The bookmark node that was added.
>+   */
>+  void onBookmarkAdded(in mozIBookmarkNode bookmark,
>+                       in mozIBookmarkContainer container);
>+
>+  /**
>+   * Notify this observer that the bookmark was removed.
>+   * Called before the actual remove takes place.
>+   *
>+   * @param bookmark The bookmark that will be removed.
>+   */
>+  void onBookmarkRemoved(in mozIBookmarkNode bookmark,
>+                         in mozIBookmarkContainer container);

I think that both of these need an |unsigned long index| specifying at what index the bookmark was added in/removed from the container (since the notifier will know, and having that info available can speed up the UI update.. as well as help disambiguate which node is being referred to, if we allow the same node to have multiple parents -- which is a situation that we might end up in, think of a bookmark with the tags "one" and "two", and then a tag-filtering-container for "one" and a separate tag-filtering-container for "two").

>+/*
>+ * The service is the main entrypoint for bookmark users; it provides
>+ * functions for creating bookmarks, as well as access to well-known bookmarks.
>+ */
>+
>+[scriptable, uuid(5ee39c0f-74f1-48ed-990f-fd68f28028e0)]
>+interface mozIBookmarkService  : nsISupports {
>+  /* convenience to avoid having to call getKnownBookmark(BookmarksRoot) */
>+  readonly attribute mozIBookmarkContainer bookmarksRoot;
>+
>+  /* well-known bookmark node identifiers
>+   * XXX - how do we actually set the BookmarksToolbarFolder?
>+   */
>+  const unsigned long kBookmarksRoot = 0;
>+  const unsigned long kBookmarksToolbarFolder = 1;

I added a setKnownBookmark call to set the any of the known bookmarks to be a certain node; I also changed the known bookmarks to use a string instead of an ID, to allow providers to create their own "well-known" bookmarks, to allow for something like:

   var node = bm.getKnownBookmark("remote-sync-root");
   var syncnode = node.QueryInterface(mozIRemoteServerBookmarkContainer);
   syncnode.synchronizeNow();

etc.

>+  /**
>+   * Create a new, empty bookmark container.
>+   *  @param readOnly   Whether the new container is read-only
>+   *  @param temporary  Whether the new container is temporary
>+   *  @param provider   The providerName of the new container
>+   *  @param containerReadOnly  Whether the container's children can be modified
>+   *  @returns          The new bookmark container
>+   */
>+  mozIBookmarkContainer createContainer(in boolean readOnly,
>+                                        in boolean temporary,
>+                                        in AString provider,
>+                                        in boolean containerReadOnly);

What does it mean to create a read-only container here?  How does it get populated?  I'm also ok with having dangling containers/nodes -- it sounds potentially useful and I don't think we should explicitly disallow it (since it doesn't cost us anything).

Comment 9

[Brett Wilson]

Metadata storing: The idea is that if services want to store other information, they can just use the annotation service to associate items with that URL. I think if we decide to use the wide table idea, implementing a property bag interface for the annotation service would be a bad idea because it could encourage people to create random keys, which would be relatively heavyweight. If we use a narrow table, implementing a property bag interface would be fine, but I don't see any value in it if you can use the annotation service interface.

when to load/save: Will just using the DB (if we have good caching) be too slow? It's not clear. I think we can just go and assume it is fast enough. If it's not, we can keep a duplicate copy in memory with write-through. Writing is uncommon and doesn't matter if it's slower. With the background-thread DB flushing, this should even have good performance.

temporary flag: We'll probably get rid of this.

read only: This flag is just a hint to the UI that the user should not be able to change things. RSS would, for example, populate the list itself, so the container has to be technically writable. But the user will be disallowed from doing anything with it.

Comment 10

[Brian Ryner (not reading)]

Some additions to brett and vlad's comments:

(In reply to comment #6)
> > >+  readonly attribute unsigned long id;
> > 
> > Is there any reason to expose this value?
> 

Brett and I agreed that this could be useful to container providers, so it probably makes sense to keep it here.

> > Did we decide on the value of this? Just some random string? Authors could
> > probably use some naming guidance.
> > 
> 
> I'd been using "local" in my testing, but maybe it makes sense for this to be a
> contract-id type string?

I added some comments to this effect.

> I think we're sending remove then add for a node move.

I added some documentation on this as well.

> 
> > Is there some reason they all aren't exposed like this? I don't even see why we
> > need getKnownBookmark(). The advantage seems to be that we can add new roots

I think we're just going to leave it as-is for now and revisit this.

> > >+  mozIBookmarkContainer createContainer(in boolean readOnly,
> > >+                                        in boolean temporary,
> > >+                                        in AString provider,
> > >+                                        in boolean containerReadOnly);
> > 
> > Could this be moved to the container interface? That way, we would avoid having

As a first pass at this, we'll just clean up dangling containers on shutdown.

(In reply to comment #8)
> One of the main problems that I see with this implementation is that it limits
> the kind of data that can be stored in a bookmark.  Any bookmark metadata would
> have to be stored in separate tables.  I originally wanted a bookmark

I think we're going to go with the assumption that item-specific data can be stored in the annotation service, and container-specific data would be managed in a separate table by the container provider.  By starting off with a minimalistic approach like this, we should be able to restructure the storage into a combined table later if we decide to, without a lot of extra work.

> be used everywhere.. if it's a simple increasing id, then whatever implements
> any kind of remote bookmarks will have to map between these ids and whatever is
> being used server-side.  This may be ok, as having this be an integer will

I think this is ok, especially since there are clearly other bottlenecks with server-side storage.

> 
> >+  /**
> >+   * Indicates whether this node is temporary. Temporary nodes will be removed
> >+   * from the bookmarks store as soon as their container is no longer open.
> >+   */
> >+  readonly attribute boolean temporary;
> 
> Do we want a hard guarantee that the node will be removed as soon as the
> container closes?  I'm not even sure how we'd track that in some UI cases. 

We can just remove this attribute and let the container provider manage this.

> I think that both of these need an |unsigned long index| specifying at what
> index the bookmark was added in/removed from the container (since the notifier

Added.

> I added a setKnownBookmark call to set the any of the known bookmarks to be a
> certain node; I also changed the known bookmarks to use a string instead of an
> ID, to allow providers to create their own "well-known" bookmarks, to allow for

This interface change sounds ok to me; I'll change my implementation.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #10)
> (In reply to comment #8)
> > One of the main problems that I see with this implementation is that it limits
> > the kind of data that can be stored in a bookmark.  Any bookmark metadata would
> > have to be stored in separate tables.  I originally wanted a bookmark
> 
> I think we're going to go with the assumption that item-specific data can be
> stored in the annotation service, and container-specific data would be managed
> in a separate table by the container provider.  By starting off with a
> minimalistic approach like this, we should be able to restructure the storage
> into a combined table later if we decide to, without a lot of extra work.

Well, except that all containters would need to be redesigned/rewritten if they were to store data in a property bag.  If an extension wants to annotate a bookmark (not necessarily a URL, but the specific bookmark) -- perhaps with information about when to next check that bookmark to see if the page changed, or maybe to flag it for sime kind of processing -- it would need to not only  maintain its own data store, but would also have to track bookmark removals to make sure it cleaned up its own data correctly (or at least have to handle the case where the original bookmark ID no longer exists).

I'm pretty sure that unless we design for this kind of arbitrary bookmark annotation early on, we'll never get around to adding it later on.

I guess now that I think about it we could just use the annotation service for this -- the URL would be "moz-bookmark-node:15" or whatever (I still am a little worried about using a naked number for an ID, but I /think/ I'm almost convinced it should be ok).

Comment 12

[Brian Ryner (not reading)]

Attachment: new patch

This implements the changes above, and cleans up the unit test a bit more (it can now run from xpcshell, by registering its own DirectoryServiceProvider for the storage file).

Comment 13

[Brett Wilson]

> I guess now that I think about it we could just use the annotation service for
> this -- the URL would be "moz-bookmark-node:15" or whatever (I still am a
> little worried about using a naked number for an ID, but I /think/ I'm almost
> convinced it should be ok).

This is what we had in mind. The annotation service seems like it is the appropriate interface for storing this information. Since bookmarks are just URLs, this is fine, and even desirable in many cases. Very lieelt of the data that we would want to store with bookmarks seems exclusive to bookmarks. Plus, it would have some nice side effects: deleting a bookmark and re-adding it would not (necessarily) clear any additional information you've added. The only issue here is that if you change the URL of a bookmark, we would have to migrate your annotations to the new one.

A problem is with containers, which have no unique URL. If somebody needed a lot of information associated with containers, they could use their own table. It seems the only use case for this is a magical online syncing service of whose properties nobody can yet define, and RSS. We've had a lot of discussions about the mysical services and haven't had much concensus, so I'm inclined to not worry about that for now.

We could add an extra "data" string to bookmark containers. This would allow RSS bookmarks and probably handle similar services with no extra work. More complicated services could use their own table.

Comment 14

[Brian Ryner (not reading)]

Attachment: new patch

I decided to go ahead and translate this to C++.  In the process, I discovered some inconsistencies with string types, and also that the node id needs to be 64-bit in order to match up with sqlite row ids.  Those issues are all fixed now, and this implementation passes the same unit test that the previous implementation did.

I also moved everything into bookmarks2/

Comment 15

[Brett Wilson]

Comment on attachment 201691 [details] [diff] [review]
new patch

>Index: browser/components/bookmarks2/.cvsignore
I think this file is a mistake.

I don't think you want to create statements for every bookmark node. Since it is common to have a reference to every bookmark (like in the bookmark manager), sometimes even more than once (multiple windows), and you store ~10 per node, that's a lot of statements open. These each take some sqlite memory (no idea how much), and have to be parsed and compiled every time you get a new node. You probably want these to live in the bookmark service and have all the nodes have weak references to that.

>+nsresult
>+BookmarkNode::Init(const char* const *aProperties, PRInt32 aNumProperties)
>+{
>+  PRUint32 propCount = NS_ARRAY_LENGTH(sNodeProps) + aNumProperties;
>+  NS_ENSURE_TRUE(mGetStatements.Init(propCount), NS_ERROR_OUT_OF_MEMORY);
>+  NS_ENSURE_TRUE(mSetStatements.Init(propCount), NS_ERROR_OUT_OF_MEMORY);

You're initializing a hash table with the number of things that will be stored in it. This basically guarantees some collisions. Don't you want something bigger?

>+class Transaction
>+{
>+public:
>+  Transaction() : mCommitted(PR_FALSE) { gStorage->BeginTransaction(); }
>+  ~Transaction() { if (!mCommitted) commit(); }
>+  nsresult commit() { mCommitted = PR_TRUE; return gStorage->CommitTransaction(); }
>+private:
>+  PRBool mCommitted;
>+};

Watch out: there may be a transaction open when you start this, in which case the BeginTransaction will fail and Commit will commit somebody else's transaction. I've got some code for this to check in.


>+nsresult
>+BookmarkContainer::RemoveChildFull(mozIBookmarkNode *aNode, PRUint32 aIndex)
>+{
>+  ContainerNotifyData data = { aNode, this, aIndex };
>+  gService->NotifyObservers(NotifyRemoved, &data);

Don't you want to notify on success at the end of this function? Observers will often expect the new state when they get called. Also, I would recommend committing the transaction before you do this (here and elsewhere). That way there is less chance of transaction corruption, and they can do their own transactions that are rollbackable.

Otherwise looks good (too many statements is the main thing).

Comment 16

[Brian Ryner (not reading)]

(In reply to comment #15)
> (From update of attachment 201691 [details] [diff] [review] [edit])
> >Index: browser/components/bookmarks2/.cvsignore
> I think this file is a mistake.
> 

Nope, there should be one of these in every directory with a Makefile.in.

> I don't think you want to create statements for every bookmark node. Since it
> is common to have a reference to every bookmark (like in the bookmark manager),
> sometimes even more than once (multiple windows), and you store ~10 per node,
> that's a lot of statements open. These each take some sqlite memory (no idea
> how much), and have to be parsed and compiled every time you get a new node.
> You probably want these to live in the bookmark service and have all the nodes
> have weak references to that.

This would work for the SELECT statements, I think (we could make the id be a parameter and reuse the statements across nodes).  For UPDATE statements, you can't make the column being set a parameter (http://www.sqlite.org/lang_update.html), so if the space cost is too big, we'll just have to create these statements each time they're needed.

> 
> >+nsresult
> >+BookmarkNode::Init(const char* const *aProperties, PRInt32 aNumProperties)
> >+{
> >+  PRUint32 propCount = NS_ARRAY_LENGTH(sNodeProps) + aNumProperties;
> >+  NS_ENSURE_TRUE(mGetStatements.Init(propCount), NS_ERROR_OUT_OF_MEMORY);
> >+  NS_ENSURE_TRUE(mSetStatements.Init(propCount), NS_ERROR_OUT_OF_MEMORY);
> 
> You're initializing a hash table with the number of things that will be stored
> in it. This basically guarantees some collisions. Don't you want something
> bigger?

It will round up to the next power of two, apparently, but let's figure out if we want these hashtables at all.

> 
> >+class Transaction
> >+{
> >+public:
> >+  Transaction() : mCommitted(PR_FALSE) { gStorage->BeginTransaction(); }
> >+  ~Transaction() { if (!mCommitted) commit(); }
> >+  nsresult commit() { mCommitted = PR_TRUE; return gStorage->CommitTransaction(); }
> >+private:
> >+  PRBool mCommitted;
> >+};
> 
> Watch out: there may be a transaction open when you start this, in which case
> the BeginTransaction will fail and Commit will commit somebody else's
> transaction. I've got some code for this to check in.

Ok, how about if I just take your code and commit it as part of this patch?

> 
> 
> >+nsresult
> >+BookmarkContainer::RemoveChildFull(mozIBookmarkNode *aNode, PRUint32 aIndex)
> >+{
> >+  ContainerNotifyData data = { aNode, this, aIndex };
> >+  gService->NotifyObservers(NotifyRemoved, &data);
> 
> Don't you want to notify on success at the end of this function? Observers will
> often expect the new state when they get called. Also, I would recommend

Probably, unless we want observers to be able to veto the removal (I don't think we need that).  I was basing this on the original comments in the IDL interface that specified that the notification happens before the removal.

> committing the transaction before you do this (here and elsewhere). That way

I thought I was doing that everywhere already, but I'll go verify.

Comment 17

[Brian Ryner (not reading)]

Attachment: totally new implementation

This implementation sits on top of history.

Comment 18

[Brett Wilson]

Comment on attachment 203202 [details] [diff] [review]
totally new implementation

You should wrap the functions containing more than one statement in a transaction. This will cause the statements to share the same cache, and will prevent things from happening behind your back. You should have it at the widest scope. For example, in MoveFolder you have a transaction just around your writes, but you depend on the result of a read for the data to write. Somebody could come in and modify the database between your read and the dependent write, and it will be out of sync.

Are you sure the JOINs in ExecuteQueries is correct? It looks to me like this will select one row for each combination of history and bookmark category. Did you test history queries with this change? I think what we want is to leave the bookmarked table out of the main query's FROM clause, and if you want bookmarked items only, say "AND EXISTS (SELECT b.item_child FROM moz_bookmarks_assoc b WHERE b.item_child = id)"

I added some public functions to nsNavHistory that overlaps some of your additions here:
- GetHistoryService (static)
- GetUrlIdFor (duplicates your nsNavBookmarks::GetIDFromURI, but I should use "URI" instead of "URL" - d'oh).
- GetStorageConnection

With these (and perhaps a few additions), is it necessary to make the navBookmarks class a friend? It would be nice to avoid this.

Comment 19

[Brian Ryner (not reading)]

Attachment: revised patch

Comment 20

[Brian Ryner (not reading)]

Comment on attachment 203301 [details] [diff] [review]
revised patch

checked in; leaving open for future work.

Comment 21

[Brian Ryner (not reading)]

Attachment: fixes, and hook up to preliminary UI

assorted fixes to make lazy building of children actually work

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #19)
> Created an attachment (id=203301) [edit]
> revised patch
> 
Couldn't you have used a macro for those *very* long literals?
For example the following is not really readable atm
http://lxr.mozilla.org/mozilla/source/browser/components/places/src/nsNavBookmarks.cpp#140

Comment 23

[Brian Ryner (not reading)]

Attachment: notify wantAllDetails obsevers on renumber, and make api nicer

Comment 24

[Brian Ryner (not reading)]

Attachment: support items that refer to a history query

a few changes here:
- added a new result type for queries, with a getter for the parsed queries/options
- removed the |const| on nsINavHistoryQuery arrays... it doesn't make a lot of sense to me the way xpidl generates the header.  A const array (nsINavHistoryQuery * const *) might make sense, but that's not what xpidl wants to give you.
- made the code more tolerant of uris that don't have hostnames -- not all do, in particular, place:
- fixed bug in QueryStringToQueries -- the options object wasn't being returned
- fixed bug in nsNavHistoryResultNode, it wasn't QI'ing to the private IID

Comment 25

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Is there any documentation available on this new system and how it works, i.e. a Wiki? I'm interested to know whether or not the change of storage mechanism will also change the way the bookmarks core interacts with the tabbed browser.

Comment 26

[Jo Hermans]

(In reply to comment #25)
> Is there any documentation available on this new system and how it works, i.e.
> a Wiki? I'm interested to know whether or not the change of storage mechanism
> will also change the way the bookmarks core interacts with the tabbed browser.
> 

http://wiki.mozilla.org/Places

Comment 27

[Brian Ryner (not reading)]

Attachment: make folders just be a type of query node

This gets rid of some redundancy between folders and queries, and just makes folders be a type of query node.  Plus some other misc fixes.

Comment 28

[Brett Wilson]

Attachment: Add some default places queries to places root

This is kind of a hack to get "All History" and "Today's History" in the places view on the left. I think the current plan is to move the defaults into some bookmarks.html-like-file that we import on profile init. This will let people use these features in the meantime.

Note: moved folder initialization until after statements are created so that secondary functions will work correctly.

Comment 29

[Brett Wilson]

Comment on attachment 204375 [details] [diff] [review]
make folders just be a type of query node

>   /**
>-   * Group by bookmark folder.
>-   * This should only be used for queries which have onlyBookmarked set.
>+   * Group by bookmark folder.  Since this determines the entire subtree
>+   * hierarchy, it must be the last grouping option given.  This option
>+   * requires the query to have onlyBookmarked set, and for there to be
>+   * at least one parent folder specified via nsINavHistoryQuery::setFolders.
>+   * If all of the results belong to a single folder, they will be moved up
>+   * to the top level of the result.
>    */
>   const PRInt32 GROUP_BY_FOLDER = 3;

What do you mean by "all results belong to a single folder"? This seems like the right thing for searching, but isn't this interface used by the menu as well? Will this make folders containing a single sub-folder collapse?


>+      // folders (comma-separated id list)
>+      // XXX fixme to work with > 1 folder

The groups which are also a list are specified as separate parameters. So you would have group=2&group=1 to have two-level grouping. This should probably work the same way, or we should change the grouping syntax.

Comment 30

[Brian Ryner (not reading)]

Attachment: make moveFolder handle new index = -1, and update unit test

Comment 31

[Brett Wilson]

Attachment: Add GetChildFolder to bookmarks service

This adds a way to go from a name to a folder ID given a parent.
Note: case sensetive.

Comment 32

[Brian Ryner (not reading)]

Comment on attachment 204498 [details] [diff] [review]
Add GetChildFolder to bookmarks service

You've got a lot of changes tangled up together here.  r=me on the getChildFolder stuff.

Comment 33

[Brian Ryner (not reading)]

I'm going to go ahead and resolve this bug; we have other bugs to track remaining issues.

Comment 34

[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