Bug 472343 (Part 3) - Merge history and bookmarks batchers.

r=dietrich a=blocking

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -126,8 +126,6 @@ nsNavBookmarks::nsNavBookmarks() : mItemCount(0)
                                  , mBookmarksRoot(0)
                                  , mTagRoot(0)
                                  , mToolbarFolder(0)
-                                 , mBatchLevel(0)
-                                 , mBatchDBTransaction(nsnull)
                                  , mCanNotify(false)
                                  , mCacheObservers("bookmark-observers")
                                  , mShuttingDown(false)
@@ -2965,48 +2963,20 @@ nsNavBookmarks::EnsureKeywordsHash() {
 }
 
 
-// See RunInBatchMode
-nsresult
-nsNavBookmarks::BeginUpdateBatch()
-{
-  if (mBatchLevel++ == 0) {
-    mBatchDBTransaction = new mozStorageTransaction(mDBConn, PR_FALSE);
-
-    NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
-                     nsINavBookmarkObserver, OnBeginUpdateBatch());
-  }
-  return NS_OK;
-}
-
-
-nsresult
-nsNavBookmarks::EndUpdateBatch()
-{
-  if (--mBatchLevel == 0) {
-    if (mBatchDBTransaction) {
-      nsresult rv = mBatchDBTransaction->Commit();
-      NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Batch failed to commit transaction");
-      delete mBatchDBTransaction;
-      mBatchDBTransaction = nsnull;
-    }
-
-    NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
-                     nsINavBookmarkObserver, OnEndUpdateBatch());
-  }
-  return NS_OK;
-}
-
-
 NS_IMETHODIMP
 nsNavBookmarks::RunInBatchMode(nsINavHistoryBatchCallback* aCallback,
                                nsISupports* aUserData) {
   NS_ENSURE_ARG(aCallback);
 
-  BeginUpdateBatch();
-  nsresult rv = aCallback->RunBatched(aUserData);
-  EndUpdateBatch();
+  // Just forward the request to history.  History service must exist for
+  // bookmarks to work and we are observing it, thus batch notifications will be
+  // forwarded to bookmarks observers.
+  nsNavHistory* history = nsNavHistory::GetHistoryService();
+  NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
+  nsresult rv = history->RunInBatchMode(aCallback, aUserData);
+  NS_ENSURE_SUCCESS(rv, rv);
 
-  return rv;
+  return NS_OK;
 }
 
 
@@ -3031,7 +3001,8 @@ nsNavBookmarks::RemoveObserver(nsINavBookmarkObserver* aObserver)
 NS_IMETHODIMP
 nsNavBookmarks::OnBeginUpdateBatch()
 {
-  // These aren't passed through to bookmark observers currently.
+  NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
+                   nsINavBookmarkObserver, OnBeginUpdateBatch());
   return NS_OK;
 }
 
@@ -3039,7 +3010,8 @@ nsNavBookmarks::OnBeginUpdateBatch()
 NS_IMETHODIMP
 nsNavBookmarks::OnEndUpdateBatch()
 {
-  // These aren't passed through to bookmark observers currently.
+  NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
+                   nsINavBookmarkObserver, OnEndUpdateBatch());
   return NS_OK;
 }
 

toolkit/components/places/src/nsNavBookmarks.h

@@ -161,9 +161,6 @@ public:
    */
   PRBool IsRealBookmark(PRInt64 aPlaceId);
 
-  nsresult BeginUpdateBatch();
-  nsresult EndUpdateBatch();
-
   PRBool ItemExists(PRInt64 aItemId);
 
   /**
@@ -183,10 +180,6 @@ public:
     return nsnull;
   }
 
-  bool isBatching() {
-    return mBatchLevel > 0;
-  }
-
 private:
   static nsNavBookmarks* gBookmarksService;
 
@@ -238,11 +231,6 @@ private:
   // personal toolbar folder
   PRInt64 mToolbarFolder;
 
-  // The level of batches' nesting, 0 when no batches are open.
-  PRInt32 mBatchLevel;
-  // Current active transaction for a batch.
-  mozStorageTransaction* mBatchDBTransaction;
-
   nsresult GetParentAndIndexOfFolder(PRInt64 aFolder,
                                      PRInt64* aParent,
                                      PRInt32* aIndex);
@@ -469,21 +457,4 @@ private:
   nsresult UpdateKeywordsHashForRemovedBookmark(PRInt64 aItemId);
 };
 
-struct nsBookmarksUpdateBatcher
-{
-  nsBookmarksUpdateBatcher()
-  {
-    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
-    if (bookmarks)
-      bookmarks->BeginUpdateBatch();
-  }
-  ~nsBookmarksUpdateBatcher()
-  {
-    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
-    if (bookmarks)
-      bookmarks->EndUpdateBatch();
-  }
-};
-
-
 #endif // nsNavBookmarks_h_

toolkit/components/places/src/nsNavHistory.cpp

@@ -3054,10 +3054,9 @@ nsNavHistory::ExecuteQueries(nsINavHistoryQuery** aQueries, PRUint32 aQueryCount
   }
 
   // Create the result that will hold nodes.  Inject batching status into it.
-  bool batchInProgress = isBatching() || bookmarks->isBatching();
   nsRefPtr<nsNavHistoryResult> result;
   rv = nsNavHistoryResult::NewHistoryResult(aQueries, aQueryCount, options,
-                                            rootNode, batchInProgress,
+                                            rootNode, isBatching(),
                                             getter_AddRefs(result));
   NS_ENSURE_SUCCESS(rv, rv);
 

toolkit/components/places/src/nsNavHistoryResult.cpp

@@ -3182,7 +3182,8 @@ nsNavHistoryQueryResultNode::OnItemVisited(PRInt64 aItemId,
   // for bookmark queries, "all bookmark" observer should get OnItemVisited
   // but it is ignored.
   if (mLiveUpdate != QUERYUPDATE_COMPLEX_WITH_BOOKMARKS)
-    NS_WARNING("history observers should not get OnItemVisited, but should get OnVisit instead");
+    NS_WARN_IF_FALSE(mResult && (mResult->mIsAllBookmarksObserver || mResult->mIsBookmarkFolderObserver),
+                     "history observers should not get OnItemVisited, but should get OnVisit instead");
   return NS_OK;
 }
 
@@ -4370,11 +4371,12 @@ nsNavHistoryResult::AddHistoryObserver(nsNavHistoryQueryResultNode* aNode)
       history->AddObserver(this, PR_TRUE);
       mIsHistoryObserver = PR_TRUE;
   }
-  if (mHistoryObservers.IndexOf(aNode) != mHistoryObservers.NoIndex) {
-    NS_WARNING("Attempting to register as a history observer twice!");
-    return;
+  // Don't add duplicate observers.  In some case we don't unregister when
+  // children are cleared (see ClearChildren) and the next FillChildren call
+  // will try to add the observer again.
+  if (mHistoryObservers.IndexOf(aNode) == mHistoryObservers.NoIndex) {
+    mHistoryObservers.AppendElement(aNode);
   }
-  mHistoryObservers.AppendElement(aNode);
 }
 
 
@@ -4390,11 +4392,12 @@ nsNavHistoryResult::AddAllBookmarksObserver(nsNavHistoryQueryResultNode* aNode)
     bookmarks->AddObserver(this, PR_TRUE);
     mIsAllBookmarksObserver = PR_TRUE;
   }
-  if (mAllBookmarksObservers.IndexOf(aNode) != mAllBookmarksObservers.NoIndex) {
-    NS_WARNING("Attempting to register an all bookmarks observer twice!");
-    return;
+  // Don't add duplicate observers.  In some case we don't unregister when
+  // children are cleared (see ClearChildren) and the next FillChildren call
+  // will try to add the observer again.
+  if (mAllBookmarksObservers.IndexOf(aNode) == mAllBookmarksObservers.NoIndex) {
+    mAllBookmarksObservers.AppendElement(aNode);
   }
-  mAllBookmarksObservers.AppendElement(aNode);
 }
 
 
@@ -4411,13 +4414,13 @@ nsNavHistoryResult::AddBookmarkFolderObserver(nsNavHistoryFolderResultNode* aNod
     bookmarks->AddObserver(this, PR_TRUE);
     mIsBookmarkFolderObserver = PR_TRUE;
   }
-
+  // Don't add duplicate observers.  In some case we don't unregister when
+  // children are cleared (see ClearChildren) and the next FillChildren call
+  // will try to add the observer again.
   FolderObserverList* list = BookmarkFolderObserversForId(aFolder, PR_TRUE);
-  if (list->IndexOf(aNode) != list->NoIndex) {
-    NS_NOTREACHED("Attempting to register as a folder observer twice!");
-    return;
+  if (list->IndexOf(aNode) == list->NoIndex) {
+    list->AppendElement(aNode);
   }
-  list->AppendElement(aNode);
 }
 
 

toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js

@@ -38,18 +38,9 @@
  * ***** END LICENSE BLOCK ***** */
 
 // Get history service
-try {
-  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
-} catch(ex) {
-  do_throw("Could not get history service\n");
-} 
-
-// Get global history service
-try {
-  var bhist = Cc["@mozilla.org/browser/global-history;2"].getService(Ci.nsIBrowserHistory);
-} catch(ex) {
-  do_throw("Could not get history service\n");
-} 
+var histsvc = PlacesUtils.history;
+var bhist = PlacesUtils.bhistory;
+var bmsvc = PlacesUtils.bookmarks;
 
 // adds a test URI visit to the database, and checks for a valid place ID
 function add_visit(aURI, aDate) {
@@ -195,6 +186,12 @@ function run_test() {
     }
   }, null);
   do_check_false(resultObserver.inBatchMode);
+  bmsvc.runInBatchMode({
+    runBatched: function (aUserData) {
+      do_check_true(resultObserver.inBatchMode);
+    }
+  }, null);
+  do_check_false(resultObserver.inBatchMode);
 
   // nsINavHistoryResultObserver.containerClosed
   root.containerOpen = false;
@@ -206,12 +203,6 @@ function run_test() {
   // Reset the result observer.
   resultObserver.reset();
 
-  try {
-    var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].getService(Ci.nsINavBookmarksService);
-  } catch(ex) {
-    do_throw("Could not get nav-bookmarks-service\n");
-  }
-
   var options = histsvc.getNewQueryOptions();
   var query = histsvc.getNewQuery();
   query.setFolders([bmsvc.bookmarksMenuFolder], 1);
@@ -259,6 +250,12 @@ function run_test() {
 
   // nsINavHistoryResultObserver.batching
   do_check_false(resultObserver.inBatchMode);
+  histsvc.runInBatchMode({
+    runBatched: function (aUserData) {
+      do_check_true(resultObserver.inBatchMode);
+    }
+  }, null);
+  do_check_false(resultObserver.inBatchMode);
   bmsvc.runInBatchMode({
     runBatched: function (aUserData) {
       do_check_true(resultObserver.inBatchMode);