Bug 332143 r=brettw sr=beng (checkin for pamg) New livemarks don't get proper icon in toolbar.

browser/components/places/public/nsINavBookmarksService.idl

@@ -200,7 +200,7 @@ interface nsINavBookmarkObserver : nsISupports
  * folders.  A URI in history can be contained in one or more such folders.
  */
 
-[scriptable, uuid(df93900d-7ef1-4c5f-8e44-a930aeaf1462)]
+[scriptable, uuid(860d786d-9bba-4011-a396-486a87af8f07)]
 interface nsINavBookmarksService : nsISupports
 {
   /**
@@ -264,12 +264,12 @@ interface nsINavBookmarksService : nsISupports
    * parent and sets the container type.
    *  @param parent     The id of the parent folder
    *  @param name       The name of the new folder
-   *  @param index      The index to insert at, or -1 to append
    *  @param type       The type of container to insert
+   *  @param index      The index to insert at, or -1 to append
    *  @returns the ID of the newly-inserted folder
    */
   PRInt64 createContainer(in PRInt64 parent, in AString name,
-                          in PRInt32 index, in AString type);
+                          in AString type, in PRInt32 index);
 
   /**
    * Removes a folder from the bookmarks tree.

browser/components/places/src/nsLivemarkService.cpp

@@ -260,9 +260,9 @@ nsLivemarkService::CreateLivemark(PRInt64 aFolder,
 {
   // Create the livemark as a bookmark container
   nsNavBookmarks *bookmarks = nsNavBookmarks::GetBookmarksService();
-  nsresult rv = bookmarks->CreateContainer(aFolder, aName, aIndex,
+  nsresult rv = bookmarks->CreateContainer(aFolder, aName,
                                            NS_LITERAL_STRING(NS_LIVEMARKSERVICE_CONTRACTID),
-                                           aNewLivemark);
+                                           aIndex, aNewLivemark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Get the livemark URI

browser/components/places/src/nsNavBookmarks.cpp

@@ -970,30 +970,36 @@ NS_IMETHODIMP
 nsNavBookmarks::CreateFolder(PRInt64 aParent, const nsAString &aName,
                              PRInt32 aIndex, PRInt64 *aNewFolder)
 {
-  return CreateFolderWithID(-1, aParent, aName, aIndex, aNewFolder);
+  // CreateFolderWithID returns the index of the new folder, but that's not
+  // used here.  To avoid any risk of corrupting data should this function
+  // be changed, we'll use a local variable to hold it.  The PR_TRUE argument
+  // will cause notifications to be sent to bookmark observers.
+  PRInt32 localIndex = aIndex;
+  return CreateFolderWithID(-1, aParent, aName, PR_TRUE, &localIndex, aNewFolder);
 }
 
 NS_IMETHODIMP
 nsNavBookmarks::CreateContainer(PRInt64 aParent, const nsAString &aName,
-                                PRInt32 aIndex, const nsAString &aType,
+                                const nsAString &aType, PRInt32 aIndex,
                                 PRInt64 *aNewFolder)
 {
-  return CreateContainerWithID(-1, aParent, aName, aIndex, aType, aNewFolder);
+  return CreateContainerWithID(-1, aParent, aName, aType, aIndex, aNewFolder);
 }
 
 nsresult
 nsNavBookmarks::CreateFolderWithID(PRInt64 aFolder, PRInt64 aParent,
-                                   const nsAString& aName, PRInt32 aIndex,
+                                   const nsAString& aName,
-                                   PRInt64* aNewFolder)
+                                   PRBool aSendNotifications,
+                                   PRInt32* aIndex, PRInt64* aNewFolder)
 {
   // You can pass -1 to indicate append, but no other negative number is allowed
-  if (aIndex < -1)
+  if (*aIndex < -1)
     return NS_ERROR_INVALID_ARG;
 
   mozIStorageConnection *dbConn = DBConn();
   mozStorageTransaction transaction(dbConn, PR_FALSE);
 
-  PRInt32 index = (aIndex == -1) ? FolderCount(aParent) : aIndex;
+  PRInt32 index = (*aIndex == -1) ? FolderCount(aParent) : *aIndex;
 
   nsresult rv = AdjustIndices(aParent, index, PR_INT32_MAX, 1);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -1040,20 +1046,31 @@ nsNavBookmarks::CreateFolderWithID(PRInt64 aFolder, PRInt64 aParent,
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
-  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+  // When creating a livemark container, we need to delay sending notifications
-                      OnFolderAdded(child, aParent, index))
+  // until the container type has been set.  In that case, they'll be sent by
+  // CreateContainerWithID rather than here.
+  if (aSendNotifications) {
+    ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+                        OnFolderAdded(child, aParent, index))
+  }
 
+  *aIndex = index;
   *aNewFolder = child;
   return NS_OK;
 }
 
 nsresult 
 nsNavBookmarks::CreateContainerWithID(PRInt64 aFolder, PRInt64 aParent, 
-                                      const nsAString &aName, PRInt32 aIndex,
+                                      const nsAString &aName, const nsAString &aType, 
-                                      const nsAString &aType, PRInt64 *aNewFolder)
+                                      PRInt32 aIndex, PRInt64 *aNewFolder)
 {
   // Containers are wrappers around read-only folders, with a specific type.
-  nsresult rv = CreateFolderWithID(aFolder, aParent, aName, aIndex, aNewFolder);
+  // CreateFolderWithID will return the index of the newly created folder,
+  // which we will need later on in order to send notifications.  The PR_FALSE
+  // argument disables sending notifiactions, since we need to defer that until
+  // the folder type has been set.
+  PRInt32 localIndex = aIndex;
+  nsresult rv = CreateFolderWithID(aFolder, aParent, aName, PR_FALSE, &localIndex, aNewFolder);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Set the type.
@@ -1070,6 +1087,10 @@ nsNavBookmarks::CreateContainerWithID(PRInt64 aFolder, PRInt64 aParent,
   rv = statement->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  // Send notifications after folder type has been set.
+  ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
+                      OnFolderAdded(*aNewFolder, aParent, localIndex))
+
   return NS_OK;
 }
 

browser/components/places/src/nsNavBookmarks.h

@@ -82,13 +82,22 @@ public:
                                nsNavHistoryQueryOptions *aOptions,
                                nsCOMArray<nsNavHistoryResultNode> *children);
 
-  // If aFolder is -1, use the autoincrement id for folder index. 
+  // 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, PRInt32 aIndex,
+                              const nsAString& title,
-                              PRInt64* aNewFolder);
+                              PRBool aSendNotifications,
-  nsresult CreateContainerWithID(PRInt64 aFolder, PRInt64 aParent, 
+                              PRInt32 *aIndex, PRInt64* aNewFolder);
-                                 const nsAString& title, PRInt32 aIndex, 
+
-                                 const nsAString& type, 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);
 
   // Returns a statement to get information about a folder id
   mozIStorageStatement* DBGetFolderInfo() { return mDBGetFolderInfo; }
@@ -197,10 +206,12 @@ private:
     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, mIndex, &newFolder);
+        return bookmarks->CreateFolderWithID(mID, mParent, mTitle, PR_TRUE, &mIndex, &newFolder);
       nsAutoString type; type.AssignWithConversion(mType);
-      return bookmarks->CreateContainerWithID(mID, mParent, mTitle, mIndex, type, &newFolder); 
+      return bookmarks->CreateContainerWithID(mID, mParent, mTitle, type, mIndex, &newFolder); 
     }
 
     NS_IMETHOD RedoTransaction() {