backing out bug 432706, causes multiple regressions
authorDietrich Ayala <dietrich@mozilla.com>
Wed, 04 Feb 2009 09:49:58 -0800
changeset 24582 5977e010b3db48fe852347f49eb04f23fb2f3ca0
parent 24581 ddcb3ca67b5eec9c7ca522811834709d09cb8d2a
child 24583 e646709ecfc55cd8763775773158ef7dbe20e052
push id5126
push userdietrich@mozilla.com
push dateWed, 04 Feb 2009 17:50:21 +0000
treeherdermozilla-central@5977e010b3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs432706
milestone1.9.2a1pre
backing out bug 432706, causes multiple regressions
toolkit/components/places/src/nsNavHistoryResult.cpp
toolkit/components/places/src/nsNavHistoryResult.h
toolkit/components/places/tests/bookmarks/test_432706_in_batch.js
toolkit/components/places/tests/bookmarks/test_disconnected_nodes.js
--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
@@ -181,17 +181,17 @@ nsNavHistoryResultNode::GetTags(nsAStrin
     aTags.Truncate();
     return NS_OK;
   }
 
   // Initially, the tags string is set to a void string (see constructor). We
   // then build it the first time this method called is called (and by that,
   // implicitly unset the void flag). Result observers may re-set the void flag
   // in order to force rebuilding of the tags string.
-  if (!mTags.IsVoid() && mParent) {
+  if (!mTags.IsVoid()) {
     aTags.Assign(mTags);
     return NS_OK;
   }
 
   // Fetch the tags
   nsNavHistory* history = nsNavHistory::GetHistoryService();
   NS_ENSURE_STATE(history);
   mozIStorageStatement* getTagsStatement = history->DBGetTags();
@@ -2974,17 +2974,16 @@ nsNavHistoryFolderResultNode::nsNavHisto
     const nsACString& aTitle, nsNavHistoryQueryOptions* aOptions,
     PRInt64 aFolderId, const nsACString& aDynamicContainerType) :
   nsNavHistoryContainerResultNode(EmptyCString(), aTitle, EmptyCString(),
                                   nsNavHistoryResultNode::RESULT_TYPE_FOLDER,
                                   PR_FALSE, aDynamicContainerType, aOptions),
   mContentsValid(PR_FALSE),
   mQueryItemId(-1),
   mIsRegisteredFolderObserver(PR_FALSE)
-, mBatchInProgress(PR_FALSE)
 {
   mItemId = aFolderId;
 }
 
 nsNavHistoryFolderResultNode::~nsNavHistoryFolderResultNode()
 {
   if (mIsRegisteredFolderObserver && mResult)
     mResult->RemoveBookmarkFolderObserver(this, mItemId);
@@ -3367,49 +3366,38 @@ nsNavHistoryFolderResultNode::FindChildB
 }
 
 
 // nsNavHistoryFolderResultNode::OnBeginUpdateBatch (nsINavBookmarkObserver)
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnBeginUpdateBatch()
 {
-  mBatchInProgress = PR_TRUE;
   return NS_OK;
 }
 
 
 // nsNavHistoryFolderResultNode::OnEndUpdateBatch (nsINavBookmarkObserver)
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnEndUpdateBatch()
 {
-  if (mBatchInProgress) {
-    mBatchInProgress = PR_FALSE;
-    nsresult rv = Refresh();
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-  else
-    NS_WARNING("EndUpdateBatch without a begin");
   return NS_OK;
 }
 
 
 // nsNavHistoryFolderResultNode::OnItemAdded (nsINavBookmarkObserver)
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemAdded(PRInt64 aItemId,
                                           PRInt64 aParentFolder,
                                           PRInt32 aIndex)
 {
   NS_ASSERTION(aParentFolder == mItemId, "Got wrong bookmark update");
 
-  if (mBatchInProgress)
-    return NS_OK;
-
   // here, try to do something reasonable if the bookmark service gives us
   // a bogus index.
   if (aIndex < 0) {
     NS_NOTREACHED("Invalid index for item adding: <0");
     aIndex = 0;
   } else if (aIndex > mChildren.Count()) {
     NS_NOTREACHED("Invalid index for item adding: greater than count");
     aIndex = mChildren.Count();
@@ -3479,19 +3467,16 @@ nsNavHistoryFolderResultNode::OnItemAdde
 
 
 // nsNavHistoryFolderResultNode::OnItemRemoved (nsINavBookmarkObserver)
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemRemoved(PRInt64 aItemId,
                                             PRInt64 aParentFolder, PRInt32 aIndex)
 {
-  if (mBatchInProgress)
-    return NS_OK;
-
   // We only care about notifications when a child changes. When the deleted
   // item is us, our parent should also be registered and will remove us from
   // its list.
   if (mItemId == aItemId)
     return NS_OK;
 
   // don't trust the index from the bookmark service, find it ourselves. The
   // sorting could be different, or the bookmark services indices and ours might
@@ -3597,19 +3582,16 @@ nsNavHistoryResultNode::OnItemChanged(PR
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemChanged(PRInt64 aItemId,
                                             const nsACString& aProperty,
                                             PRBool aIsAnnotationProperty,
                                             const nsACString& aValue) {
-  if (mBatchInProgress)
-    return NS_OK;
-
   // The query-item's title is used for simple-query nodes
   if (mQueryItemId != -1) {
     PRBool isTitleChange = aProperty.EqualsLiteral("title");
     if ((mQueryItemId == aItemId && !isTitleChange) ||
         (mQueryItemId != aItemId && isTitleChange)) {
       return NS_OK;
     }
   }
@@ -3622,19 +3604,16 @@ nsNavHistoryFolderResultNode::OnItemChan
 // nsNavHistoryFolderResultNode::OnItemVisited (nsINavBookmarkObserver)
 //
 //    Update visit count and last visit time and refresh.
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemVisited(PRInt64 aItemId,
                                             PRInt64 aVisitId, PRTime aTime)
 {
-  if (mBatchInProgress)
-    return NS_OK;
-
   if (mOptions->ExcludeItems())
     return NS_OK; // don't update items when we aren't displaying them
   if (! StartIncrementalUpdate())
     return NS_OK;
 
   PRUint32 nodeIndex;
   nsNavHistoryResultNode* node = FindChildById(aItemId, &nodeIndex);
   if (! node)
@@ -3676,20 +3655,16 @@ nsNavHistoryFolderResultNode::OnItemVisi
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemMoved(PRInt64 aItemId, PRInt64 aOldParent,
                                           PRInt32 aOldIndex, PRInt64 aNewParent,
                                           PRInt32 aNewIndex)
 {
   NS_ASSERTION(aOldParent == mItemId || aNewParent == mItemId,
                "Got a bookmark message that doesn't belong to us");
-
-  if (mBatchInProgress)
-    return NS_OK;
-
   if (! StartIncrementalUpdate())
     return NS_OK; // entire container was refreshed for us
 
   if (aOldParent == aNewParent) {
     // getting moved within the same folder, we don't want to do a remove and
     // an add because that will lose your tree state.
 
     // adjust bookmark indices
@@ -4097,51 +4072,30 @@ nsNavHistoryResult::GetRoot(nsINavHistor
   }
   return mRootNode->QueryInterface(NS_GET_IID(nsINavHistoryContainerResultNode),
                                    reinterpret_cast<void**>(aRoot));
 }
 
 
 // nsINavBookmarkObserver implementation
 
-PR_STATIC_CALLBACK(PLDHashOperator)
-FolderObserverEnumerator(nsTrimInt64HashKey::KeyType,
-                         nsNavHistoryResult::FolderObserverList *aList,
-                         void *aData)
-{
-  nsNavHistoryResult::FolderObserverList *list =
-    static_cast<nsNavHistoryResult::FolderObserverList *>(aData);
-  (void)list->AppendElements(*aList);
-  
-  return PL_DHASH_NEXT;
-}
-
 // Here, it is important that we create a COPY of the observer array. Some
 // observers will requery themselves, which may cause the observer array to
 // be modified or added to.
 #define ENUMERATE_BOOKMARK_FOLDER_OBSERVERS(_folderId, _functionCall) \
   { \
     FolderObserverList* _fol = BookmarkFolderObserversForId(_folderId, PR_FALSE); \
     if (_fol) { \
       FolderObserverList _listCopy(*_fol); \
       for (PRUint32 _fol_i = 0; _fol_i < _listCopy.Length(); _fol_i ++) { \
         if (_listCopy[_fol_i]) \
           _listCopy[_fol_i]->_functionCall; \
       } \
     } \
   }
-#define ENUMERATE_ALL_BOOKMARK_FOLDER_OBSERVERS(_functionCall) \
-  { \
-    FolderObserverList _folders; \
-    mBookmarkFolderObservers.EnumerateRead(FolderObserverEnumerator, &_folders); \
-    for (PRUint32 _fol_i = 0; _fol_i < _folders.Length(); _fol_i++) { \
-      if (_folders[_fol_i]) \
-        _folders[_fol_i]->_functionCall; \
-    } \
-  }
 #define ENUMERATE_ALL_BOOKMARKS_OBSERVERS(_functionCall) \
   { \
     nsTArray<nsNavHistoryQueryResultNode*> observerCopy(mAllBookmarksObservers); \
     for (PRUint32 _obs_i = 0; _obs_i < observerCopy.Length(); _obs_i ++) { \
       if (observerCopy[_obs_i]) \
       observerCopy[_obs_i]->_functionCall; \
     } \
   }
@@ -4157,31 +4111,29 @@ FolderObserverEnumerator(nsTrimInt64Hash
 // nsNavHistoryResult::OnBeginUpdateBatch (nsINavBookmark/HistoryObserver)
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnBeginUpdateBatch()
 {
   mBatchInProgress = PR_TRUE;
   ENUMERATE_HISTORY_OBSERVERS(OnBeginUpdateBatch());
   ENUMERATE_ALL_BOOKMARKS_OBSERVERS(OnBeginUpdateBatch());
-  ENUMERATE_ALL_BOOKMARK_FOLDER_OBSERVERS(OnBeginUpdateBatch());
   return NS_OK;
 }
 
 
 // nsNavHistoryResult::OnEndUpdateBatch (nsINavBookmark/HistoryObserver)
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnEndUpdateBatch()
 {
   if (mBatchInProgress) {
     mBatchInProgress = PR_FALSE;
     ENUMERATE_HISTORY_OBSERVERS(OnEndUpdateBatch());
     ENUMERATE_ALL_BOOKMARKS_OBSERVERS(OnEndUpdateBatch());
-    ENUMERATE_ALL_BOOKMARK_FOLDER_OBSERVERS(OnEndUpdateBatch());
   }
   else
     NS_WARNING("EndUpdateBatch without a begin");
   return NS_OK;
 }
 
 
 // nsNavHistoryResult::OnItemAdded (nsINavBookmarkObserver)
--- a/toolkit/components/places/src/nsNavHistoryResult.h
+++ b/toolkit/components/places/src/nsNavHistoryResult.h
@@ -805,17 +805,16 @@ public:
   void ReindexRange(PRInt32 aStartIndex, PRInt32 aEndIndex, PRInt32 aDelta);
 
   nsNavHistoryResultNode* FindChildById(PRInt64 aItemId,
                                         PRUint32* aNodeIndex);
 
 private:
 
   PRBool mIsRegisteredFolderObserver;
-  PRBool mBatchInProgress;
 };
 
 // nsNavHistorySeparatorResultNode
 //
 // Separator result nodes do not hold any data.
 class nsNavHistorySeparatorResultNode : public nsNavHistoryResultNode
 {
 public:
deleted file mode 100644
--- a/toolkit/components/places/tests/bookmarks/test_432706_in_batch.js
+++ /dev/null
@@ -1,75 +0,0 @@
-/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
-/* vim:set ts=2 sw=2 sts=2 et: */
-/* ***** BEGIN LICENSE BLOCK *****
- * Version: MPL 1.1/GPL 2.0/LGPL 2.1
- *
- * The contents of this file are subject to the Mozilla Public License Version
- * 1.1 (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- * http://www.mozilla.org/MPL/
- *
- * Software distributed under the License is distributed on an "AS IS" basis,
- * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
- * for the specific language governing rights and limitations under the
- * License.
- *
- * The Original Code is Places test code.
- *
- * The Initial Developer of the Original Code is Mozilla Corp.
- * Portions created by the Initial Developer are Copyright (C) 2008
- * the Initial Developer. All Rights Reserved.
- *
- * Contributor(s):
- *  Dietrich Ayala <dietrich@mozilla.com>
- *
- * 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
- * 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 ***** */
-
-var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
-            getService(Ci.nsINavBookmarksService);
-var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
-              getService(Ci.nsINavHistoryService);
-
-// main
-function run_test() {
-
-  // create a folder
-  var testFolder = bmsvc.createFolder(bmsvc.placesRoot, "test Folder",
-                                      bmsvc.DEFAULT_INDEX);
-
-  // get a node for the folder
-  var query = histsvc.getNewQuery();
-  query.setFolders([testFolder], 1);
-  var result = histsvc.executeQuery(query, histsvc.getNewQueryOptions());
-  var rootNode = result.root;
-  rootNode.containerOpen = true;
-
-  // start a batch
-  bmsvc.runInBatchMode({
-    runBatched: function(aUserData) {
-      // add a bookmark
-      var bookmarkId = bmsvc.insertBookmark(testFolder, uri("http://google.com/"),
-                                            bmsvc.DEFAULT_INDEX, "");
-
-      // confirm the node child count didn't change inside the batch
-      do_check_eq(rootNode.childCount, 0);
-    }
-  }, null);
-
-  // confirm that after the batch ended, the node and
-  // observer have been updated
-  do_check_eq(rootNode.childCount, 1);
-
-  rootNode.containerOpen = false;
-}
deleted file mode 100644
--- a/toolkit/components/places/tests/bookmarks/test_disconnected_nodes.js
+++ /dev/null
@@ -1,93 +0,0 @@
-/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
-/* vim:set ts=2 sw=2 sts=2 et: */
-/* ***** BEGIN LICENSE BLOCK *****
- * Version: MPL 1.1/GPL 2.0/LGPL 2.1
- *
- * The contents of this file are subject to the Mozilla Public License Version
- * 1.1 (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- * http://www.mozilla.org/MPL/
- *
- * Software distributed under the License is distributed on an "AS IS" basis,
- * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
- * for the specific language governing rights and limitations under the
- * License.
- *
- * The Original Code is bug 471563 test code.
- *
- * The Initial Developer of the Original Code is
- * Mozilla Corporation.
- * Portions created by the Initial Developer are Copyright (C) 2008
- * the Initial Developer. All Rights Reserved.
- *
- * Contributor(s):
- *   Dietrich Ayala <dietrich@mozilla.com> (Original Author)
- *
- * 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
- * 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 ***** */
-
-// get services
-var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
-              getService(Ci.nsINavHistoryService);
-var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
-            getService(Ci.nsINavBookmarksService);
-var tagssvc = Cc["@mozilla.org/browser/tagging-service;1"].
-              getService(Ci.nsITaggingService);
-
-function run_test() {
-  // get toolbar node
-  var options = histsvc.getNewQueryOptions();
-  var query = histsvc.getNewQuery();
-  query.setFolders([bmsvc.toolbarFolder], 1);
-  var result = histsvc.executeQuery(query, options);
-  var toolbarNode = result.root;
-  toolbarNode.containerOpen = true;
-
-  // add a bookmark
-  var bookmarkURI = uri("http://foo.com");
-  var bookmarkId = bmsvc.insertBookmark(bmsvc.toolbarFolder, bookmarkURI,
-                                        bmsvc.DEFAULT_INDEX, "");
-
-  // get the node for the new bookmark
-  var node = toolbarNode.getChild(toolbarNode.childCount-1);
-  do_check_eq(node.itemId, bookmarkId);
-
-  // confirm there's no tags via the .tags property
-  do_check_eq(node.tags, null); 
-
-  // add a tag
-  // NOTE: this executes in a batch, which causes |toolbarNode|
-  // to be refreshed, which disconnects the uri node |node|.
-  tagssvc.tagURI(bookmarkURI, ["foo"]);
-  do_check_eq(node.tags, "foo");
-
-  // add another tag, and check that the node updates itself.
-  tagssvc.tagURI(bookmarkURI, ["bar"]);
-  do_check_eq(node.tags, "bar, foo");
-
-  // change the bookmark title,
-  // confirm that the node updates itself.
-  // XXX disabled until bug 471563 is fixed.
-  //bmsvc.setItemTitle(node.itemId, "newtitle");
-  //do_check_eq(node.title, "newtitle");
-
-  // change the bookmark uri,
-  // confirm that the node updates itself.
-  // XXX disabled until bug 471563 is fixed.
-  //var newURI = uri("http://newuri");
-  //bmsvc.changeBookmarkURI(node.itemId, newURI);
-  //do_check_eq(node.uri, newURI.spec);
-
-  toolbarNode.containerOpen = false;
-}