382877 - dynamic containers implementation

Status: RESOLVED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

there's a bunch of commented out code for this which needs to evaluated. this is a key feature for extensions to hook into into the Places system.

Comment 1

[Asaf Romano (gone)]

Attachment: part 1

address all/most of the requirements for the tag queries.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 274084 [details] [diff] [review]
part 1


>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v
>retrieving revision 1.45
>diff -u -p -8 -r1.45 nsINavBookmarksService.idl
>--- toolkit/components/places/public/nsINavBookmarksService.idl	1 Jun 2007 00:44:43 -0000	1.45
>+++ toolkit/components/places/public/nsINavBookmarksService.idl	26 Jul 2007 23:40:51 -0000
>@@ -192,16 +192,17 @@ interface nsINavBookmarksService : nsISu
>    * where an index is not known, or not required to be specified.
>    * e.g.: When appending an item to a folder.
>    */
>   const short DEFAULT_INDEX = -1;
> 
>   const unsigned short TYPE_BOOKMARK = 1;
>   const unsigned short TYPE_FOLDER = 2;
>   const unsigned short TYPE_SEPARATOR = 3;
>+  const unsigned short TYPE_INTERACTIVE_CONTAINER = 4;

rev UUID

also, why interactive instead of remote? i prefer "dynamic", but regardless i think we should change them all (per bug 333732), or none at all yet.

>@@ -238,24 +239,32 @@ interface nsINavBookmarksService : nsISu
>    * Wrapper for container services.  Creates a folder under the given
>    * parent and sets the container type. Containers are wrappers around
>    * read-only folders, with a specific type.
>    *
>    *  @param aParentFolder
>    *         The id of the parent folder
>    *  @param aName
>    *         The name of the new folder
>-   *  @param aType
>-   *         The type of container to insert
>+   *  @param aContractId
>+   *         The contract id of the service which is to manipulate this
>+   *         container.
>+   *  @param aIsBookmarkFolder
>+   *         Whether this container is a bookmark folder. If false, the
>+   *         contents of the container can be manipulated using the append*
>+   *         methods of nsINavHistoryContainerResultNode* only, and only from
>+   *         onContainer* of the container implementation.
>    *  @param aIndex
>    *         The index to insert at, or -1 to append
>+   *         
>    *  @returns the ID of the newly-inserted folder
>    */
>-  long long createContainer(in long long aParentFolder, in AString aName,
>-                            in AString aType, in long aIndex);
>+  long long createRemoteContainer(in long long aParentFolder, in AString aName,
>+                                  in AString aType, in boolean aIsBookmarkFolder,
>+                                  in long aIndex);

s/aType/aContractId/

Comment 3

[Asaf Romano (gone)]

<Mano>	dietrich: so, interactive to distinguish from permanent remote containers
<Mano>	dietrich: like livemarks
<Mano>	dietrich: which cannot and should not use the append* apis
<Mano>	dietrich: createRemoteContainer can create either a folder or an interactive container
<Mano>	depending on aIsBookmarksFolder

Comment 4

[Asaf Romano (gone)]

Attachment: more...

Comment 5

[Asaf Romano (gone)]

Attachment: more...

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 274305 [details] [diff] [review]
more...


>+/**
>+ * The dynamic container interface provides a base class for services that want
>+ * to provide containers for temporaray contents.

spelling: temporary

>+ *
>+ * The service can fill result nodes directly into the container when it is
>+ * being opened. It can use the property bag on every result node to store data
>+ * associated with each item, such as full path on disk. It would create
>+ * additional containers for each container, resgistered to its service.

spelling: registered

>+ *
>+ * See also createDynamicContainer in nsINavBookmarksService.
>+ */
>+
>+[scriptable, uuid(7e85d97b-4109-4ea7-afd8-bc2cd3840d70)]
>+interface nsIDynamicContainer : nsISupports
>+{
>+  /**
>+   * Called when the given container is about to be populated so that the
>+   * service can populate the container if necessary.
>+   *
>+   * @param container   The container node for the container being opened.
>+   *                    If the node type is a bookmarks container, you can
>+   *                    QI it to nsINavHistoryFolderResultNode and access the
>+   *                    folder ID, etc. Note that all result nodes implement
>+   *                    a property bag if you need to store state.
>+   * @param options     The options used to generate this query. Containers
>+   *                    should follow these when possible, for example,
>+   *                    whether to expand queries, etc. Implementations should
>+   *                    use this when possible if adding query and folder nodes
>+   *                    to the container. DO NOT MODIFY THIS VALUE.
>+   *
>+   */
>+  void onContainerOpening(in nsINavHistoryContainerResultNode container,
>+                          in nsINavHistoryQueryOptions options);
>+

prefix parameters with "a"?

>+  /**
>+   * Called when the given container has just been hidden so that the service
>+   * can do any necessary cleanup. This is NOT guaranteed to get called. In
>+   * particular, if the query just goes away (like the user switched views on
>+   * the places page) you will not get this call. This only happens when the
>+   * container itself goes from the open state to the closed state. A serviced

typo: service

>+   * with large numbers of dynamically populated items might use this to do
>+   * some cleanup so those items don't hang around
>+   *
>+   * @param container   The container node of the container being closed. The
>+   *                    service need not worry about removing any created nodes,
>+   *                    they will be automatically removed when this call
>+   *                    completes.
>+   */
>+  void onContainerClosed(in nsINavHistoryContainerResultNode container);
>+
>+  /**
>+   * Called when the given container is about to be deleted, so
>+   * that the service can do any necessary cleanup.
>+   * Called BEFORE the container is deleted, so that the service
>+   * can still reference it.
>+   * @param container   The folderId of the bookmark folder
>+   *                    representing the container to be deleted.
>+   */
>+  void onContainerRemoving(in long long container);

why folder id instead of container node, as with the previous apis?

>+
>+  /**
>+   * Called when the given container has just been moved, in case
>+   * the service needs to do any bookkeeping.
>+   * Called AFTER the container has been moved, so the service can
>+   * get the new URI.

s/URI/folder id/

>+   * @param container   The folderId of the bookmark folder
>+   *                    representing the container to be moved.
>+   * @param newFolder   The folderId of the new parent folder
>+   *                    for the container.
>+   * @param newIndex    The index the container will be inserted at,
>+   *                    or -1 for append.
>+   */
>+  void onContainerMoved(in long long container,
>+                        in long long newFolder, in long newIndex);
>+};

would be clearer:

s/container/folderId/
s/newFolder/newParentId/

>Index: toolkit/components/places/public/nsILivemarkService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsILivemarkService.idl,v
>retrieving revision 1.7
>diff -u -p -8 -r1.7 nsILivemarkService.idl
>--- toolkit/components/places/public/nsILivemarkService.idl	23 May 2007 01:18:21 -0000	1.7
>+++ toolkit/components/places/public/nsILivemarkService.idl	28 Jul 2007 20:55:39 -0000
>@@ -33,23 +33,22 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
>-#include "nsIRemoteContainer.idl"
> 
> interface nsIURI;
> interface nsINavBookmarksService;
> 
>-[scriptable, uuid(86f0be08-7b7f-4ec6-97ff-ecace917b852)]
>-interface nsILivemarkService : nsIRemoteContainer
>+[scriptable, uuid(602e3a71-2d10-4d8f-80c2-6db302b5c89d)]
>+interface nsILivemarkService : nsISupports
> {
>   /**
>    * Creates a new livemark
>    * @param folder      The id of the parent folder
>    * @param name        The name to show when displaying the livemark
>    * @param siteURI     The URI of the site the livemark was created from
>    * @param feedURI     The URI of the actual RSS feed
>    * @param index       The index to insert at, or -1 to append
>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v
>retrieving revision 1.46
>diff -u -p -8 -r1.46 nsINavBookmarksService.idl
>--- toolkit/components/places/public/nsINavBookmarksService.idl	26 Jul 2007 03:53:59 -0000	1.46
>+++ toolkit/components/places/public/nsINavBookmarksService.idl	28 Jul 2007 20:55:40 -0000
>@@ -17,16 +17,18 @@
>  * The Initial Developer of the Original Code is
>  * Google Inc.
>  * Portions created by the Initial Developer are Copyright (C) 2005
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Brian Ryner <[e-mail]> (original author)
>  *   Joe Hughes <[e-mail]>
>+ *   Dietrich Ayala <[e-mail]>
>+ *   Asaf Romano <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>@@ -156,17 +158,17 @@ interface nsINavBookmarkObserver : nsISu
> };
> 
> /**
>  * The BookmarksService interface provides methods for managing bookmarked
>  * history items.  Bookmarks consist of a set of user-customizable
>  * folders.  A URI in history can be contained in one or more such folders.
>  */
> 
>-[scriptable, uuid(630dcd21-402c-44cd-8336-78bff3efc5f3)]
>+[scriptable, uuid(117e4d4c-8c10-4c50-b588-848942b55b6e)]
> interface nsINavBookmarksService : nsISupports
> {
>   /**
>    * The folder ID of the Places root.
>    */
>   readonly attribute long long placesRoot;
> 
>   /**
>@@ -192,16 +194,17 @@ interface nsINavBookmarksService : nsISu
>    * where an index is not known, or not required to be specified.
>    * e.g.: When appending an item to a folder.
>    */
>   const short DEFAULT_INDEX = -1;
> 
>   const unsigned short TYPE_BOOKMARK = 1;
>   const unsigned short TYPE_FOLDER = 2;
>   const unsigned short TYPE_SEPARATOR = 3;
>+  const unsigned short TYPE_DYNAMIC_CONTAINER = 4;
> 
>   /**
>    * Inserts a child bookmark into the given folder. If this item already exists in
>    * the given folder, it will be moved to the new position.
>    *  @param aParentFolder
>    *         The id of the parent folder
>    *  @param aURI
>    *         The URI to insert
>@@ -230,32 +233,32 @@ interface nsINavBookmarksService : nsISu
>    *  @param aIndex
>    *         The index to insert at, or -1 to append
>    *  @returns the ID of the newly-inserted folder
>    */
>   long long createFolder(in long long aParentFolder, in AString name,
>                          in long index);
> 
>   /**
>-   * Wrapper for container services.  Creates a folder under the given
>-   * parent and sets the container type. Containers are wrappers around
>-   * read-only folders, with a specific type.
>+   * Creates a dynamic container under the given parent folder.
>    *
>    *  @param aParentFolder
>    *         The id of the parent folder
>    *  @param aName
>    *         The name of the new folder
>-   *  @param aType
>-   *         The type of container to insert
>+   *  @param aContractId
>+   *         The contract id of the service which is to manipulate this
>+   *         container.
>    *  @param aIndex
>    *         The index to insert at, or -1 to append
>+   *         
>    *  @returns the ID of the newly-inserted folder
>    */
>-  long long createContainer(in long long aParentFolder, in AString aName,
>-                            in AString aType, in long aIndex);
>+  long long createDynamicContainer(in long long aParentFolder, in AString aName,
>+                                  in AString aType, in long aIndex);

s/aType/aContractId

up to here...

Comment 7

[Asaf Romano (gone)]

Attachment: patch

Comment 8

[O. Atsushi (Torisugari)]

> const unsigned long RESULT_TYPE_DYNAMIC_CONTAINER = 4;
> const unsigned long RESULT_TYPE_FOLDER = 6;

Maybe it's too late to complain about, but if it was implemented something
like this, it would be by far more developer-friendly.

> const unsigned long RESULT_TYPE_CONTAINER = 0x0001;
> const unsigned long RESULT_TYPE_FOLDER = 0x0001 | 0x0002;
> const unsigned long RESULT_TYPE_DYNAMIC_CONTAINER = 0x0001 | 0x0100;

Sometimes we want to know the target is a container or not, regardless of
whether dynamic or not. Anothertimes we may need to know just its
persistence... 

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #8)
> > const unsigned long RESULT_TYPE_DYNAMIC_CONTAINER = 4;
> > const unsigned long RESULT_TYPE_FOLDER = 6;
> 
> Maybe it's too late to complain about, but if it was implemented something
> like this, it would be by far more developer-friendly.
> 
> > const unsigned long RESULT_TYPE_CONTAINER = 0x0001;
> > const unsigned long RESULT_TYPE_FOLDER = 0x0001 | 0x0002;
> > const unsigned long RESULT_TYPE_DYNAMIC_CONTAINER = 0x0001 | 0x0100;
> 
> Sometimes we want to know the target is a container or not, regardless of
> whether dynamic or not. Anothertimes we may need to know just its
> persistence... 
> 

Perhaps not as efficient, but there are utility functions for this:

http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistoryResult.h#290

Comment 10

[Dietrich Ayala (:dietrich)]

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


>+
>+  /**
>+   * Called when the given container has just been moved, in case
>+   * the service needs to do any bookkeeping.
>+   * Called AFTER the container has been moved.
>+   * @param aItemId
>+   *        The item-id of the container item.
>+   * @param aNewParent
>+   *        The item of the new parent folder for the container.
>+   * @param aNewIndex
>+   *        The index the container will be inserted at, or -1 for append.
>+   */
>+  void onContainerMoved(in long long aItmeId,
>+                        in long long aNewParent, in long aNewIndex);
>+};

s/aItmeId/aItemId/


>Index: toolkit/components/places/src/nsLivemarkService.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v
>retrieving revision 1.19
>diff -u -p -8 -r1.19 nsLivemarkService.js
>--- toolkit/components/places/src/nsLivemarkService.js	11 Jul 2007 09:58:00 -0000	1.19
>+++ toolkit/components/places/src/nsLivemarkService.js	30 Jul 2007 13:18:31 -0000
>@@ -139,16 +139,18 @@ function LivemarkService() {
>   for (var i = 0; i < livemarks.length; i++) {
>     var feedURI =
>       gIoService.newURI(
>         this._ans.getItemAnnotation(livemarks[i], LMANNO_FEEDURI),
>         null, null
>       );
>     this._pushLivemark(livemarks[i], feedURI);
>   }
>+
>+  this._bms.addObserver(this, false);

does this observer need to be removed at some point?

>@@ -1398,17 +1401,17 @@ nsNavBookmarks::GetRemoveFolderTransacti
>   rv = GetParentAndIndexOfFolder(aFolder, &parent, &index);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCAutoString type;
>   rv = GetFolderType(aFolder, type);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   RemoveFolderTransaction* rft = 
>-    new RemoveFolderTransaction(aFolder, parent, title, index, type);
>+    new RemoveFolderTransaction(aFolder, parent, title, index, NS_ConvertUTF8toUTF16(type));
>   if (!rft)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   NS_ADDREF(*aResult = rft);
>   return NS_OK;
> }
> 
> NS_IMETHODIMP



>-  // type
>-  nsCAutoString folderType;
>-  rv = mDBGetItemProperties->GetUTF8String(kGetItemPropertiesIndex_FolderType, folderType);
>+  // contract id
>+  nsCAutoString contractId;
>+  rv = mDBGetItemProperties->GetUTF8String(kGetItemPropertiesIndex_FolderType, contractId);
>   NS_ENSURE_SUCCESS(rv, rv);

nit: change the index name?

> 
>   // title
>   nsCAutoString title;
>   rv = mDBGetItemProperties->GetUTF8String(kGetItemPropertiesIndex_Title, title);
> 
>-  *aNode = new nsNavHistoryFolderResultNode(title, aOptions, aID, folderType);
>+  PRInt32 itemType = mDBGetItemProperties->AsInt32(kGetItemPropertiesIndex_Type);
>+  if (itemType == TYPE_DYNAMIC_CONTAINER) {
>+    *aNode = new nsNavHistoryContainerResultNode(EmptyCString(), title, EmptyCString(),
>+                                                 nsINavHistoryResultNode::RESULT_TYPE_DYNAMIC_CONTAINER,
>+                                                 PR_TRUE,
>+                                                 contractId,
>+                                                 aOptions);
>+    (*aNode)->mItemId = aID;
>+  } else { // TYPE_FOLDER
>+    *aNode = new nsNavHistoryFolderResultNode(title, aOptions, aID, contractId);
>+  }
>   if (!*aNode)
>     return NS_ERROR_OUT_OF_MEMORY;
> 
>   (*aNode)->mDateAdded =
>     mDBGetItemProperties->AsInt64(kGetItemPropertiesIndex_DateAdded);
>   (*aNode)->mLastModified =
>     mDBGetItemProperties->AsInt64(kGetItemPropertiesIndex_LastModified);
> 
>@@ -1961,37 +1974,16 @@ nsNavBookmarks::GetFolderURI(PRInt64 aFo
>   //
>   // If you change this, change IsSimpleFolderURI which detects this string.
>   nsCAutoString spec("place:folder=");
>   spec.AppendInt(aFolder);
>   spec.AppendLiteral("&group=3"); // GROUP_BY_FOLDER
>   return NS_NewURI(aURI, spec);
> }
> 
>-NS_IMETHODIMP
>-nsNavBookmarks::GetFolderReadonly(PRInt64 aFolder, PRBool *aResult)
>-{
>-  // Ask the folder's nsIRemoteContainer for the readonly property.
>-  *aResult = PR_FALSE;
>-  nsCAutoString type;
>-  nsresult rv = GetFolderType(aFolder, type);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  if (!type.IsEmpty()) {
>-    nsCOMPtr<nsIRemoteContainer> container =
>-      do_GetService(type.get(), &rv);
>-    if (NS_SUCCEEDED(rv)) {
>-      rv = container->GetChildrenReadOnly(aResult);
>-      NS_ENSURE_SUCCESS(rv, rv);
>-    }
>-  }
>-
>-  return NS_OK;
>-}
>-
> nsresult
> nsNavBookmarks::QueryFolderChildren(PRInt64 aFolderId,
>                                     nsNavHistoryQueryOptions *aOptions,
>                                     nsCOMArray<nsNavHistoryResultNode> *aChildren)
> {
>   mozStorageStatementScoper scope(mDBGetChildren);
> 
>   nsresult rv = mDBGetChildren->BindInt64Parameter(0, aFolderId);
>@@ -2010,30 +2002,30 @@ nsNavBookmarks::QueryFolderChildren(PRIn
>   while (NS_SUCCEEDED(mDBGetChildren->ExecuteStep(&results)) && results) {
> 
>     // The results will be in order of index. Even if we don't add a node
>     // because it was excluded, we need to count it's index, so do that
>     // before doing anything else. Index was initialized to -1 above, so
>     // it will start counting at 0 the first time through the loop.
>     index ++;
> 
>-    PRBool isFolder = mDBGetChildren->AsInt32(kGetChildrenIndex_Type) == TYPE_FOLDER;
>+    PRInt32 itemType = mDBGetChildren->AsInt32(kGetChildrenIndex_Type);
>     PRInt64 id = mDBGetChildren->AsInt64(nsNavHistory::kGetInfoIndex_ItemId);
>     nsCOMPtr<nsNavHistoryResultNode> node;
>-    if (isFolder) {
>-
>-      if (options->ExcludeReadOnlyFolders()) {
>+    if (itemType == TYPE_FOLDER || itemType == TYPE_DYNAMIC_CONTAINER) {
>+      if (itemType == TYPE_DYNAMIC_CONTAINER ||
>+          (itemType == TYPE_FOLDER && options->ExcludeReadOnlyFolders())) {
>         // see if it's read only and skip it
>         PRBool readOnly = PR_FALSE;
>         GetFolderReadonly(id, &readOnly);
>         if (readOnly)
>           continue; // skip
>       }
> 
>-      rv = ResultNodeForFolder(id, aOptions, getter_AddRefs(node));
>+      rv = ResultNodeForContainer(id, aOptions, getter_AddRefs(node));
>       if (NS_FAILED(rv))
>         continue;
>     } else if (mDBGetChildren->AsInt32(kGetChildrenIndex_Type) == TYPE_SEPARATOR) {
>       // separator
>       if (aOptions->ExcludeItems()) {
>         continue;
>       }
>       node = new nsNavHistorySeparatorResultNode();
>Index: toolkit/components/places/src/nsNavBookmarks.h
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v
>retrieving revision 1.45
>diff -u -p -8 -r1.45 nsNavBookmarks.h
>--- toolkit/components/places/src/nsNavBookmarks.h	26 Jul 2007 03:54:00 -0000	1.45
>+++ toolkit/components/places/src/nsNavBookmarks.h	30 Jul 2007 13:18:59 -0000
>@@ -72,42 +72,35 @@ public:
>       NS_ENSURE_SUCCESS(rv, nsnull);
>       NS_ASSERTION(sInstance, "Should have static instance pointer now");
>     }
>     return sInstance;
>   }
> 
>   nsresult AddBookmarkToHash(PRInt64 aBookmarkId, PRTime aMinTime);
> 
>-  nsresult ResultNodeForFolder(PRInt64 aID, nsNavHistoryQueryOptions *aOptions,
>-                               nsNavHistoryResultNode **aNode);
>+  nsresult ResultNodeForContainer(PRInt64 aID, nsNavHistoryQueryOptions *aOptions,
>+                                  nsNavHistoryResultNode **aNode);
> 
>   // Find all the children of a folder, using the given query and options.
>   // For each child, a ResultNode is created and added to |children|.
>   // The results are ordered by folder position.
>   nsresult QueryFolderChildren(PRInt64 aFolderId,
>                                nsNavHistoryQueryOptions *aOptions,
>                                nsCOMArray<nsNavHistoryResultNode> *children);
> 
>   // If aFolder is -1, uses the autoincrement id for folder index. Returns
>   // the index of the new folder in aIndex, whether it was passed in or
>-  // generated by autoincrement.  If aSendNotifications is true, sends
>-  // OnFolderAdded notifications to bookmark observers.
>-  nsresult CreateFolderWithID(PRInt64 aFolder, PRInt64 aParent, 
>-                              const nsAString& title,
>-                              PRBool aSendNotifications,
>-                              PRInt32 *aIndex, PRInt64* aNewFolder);
>-
>-  // Creates a new container of the given type.  If aFolder is -1, uses the
>-  // autoincrement id for folder index.  Sends OnFolderAdded notifications
>-  // to all observers after the folder has been created and its type has
>-  // been set.
>-  nsresult CreateContainerWithID(PRInt64 aFolder, PRInt64 aParent,
>-                                 const nsAString& title, const nsAString& type,
>-                                 PRInt32 aIndex, PRInt64* aNewFolder);
>+  // generated by autoincrement.
>+  nsresult CreateContainerWithID(PRInt64 aId, PRInt64 aParent,
>+                                 const nsAString& aName,
>+                                 const nsAString& aContractId,
>+                                 PRBool aIsBookmarkFolder,
>+                                 PRInt32* aIndex,
>+                                 PRInt64* aNewFolder);
> 
>   // Called by History service when quitting.
>   nsresult OnQuit();
> 
>   nsresult BeginUpdateBatch();
>   nsresult EndUpdateBatch();
> 
>   PRBool ItemExists(PRInt64 aItemId);
>@@ -214,17 +207,17 @@ private:
>   nsCOMPtr<mozIStorageStatement> mDBGetURIForKeyword;
> 
>   nsCOMPtr<nsIStringBundle> mBundle;
> 
>   class RemoveFolderTransaction : public nsITransaction {
>   public:
>     RemoveFolderTransaction(PRInt64 aID, PRInt64 aParent, 
>                             const nsAString& aTitle, PRInt32 aIndex,
>-                            const nsACString& aType) 
>+                            const nsAString& aType) 
>                             : mID(aID),
>                               mParent(aParent),
>                               mIndex(aIndex){
>       mTitle = aTitle;
>       mType = aType;
>     }
>     
>     NS_DECL_ISUPPORTS
>@@ -232,22 +225,18 @@ private:
>     NS_IMETHOD DoTransaction() {
>       nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>       return bookmarks->RemoveFolder(mID);
>     }
> 
>     NS_IMETHOD UndoTransaction() {
>       nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>       PRInt64 newFolder;
>-      // If the transaction has no specific type, default to a folder, and send notifications
>-      // to all bookmark observers (controlled by the PR_TRUE argument to CreateFolderWithID).
>-      if (mType.IsEmpty())
>-        return bookmarks->CreateFolderWithID(mID, mParent, mTitle, PR_TRUE, &mIndex, &newFolder);
>-      nsAutoString type; type.AssignWithConversion(mType);
>-      return bookmarks->CreateContainerWithID(mID, mParent, mTitle, type, mIndex, &newFolder); 
>+      return bookmarks->CreateContainerWithID(mID, mParent, mTitle, mType, PR_TRUE,
>+                                              &mIndex, &newFolder); 
>     }
> 
>     NS_IMETHOD RedoTransaction() {
>       return DoTransaction();
>     }
> 
>     NS_IMETHOD GetIsTransient(PRBool* aResult) {
>       *aResult = PR_FALSE;
>@@ -258,17 +247,17 @@ private:
>       *aResult = PR_FALSE;
>       return NS_OK;
>     }
> 
>   private:
>     PRInt64 mID;
>     PRInt64 mParent;
>     nsString mTitle;
>-    nsCString mType;
>+    nsString mType;
>     PRInt32 mIndex;
>   };
> };
> 
> struct nsBookmarksUpdateBatcher
> {
>   nsBookmarksUpdateBatcher() { nsNavBookmarks::GetBookmarksService()->BeginUpdateBatch(); }
>   ~nsBookmarksUpdateBatcher() { nsNavBookmarks::GetBookmarksService()->EndUpdateBatch(); }
>Index: toolkit/components/places/src/nsNavHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v
>retrieving revision 1.146
>diff -u -p -8 -r1.146 nsNavHistory.cpp
>--- toolkit/components/places/src/nsNavHistory.cpp	26 Jul 2007 21:39:06 -0000	1.146
>+++ toolkit/components/places/src/nsNavHistory.cpp	30 Jul 2007 13:19:39 -0000
>@@ -1972,18 +1972,18 @@ nsNavHistory::ExecuteQueries(nsINavHisto
>   nsRefPtr<nsNavHistoryContainerResultNode> rootNode;
>   PRInt64 folderId = GetSimpleBookmarksQueryFolder(queries, options);
>   if (folderId) {
>     // In the simple case where we're just querying children of a single bookmark
>     // folder, we can more efficiently generate results.
>     nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>     NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
>     nsCOMPtr<nsNavHistoryResultNode> tempRootNode;
>-    rv = bookmarks->ResultNodeForFolder(folderId, options,
>-                                        getter_AddRefs(tempRootNode));
>+    rv = bookmarks->ResultNodeForContainer(folderId, options,
>+                                           getter_AddRefs(tempRootNode));
>     NS_ENSURE_SUCCESS(rv, rv);
>     rootNode = tempRootNode->GetAsContainer();
>   } else {
>     // complex query
>     rootNode = new nsNavHistoryQueryResultNode(EmptyCString(), EmptyCString(),
>                                                queries, options);
>     NS_ENSURE_TRUE(rootNode, NS_ERROR_OUT_OF_MEMORY);
>   }
>@@ -3855,17 +3855,18 @@ nsNavHistory::GroupByDay(nsNavHistoryQue
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>       // need to create an entry for this date
>       dates[ageInDays] = new nsNavHistoryContainerResultNode(urn, 
>           curDateName,
>           EmptyCString(),
>           nsNavHistoryResultNode::RESULT_TYPE_DAY,
>           PR_TRUE,
>-          EmptyCString());
>+          EmptyCString(),
>+          nsnull);
> 
>       if (!dates[ageInDays])
>         return NS_ERROR_OUT_OF_MEMORY;
>     }
>     if (!dates[ageInDays]->mChildren.AppendObject(aSource[i]))
>       return NS_ERROR_OUT_OF_MEMORY;
>   }
> 
>@@ -3933,17 +3934,17 @@ nsNavHistory::GroupByHost(nsNavHistoryQu
>       TitleForDomain(curHostName, title);
> 
>       nsCAutoString urn;
>       nsresult rv = CreatePlacesPersistURN(aResultNode, UNDEFINED_AGE_IN_DAYS, title, urn);
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>       curTopGroup = new nsNavHistoryContainerResultNode(urn, title,
>           EmptyCString(), nsNavHistoryResultNode::RESULT_TYPE_HOST, PR_TRUE,
>-          EmptyCString());
>+          EmptyCString(), nsnull);
>       if (! curTopGroup)
>         return NS_ERROR_OUT_OF_MEMORY;
> 
>       if (! hosts.Put(curHostName, curTopGroup))
>         return NS_ERROR_OUT_OF_MEMORY;
>    
>       rv = aDest->AppendObject(curTopGroup);
>       NS_ENSURE_SUCCESS(rv, rv);
>@@ -4270,17 +4271,17 @@ nsNavHistory::QueryRowToResult(const nsA
>   } else {
>     PRInt64 folderId = GetSimpleBookmarksQueryFolder(queries, options);
>     if (folderId) {
>       // simple bookmarks folder, magically generate a bookmarks folder node
>       nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>       NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
> 
>       // this addrefs for us
>-      rv = bookmarks->ResultNodeForFolder(folderId, options, aNode);
>+      rv = bookmarks->ResultNodeForContainer(folderId, options, aNode);
>       NS_ENSURE_SUCCESS(rv, rv);
>     } else {
>       // regular query
>       *aNode = new nsNavHistoryQueryResultNode(aTitle, EmptyCString(),
>                                                queries, options);
>       if (! *aNode)
>         return NS_ERROR_OUT_OF_MEMORY;
>       NS_ADDREF(*aNode);
>Index: toolkit/components/places/src/nsNavHistoryResult.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v
>retrieving revision 1.108
>diff -u -p -8 -r1.108 nsNavHistoryResult.cpp
>--- toolkit/components/places/src/nsNavHistoryResult.cpp	26 Jul 2007 16:23:11 -0000	1.108
>+++ toolkit/components/places/src/nsNavHistoryResult.cpp	30 Jul 2007 13:20:06 -0000
>@@ -49,17 +49,17 @@
> #include "nsFaviconService.h"
> #include "nsHashPropertyBag.h"
> #include "nsIComponentManager.h"
> #include "nsIDateTimeFormat.h"
> #include "nsIDOMElement.h"
> #include "nsILocale.h"
> #include "nsILocaleService.h"
> #include "nsILocalFile.h"
>-#include "nsIRemoteContainer.h"
>+#include "nsIDynamicContainer.h"
> #include "nsIServiceManager.h"
> #include "nsISupportsPrimitives.h"
> #include "nsITreeColumns.h"
> #include "nsIURI.h"
> #include "nsIURL.h"
> #include "nsIWritablePropertyBag.h"
> #include "nsNetUtil.h"
> #include "nsPrintfCString.h"
>@@ -198,21 +198,19 @@ nsNavHistoryResultNode::GetResult()
> 
> nsNavHistoryQueryOptions*
> nsNavHistoryResultNode::GetGeneratingOptions()
> {
>   if (! mParent) {
>     // When we have no parent, it either means we haven't built the tree yet,
>     // in which case calling this function is a bug, or this node is the root
>     // of the tree. When we are the root of the tree, our own options are the
>-    // generating options, and we know we are either a query of a folder node.
>-    if (IsFolder())
>-      return GetAsFolder()->mOptions;
>-    else if (IsQuery())
>-      return GetAsQuery()->mOptions;
>+    // generating options.
>+    if (IsContainer())
>+      return GetAsContainer()->mOptions;
>     NS_NOTREACHED("Can't find a generating node for this container, perhaps FillStats has not been called on this tree yet?");
>     return nsnull;
>   }
> 
>   nsNavHistoryContainerResultNode* cur = mParent;
>   while (cur) {
>     if (cur->IsFolder())
>       return cur->GetAsFolder()->mOptions;
>@@ -267,23 +265,24 @@ NS_IMPL_RELEASE_INHERITED(nsNavHistoryCo
> NS_INTERFACE_MAP_BEGIN(nsNavHistoryContainerResultNode)
>   NS_INTERFACE_MAP_STATIC_AMBIGUOUS(nsNavHistoryContainerResultNode)
>   NS_INTERFACE_MAP_ENTRY(nsINavHistoryContainerResultNode)
> NS_INTERFACE_MAP_END_INHERITING(nsNavHistoryResultNode)
> 
> nsNavHistoryContainerResultNode::nsNavHistoryContainerResultNode(
>     const nsACString& aURI, const nsACString& aTitle,
>     const nsACString& aIconURI, PRUint32 aContainerType, PRBool aReadOnly,
>-    const nsACString& aRemoteContainerType) :
>+    const nsACString& aDynamicContainerType, nsNavHistoryQueryOptions* aOptions) :
>   nsNavHistoryResultNode(aURI, aTitle, 0, 0, aIconURI),
>   mResult(nsnull),
>   mContainerType(aContainerType),
>   mExpanded(PR_FALSE),
>   mChildrenReadOnly(aReadOnly),
>-  mRemoteContainerType(aRemoteContainerType)
>+  mDynamicContainerType(aDynamicContainerType),
>+  mOptions(aOptions)
> {
> }
> 
> 
> // nsNavHistoryContainerResultNode::OnRemoving
> //
> //    Containers should notify their children that they are being removed
> //    when the container is being removed.
>@@ -355,32 +354,30 @@ nsNavHistoryContainerResultNode::SetCont
> //    override this to do their own handling.
> 
> nsresult
> nsNavHistoryContainerResultNode::OpenContainer()
> {
>   NS_ASSERTION(! mExpanded, "Container must be expanded to close it");
>   mExpanded = PR_TRUE;
> 
>-  /* Untested container API functions
>-  if (! mRemoteContainerType.IsEmpty()) {
>-    // remote container API may want to fill us
>+  if (IsDynamicContainer()) {
>+    // dynamic container API may want to fill us
>     nsresult rv;
>-    nsCOMPtr<nsIRemoteContainer> remote = do_GetService(mRemoteContainerType.get(), &rv);
>+    nsCOMPtr<nsIDynamicContainer> svc = do_GetService(mDynamicContainerType.get(), &rv);
>     if (NS_SUCCEEDED(rv)) {
>-      remote->OnContainerOpening(this, GetGeneratingOptions());
>+      svc->OnContainerNodeOpening(this, GetGeneratingOptions());
>     } else {
>-      NS_WARNING("Unable to get remote container for ");
>-      NS_WARNING(mRemoteContainerType.get());
>+      NS_WARNING("Unable to get dynamic container for ");
>+      NS_WARNING(mDynamicContainerType.get());
>     }
>     PRInt32 oldAccessCount = mAccessCount;
>     FillStats();
>     ReverseUpdateStats(mAccessCount - oldAccessCount);
>   }
>-  */
> 
>   nsNavHistoryResult* result = GetResult();
>   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
>   if (result->GetView())
>     result->GetView()->ContainerOpened(this);
>   return NS_OK;
> }
> 
>@@ -400,25 +397,23 @@ nsNavHistoryContainerResultNode::CloseCo
>   // recursively close all child containers
>   for (PRInt32 i = 0; i < mChildren.Count(); i ++) {
>     if (mChildren[i]->IsContainer() && mChildren[i]->GetAsContainer()->mExpanded)
>       mChildren[i]->GetAsContainer()->CloseContainer(PR_FALSE);
>   }
> 
>   mExpanded = PR_FALSE;
> 
>-  /* Untested remote container functions
>   nsresult rv;
>-  if (! mRemoteContainerType.IsEmpty()) {
>-    // notify remote containers that we are closing
>-    nsCOMPtr<nsIRemoteContainer> remote = do_GetService(mRemoteContainerType.get(), &rv);
>+  if (IsDynamicContainer()) {
>+    // notify dynamic containers that we are closing
>+    nsCOMPtr<nsIDynamicContainer> svc = do_GetService(mDynamicContainerType.get(), &rv);
>     if (NS_SUCCEEDED(rv))
>-      remote->OnContainerClosed(this);
>+      svc->OnContainerNodeClosed(this);
>   }
>-  */
> 
>   if (aUpdateView) {
>     nsNavHistoryResult* result = GetResult();
>     NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
>     if (result->GetView())
>       result->GetView()->ContainerClosed(this);
>   }
>   return NS_OK;
>@@ -1373,30 +1368,16 @@ nsNavHistoryContainerResultNode::RemoveC
>   if (! aIsTemporary) {
>     ReverseUpdateStats(mAccessCount - oldAccessCount);
>     oldNode->OnRemoving();
>   }
>   return NS_OK;
> }
> 
> 
>-// nsNavHistoryContainerResultNode::CanRemoteContainersChange
>-//
>-//    Returns true if remote containers can manipulate the contents of this
>-//    container. This is false for folders and queries, true for everything
>-//    else.
>-
>-PRBool
>-nsNavHistoryContainerResultNode::CanRemoteContainersChange()
>-{
>-  return (mContainerType != nsNavHistoryResultNode::RESULT_TYPE_FOLDER &&
>-          mContainerType != nsNavHistoryResultNode::RESULT_TYPE_QUERY);
>-}
>-
>-
> // nsNavHistoryContainerResultNode::RecursiveFindURIs
> //
> //    This function searches for matches for the given URI.
> //
> //    If aOnlyOne is set, it will terminate as soon as it finds a single match.
> //    This would be used when there are URI results so there will only ever be
> //    one copy of any URI.
> //
>@@ -1616,134 +1597,129 @@ nsNavHistoryContainerResultNode::GetChil
> NS_IMETHODIMP
> nsNavHistoryContainerResultNode::GetChildrenReadOnly(PRBool *aChildrenReadOnly)
> {
>   *aChildrenReadOnly = mChildrenReadOnly;
>   return NS_OK;
> }
> 
> 
>-// nsNavHistoryContainerResultNode::GetRemoteContainerType
>+// nsNavHistoryContainerResultNode::GetDynamicContainerType
> 
> NS_IMETHODIMP
>-nsNavHistoryContainerResultNode::GetRemoteContainerType(
>-    nsACString& aRemoteContainerType)
>+nsNavHistoryContainerResultNode::GetDynamicContainerType(
>+    nsACString& aDynamicContainerType)
> {
>-  aRemoteContainerType = mRemoteContainerType;
>+  aDynamicContainerType = mDynamicContainerType;
>   return NS_OK;
> }
> 
> 
> // nsNavHistoryContainerResultNode::AppendURINode
> 
>-#if 0 // UNTESTED, commented out until it can be tested
> NS_IMETHODIMP
> nsNavHistoryContainerResultNode::AppendURINode(
>     const nsACString& aURI, const nsACString& aTitle, PRUint32 aAccessCount,
>     PRTime aTime, const nsACString& aIconURI, nsINavHistoryResultNode** _retval)
> {
>   *_retval = nsnull;
>-  if (mRemoteContainerType.IsEmpty() || ! CanRemoteContainersChange())
>-    return NS_ERROR_INVALID_ARG; // we must be a remote container
>+  if (!IsDynamicContainer())
>+    return NS_ERROR_INVALID_ARG; // we must be a dynamic container
> 
>   nsRefPtr<nsNavHistoryResultNode> result =
>       new nsNavHistoryResultNode(aURI, aTitle, aAccessCount, aTime, aIconURI);
>   NS_ENSURE_TRUE(result, NS_ERROR_OUT_OF_MEMORY);
> 
>   // append to our list
>-  if (! mChildren.AppendObject(result))
>-    return NS_ERROR_OUT_OF_MEMORY;
>+  nsresult rv = InsertChildAt(result, mChildren.Count());
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   NS_ADDREF(*_retval = result);
>   return NS_OK;
> }
>-#endif
> 
> 
> // nsNavHistoryContainerResultNode::AppendVisitNode
> 
> #if 0 // UNTESTED, commented out until it can be tested
> NS_IMETHODIMP
> nsNavHistoryContainerResultNode::AppendVisitNode(
>     const nsACString& aURI, const nsACString& aTitle, PRUint32 aAccessCount,
>     PRTime aTime, const nsACString& aIconURI, PRInt64 aSession,
>     nsINavHistoryVisitResultNode** _retval)
> {
>   *_retval = nsnull;
>-  if (mRemoteContainerType.IsEmpty() || ! CanRemoteContainersChange())
>-    return NS_ERROR_INVALID_ARG; // we must be a remote container
>+  if (!IsDynamicContainer())
>+    return NS_ERROR_INVALID_ARG; // we must be a dynamic container
> 
>   nsRefPtr<nsNavHistoryVisitResultNode> result =
>       new nsNavHistoryVisitResultNode(aURI, aTitle, aAccessCount, aTime,
>                                       aIconURI, aSession);
>   NS_ENSURE_TRUE(result, NS_ERROR_OUT_OF_MEMORY);
> 
>   // append to our list
>   if (! mChildren.AppendObject(result))
>     return NS_ERROR_OUT_OF_MEMORY;
>   NS_ADDREF(*_retval = result);
>   return NS_OK;
> }
>-#endif
> 
> 
> // nsNavHistoryContainerResultNode::AppendFullVisitNode
> 
>-#if 0 // UNTESTED, commented out until it can be tested
> NS_IMETHODIMP
> nsNavHistoryContainerResultNode::AppendFullVisitNode(
>     const nsACString& aURI, const nsACString& aTitle, PRUint32 aAccessCount,
>     PRTime aTime, const nsACString& aIconURI, PRInt64 aSession,
>     PRInt64 aVisitId, PRInt64 aReferringVisitId, PRInt32 aTransitionType,
>     nsINavHistoryFullVisitResultNode** _retval)
> {
>   *_retval = nsnull;
>-  if (mRemoteContainerType.IsEmpty() || ! CanRemoteContainersChange())
>-    return NS_ERROR_INVALID_ARG; // we must be a remote container
>+  if (!IsDynamicContainer())
>+    return NS_ERROR_INVALID_ARG; // we must be a dynamic container
> 
>   nsRefPtr<nsNavHistoryFullVisitResultNode> result =
>       new nsNavHistoryFullVisitResultNode(aURI, aTitle, aAccessCount, aTime,
>                                           aIconURI, aSession, aVisitId,
>                                           aReferringVisitId, aTransitionType);
>   NS_ENSURE_TRUE(result, NS_ERROR_OUT_OF_MEMORY);
> 
>   // append to our list
>   if (! mChildren.AppendObject(result))
>     return NS_ERROR_OUT_OF_MEMORY;
>   NS_ADDREF(*_retval = result);
>   return NS_OK;
> }
>-#endif
> 
> 
> // nsNavHistoryContainerResultNode::AppendContainerNode
> 
>-#if 0 // UNTESTED, commented out until it can be tested
> NS_IMETHODIMP
> nsNavHistoryContainerResultNode::AppendContainerNode(
>     const nsACString& aTitle, const nsACString& aIconURI,
>-    PRUint32 aContainerType, const nsACString& aRemoteContainerType,
>+    PRUint32 aContainerType, const nsACString& aDynamicContainerType,
>     nsINavHistoryContainerResultNode** _retval)
> {
>   *_retval = nsnull;
>-  if (mRemoteContainerType.IsEmpty() || ! CanRemoteContainersChange())
>-    return NS_ERROR_INVALID_ARG; // we must be a remote container
>+  if (!IsDynamicContainer())
>+    return NS_ERROR_INVALID_ARG; // we must be a dynamic container
>   if (! IsTypeContainer(aContainerType) || IsTypeFolder(aContainerType) ||
>       IsTypeQuery(aContainerType))
>     return NS_ERROR_INVALID_ARG; // not proper container type
>-  if (aContainerType == nsNavHistoryResultNode::RESULT_TYPE_REMOTE_CONTAINER &&
>+  if (aContainerType == nsNavHistoryResultNode::RESULT_TYPE_DYNAMIC_CONTAINER &&
>       aRemoteContainerType.IsEmpty())
>-    return NS_ERROR_INVALID_ARG; // remote containers must have r.c. type
>-  if (aContainerType != nsNavHistoryResultNode::RESULT_TYPE_REMOTE_CONTAINER &&
>-      ! aRemoteContainerType.IsEmpty())
>-    return NS_ERROR_INVALID_ARG; // non-remote containers must NOT have r.c. type
>+    return NS_ERROR_INVALID_ARG; // dynamic containers must have r.c. type
>+  if (aContainerType != nsNavHistoryResultNode::RESULT_TYPE_DYNAMIC_CONTAINER &&
>+      ! aDynamicContainerType.IsEmpty())
>+    return NS_ERROR_INVALID_ARG; // non-dynamic containers must NOT have r.c. type

nit: s/r.c./d.c./ or something clearer

>+
>+// main
>+function run_test() {
>+  do_load_module("/toolkit/components/places/tests/unit/nsDynamicContainerServiceSample.js");
>+  var testRoot = bmsvc.createFolder(bmsvc.placesRoot, "test root", bmsvc.DEFAULT_INDEX);
>+  var exposedFolder = bmsvc.createFolder(testRoot, "exposed folder", bmsvc.DEFAULT_INDEX);
>+  var efId1 = bmsvc.insertBookmark(exposedFolder, uri("http://uri1.tld"), bmsvc.DEFAULT_INDEX, "");
>+
>+  // create our dynamic container
>+  var remoteContainer =
>+    bmsvc.createDynamicContainer(testRoot, "remote container sample",
>+                                "@mozilla.org/browser/remote-container-sample;1",
>+                                bmsvc.DEFAULT_INDEX);
>+
>+  annosvc.setItemAnnotation(remoteContainer, "exposedFolder",
>+                            exposedFolder, 0, 0);
>+  var options = histsvc.getNewQueryOptions();
>+  var query = histsvc.getNewQuery();
>+  query.setFolders([remoteContainer], 1);
>+  var result = histsvc.executeQuery(query, options);
>+  var rootNode = result.root;
>+  rootNode.containerOpen = true;
>+  do_check_eq(rootNode.childCount, 2);
>+  do_check_eq(rootNode.getChild(0).uri, "http://foo.tld/");
>+  var folder = rootNode.getChild(1).QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+  do_check_eq(folder.itemId, exposedFolder);
>+  folder.containerOpen = true;
>+  do_check_eq(folder.childCount, 1);
>+  // check live update of a folder exposed within a remote container
>+  bmsvc.insertBookmark(exposedFolder, uri("http://uri2.tld"), bmsvc.DEFAULT_INDEX, "");
>+  do_check_eq(folder.childCount, 2);
>+}

please add some notes explaining what this test is doing, and what the expected results are

also, please file a followup on supporting import/export of remote containers.

r=me otherwise

Comment 11

[Asaf Romano (gone)]

> does this observer need to be removed at some point?

no, observers added onto a service are removed automatically removed on xpcom-shutdown.

Comment 12

[Asaf Romano (gone)]

a/are removed/are/

Comment 13

[Asaf Romano (gone)]

Attachment: part 1, for checkin

Comment 14

[Asaf Romano (gone)]

part 1:
mozilla/browser/components/places/content/utils.js 1.53
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.28
mozilla/toolkit/components/places/public/Makefile.in 1.11
mozilla/toolkit/components/places/public/nsIDynamicContainer.idl initial revision: 1.1
mozilla/toolkit/components/places/public/nsILivemarkService.idl 1.8
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.47
mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.65
mozilla/toolkit/components/places/public/nsIRemoteContainer.idl delete
mozilla/toolkit/components/places/src/nsLivemarkService.js 1.20
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.116
mozilla/toolkit/components/places/src/nsNavBookmarks.h 1.46
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.151
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.109
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.45
mozilla/toolkit/components/places/tests/bookmarks/test_livemarks.js 1.2
mozilla/toolkit/components/places/tests/unit/nsDynamicContainerServiceSample.js 
initial revision: 1.1
mozilla/toolkit/components/places/tests/unit/test_dynamic_containers.js 
initial revision: 1.1

Comment 15

[Dietrich Ayala (:dietrich)]

this may have caused increase in rlk on bm-xserve11:

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsStringBundle                                   36          -
nsNavBookmarks                                  164          -
nsXPCWrappedJSClass                              44          -
nsRunnable                                       12          -
XPCWrappedNative                                336          -
XPCNativeScriptableShared                       108          -
xptiInterfaceInfo                                20          -
nsAnnotationService                              80          -
BackstagePass                                    24          -
nsXPCWrappedJS                                   60          -
XPCWrappedNativeProto                           112          -
mozStorageStatement                            1296    1250.00%
nsVoidArray                                     136     325.00%
nsStringBuffer                                 1264     203.85%
mozStorageConnection                            128     100.00%
nsStandardURL                                   704     100.00%
nsTArray_base                                    12      50.00%
nsLocalFile                                     336      50.00%
nsWeakReference   

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> this may have caused increase in rlk on bm-xserve11:
> 
> --NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
> nsStringBundle                                   36          -
> nsNavBookmarks                                  164          -
> nsXPCWrappedJSClass                              44          -
> nsRunnable                                       12          -
> XPCWrappedNative                                336          -
> XPCNativeScriptableShared                       108          -
> xptiInterfaceInfo                                20          -
> nsAnnotationService                              80          -
> BackstagePass                                    24          -
> nsXPCWrappedJS                                   60          -
> XPCWrappedNativeProto                           112          -
> mozStorageStatement                            1296    1250.00%
> nsVoidArray                                     136     325.00%
> nsStringBuffer                                 1264     203.85%
> mozStorageConnection                            128     100.00%
> nsStandardURL                                   704     100.00%
> nsTArray_base                                    12      50.00%
> nsLocalFile                                     336      50.00%
> nsWeakReference   
> 

this was fixed in bug 390706.

mano, what else needs to happen in this bug?

Comment 17

[Asaf Romano (gone)]

Still need to implement the commented out methods....

Comment 18

[Dietrich Ayala (:dietrich)]

pushing. i don't see why this is blocking M9. please comment and re-target if you think otherwise.

Comment 19

[Asaf Romano (gone)]

Re-targeting to M9 and closing, I will file follow ups as needed.

Comment 20

[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