Bug 992670 - Make all child insertions/removals non-temporary in nsNavHistoryContainerResultNode. r=mak
☠☠ backed out by c60b059c47ca ☠ ☠
authorBirunthan Mohanathas <birunthan@mohanathas.com>
Wed, 16 Apr 2014 13:56:52 -0400
changeset 179238 e128ceec086cf203af55501ac688534c1c9610f8
parent 179237 7e31deb02ba88d389eae04f5a528d0696e22c714
child 179239 858255f591710d0f1dfbd5454003406163e7f3b3
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewersmak
bugs992670
milestone31.0a1
Bug 992670 - Make all child insertions/removals non-temporary in nsNavHistoryContainerResultNode. r=mak
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/nsNavHistoryResult.h
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -1295,108 +1295,100 @@ nsNavHistoryContainerResultNode::FindChi
   }
   return nullptr;
 }
 
 /**
  * This does the work of adding a child to the container.  The child can be
  * either a container or or a single item that may even be collapsed with the
  * adjacent ones.
- *
- * Some inserts are "temporary" meaning that they are happening immediately
- * after a temporary remove.  We do this when movings elements when they
- * change to keep them in the proper sorting position.  In these cases, we
- * don't need to recompute any statistics.
  */
 nsresult
 nsNavHistoryContainerResultNode::InsertChildAt(nsNavHistoryResultNode* aNode,
-                                               int32_t aIndex,
-                                               bool aIsTemporary)
+                                               int32_t aIndex)
 {
   nsNavHistoryResult* result = GetResult();
   NS_ENSURE_STATE(result);
 
   aNode->mParent = this;
   aNode->mIndentLevel = mIndentLevel + 1;
-  if (!aIsTemporary && aNode->IsContainer()) {
+  if (aNode->IsContainer()) {
     // need to update all the new item's children
     nsNavHistoryContainerResultNode* container = aNode->GetAsContainer();
     container->mResult = result;
     container->FillStats();
   }
 
   if (!mChildren.InsertObjectAt(aNode, aIndex))
     return NS_ERROR_OUT_OF_MEMORY;
 
   // Update our stats and notify the result's observers.
-  if (!aIsTemporary) {
-    mAccessCount += aNode->mAccessCount;
-    if (mTime < aNode->mTime)
-      mTime = aNode->mTime;
-    if (!mParent || mParent->AreChildrenVisible()) {
-      NOTIFY_RESULT_OBSERVERS(result,
-                              NodeHistoryDetailsChanged(TO_ICONTAINER(this),
-                                                        mTime,
-                                                        mAccessCount));
-    }
-
-    nsresult rv = ReverseUpdateStats(aNode->mAccessCount);
-    NS_ENSURE_SUCCESS(rv, rv);
+  mAccessCount += aNode->mAccessCount;
+  if (mTime < aNode->mTime)
+    mTime = aNode->mTime;
+  if (!mParent || mParent->AreChildrenVisible()) {
+    NOTIFY_RESULT_OBSERVERS(result,
+                            NodeHistoryDetailsChanged(TO_ICONTAINER(this),
+                                                      mTime,
+                                                      mAccessCount));
   }
 
+  nsresult rv = ReverseUpdateStats(aNode->mAccessCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   // Update tree if we are visible.  Note that we could be here and not
   // expanded, like when there is a bookmark folder being updated because its
   // parent is visible.
   if (AreChildrenVisible())
     NOTIFY_RESULT_OBSERVERS(result, NodeInserted(this, aNode, aIndex));
 
   return NS_OK;
 }
 
 
 /**
  * This locates the proper place for insertion according to the current sort
  * and calls InsertChildAt
  */
 nsresult
 nsNavHistoryContainerResultNode::InsertSortedChild(
-    nsNavHistoryResultNode* aNode, 
-    bool aIsTemporary, bool aIgnoreDuplicates)
+    nsNavHistoryResultNode* aNode,
+    bool aIgnoreDuplicates)
 {
 
   if (mChildren.Count() == 0)
-    return InsertChildAt(aNode, 0, aIsTemporary);
+    return InsertChildAt(aNode, 0);
 
   SortComparator comparator = GetSortingComparator(GetSortType());
   if (comparator) {
     // When inserting a new node, it must have proper statistics because we use
     // them to find the correct insertion point.  The insert function will then
     // recompute these statistics and fill in the proper parents and hierarchy
     // level.  Doing this twice shouldn't be a large performance penalty because
     // when we are inserting new containers, they typically contain only one
     // item (because we've browsed a new page).
-    if (!aIsTemporary && aNode->IsContainer()) {
+    if (aNode->IsContainer()) {
       // need to update all the new item's children
       nsNavHistoryContainerResultNode* container = aNode->GetAsContainer();
       container->mResult = mResult;
       container->FillStats();
     }
 
     nsAutoCString sortingAnnotation;
     GetSortingAnnotation(sortingAnnotation);
     bool itemExists;
     uint32_t position = FindInsertionPoint(aNode, comparator, 
                                            sortingAnnotation.get(), 
                                            &itemExists);
     if (aIgnoreDuplicates && itemExists)
       return NS_OK;
 
-    return InsertChildAt(aNode, position, aIsTemporary);
+    return InsertChildAt(aNode, position);
   }
-  return InsertChildAt(aNode, mChildren.Count(), aIsTemporary);
+  return InsertChildAt(aNode, mChildren.Count());
 }
 
 /**
  * This checks if the item at aIndex is located correctly given the sorting
  * move.  If it's not, the item is moved, and the result's observers are
  * notified.
  *
  * @return true if the item position has been changed, false otherwise.
@@ -1432,53 +1424,42 @@ nsNavHistoryContainerResultNode::EnsureI
 
   return true;
 }
 
 /**
  * This does all the work of removing a child from this container, including
  * updating the tree if necessary.  Note that we do not need to be open for
  * this to work.
- *
- * Some removes are "temporary" meaning that they'll just get inserted again.
- * We do this for resorting.  In these cases, we don't need to recompute any
- * statistics, and we shouldn't notify those container that they are being
- * removed.
  */
 nsresult
-nsNavHistoryContainerResultNode::RemoveChildAt(int32_t aIndex,
-                                               bool aIsTemporary)
+nsNavHistoryContainerResultNode::RemoveChildAt(int32_t aIndex)
 {
   NS_ASSERTION(aIndex >= 0 && aIndex < mChildren.Count(), "Invalid index");
 
   // Hold an owning reference to keep from expiring while we work with it.
   nsRefPtr<nsNavHistoryResultNode> oldNode = mChildren[aIndex];
 
   // Update stats.
-  uint32_t oldAccessCount = 0;
-  if (!aIsTemporary) {
-    MOZ_ASSERT(mAccessCount >= mChildren[aIndex]->mAccessCount,
-               "Invalid access count while updating!");
-    oldAccessCount = mAccessCount;
-    mAccessCount -= mChildren[aIndex]->mAccessCount;
-  }
+  MOZ_ASSERT(mAccessCount >= mChildren[aIndex]->mAccessCount,
+             "Invalid access count while updating!");
+  uint32_t oldAccessCount = mAccessCount;
+  mAccessCount -= mChildren[aIndex]->mAccessCount;
 
   // Remove it from our list and notify the result's observers.
   mChildren.RemoveObjectAt(aIndex);
   if (AreChildrenVisible()) {
     nsNavHistoryResult* result = GetResult();
     NOTIFY_RESULT_OBSERVERS(result,
                             NodeRemoved(this, oldNode, aIndex));
   }
 
-  if (!aIsTemporary) {
-    nsresult rv = ReverseUpdateStats(mAccessCount - oldAccessCount);
-    NS_ENSURE_SUCCESS(rv, rv);
-    oldNode->OnRemoving();
-  }
+  nsresult rv = ReverseUpdateStats(mAccessCount - oldAccessCount);
+  NS_ENSURE_SUCCESS(rv, rv);
+  oldNode->OnRemoving();
   return NS_OK;
 }
 
 
 /**
  * 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.
@@ -2581,17 +2562,17 @@ nsNavHistoryQueryResultNode::OnTitleChan
       // This could be a new node matching the query, thus we could need
       // to add it to the result.
       nsRefPtr<nsNavHistoryResultNode> node;
       nsNavHistory* history = nsNavHistory::GetHistoryService();
       NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
       rv = history->URIToResultNode(aURI, mOptions, getter_AddRefs(node));
       NS_ENSURE_SUCCESS(rv, rv);
       if (history->EvaluateQueryForNode(mQueries, mOptions, node)) {
-        rv = InsertSortedChild(node, true);
+        rv = InsertSortedChild(node);
         NS_ENSURE_SUCCESS(rv, rv);
       }
     }
     for (int32_t i = 0; i < matches.Count(); ++i) {
       // For each matched node we check if it passes the query filter, if not
       // we remove the node from the result, otherwise we'll update the title
       // later.
       nsNavHistoryResultNode* node = matches[i];
@@ -2794,17 +2775,17 @@ nsNavHistoryQueryResultNode::NotifyIfTag
   RecursiveFindURIs(onlyOneEntry, this, spec, &matches);
 
   if (matches.Count() == 0 && mHasSearchTerms && !mRemovingURI) {
     // A new tag has been added, it's possible it matches our query.
     NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
     rv = history->URIToResultNode(aURI, mOptions, getter_AddRefs(node));
     NS_ENSURE_SUCCESS(rv, rv);
     if (history->EvaluateQueryForNode(mQueries, mOptions, node)) {
-      rv = InsertSortedChild(node, true);
+      rv = InsertSortedChild(node);
       NS_ENSURE_SUCCESS(rv, rv);
     }
   }
 
   for (int32_t i = 0; i < matches.Count(); ++i) {
     nsNavHistoryResultNode* node = matches[i];
     // Force a tags update before checking the node.
     node->mTags.SetIsVoid(true);
@@ -3629,17 +3610,17 @@ nsNavHistoryFolderResultNode::OnItemAdde
 
   if (aItemType == nsINavBookmarksService::TYPE_SEPARATOR ||
       GetSortType() == nsINavHistoryQueryOptions::SORT_BY_NONE) {
     // insert at natural bookmarks position
     return InsertChildAt(node, aIndex);
   }
 
   // insert at sorted position
-  return InsertSortedChild(node, false);
+  return InsertSortedChild(node);
 }
 
 
 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnItemRemoved(int64_t aItemId,
                                             int64_t aParentFolder,
                                             int32_t aIndex,
                                             uint16_t aItemType,
--- a/toolkit/components/places/nsNavHistoryResult.h
+++ b/toolkit/components/places/nsNavHistoryResult.h
@@ -571,24 +571,22 @@ public:
     return FindChildURI(spec, aNodeIndex);
   }
   nsNavHistoryResultNode* FindChildURI(const nsACString& aSpec,
                                        uint32_t* aNodeIndex);
   // returns the index of the given node, -1 if not found
   int32_t FindChild(nsNavHistoryResultNode* aNode)
     { return mChildren.IndexOf(aNode); }
 
-  nsresult InsertChildAt(nsNavHistoryResultNode* aNode, int32_t aIndex,
-                         bool aIsTemporary = false);
+  nsresult InsertChildAt(nsNavHistoryResultNode* aNode, int32_t aIndex);
   nsresult InsertSortedChild(nsNavHistoryResultNode* aNode,
-                             bool aIsTemporary = false,
                              bool aIgnoreDuplicates = false);
   bool EnsureItemPosition(uint32_t aIndex);
 
-  nsresult RemoveChildAt(int32_t aIndex, bool aIsTemporary = false);
+  nsresult RemoveChildAt(int32_t aIndex);
 
   void RecursiveFindURIs(bool aOnlyOne,
                          nsNavHistoryContainerResultNode* aContainer,
                          const nsCString& aSpec,
                          nsCOMArray<nsNavHistoryResultNode>* aMatches);
   bool UpdateURIs(bool aRecursive, bool aOnlyOne, bool aUpdateSort,
                   const nsCString& aSpec,
                   nsresult (*aCallback)(nsNavHistoryResultNode*, const void*,