diff --git a/browser/base/content/browser-menubar.inc b/browser/base/content/browser-menubar.inc index cc8606df350..9575dbe29ad 100644 --- a/browser/base/content/browser-menubar.inc +++ b/browser/base/content/browser-menubar.inc @@ -452,6 +452,7 @@ @@ -119,8 +121,14 @@ 0) { preSep.sort(sortingMethod); newOrder = newOrder.concat(preSep); diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl index 22bee922cf9..7e037b701b5 100644 --- a/toolkit/components/places/public/nsINavHistoryService.idl +++ b/toolkit/components/places/public/nsINavHistoryService.idl @@ -614,6 +614,13 @@ interface nsINavHistoryResult : nsISupports /** * This is the root of the results. Remember that you need to open all * containers for their contents to be valid. + * + * When a result goes out of scope it will continue to observe changes till + * it is cycle collected. While the result waits to be collected it will stay + * in memory, and continue to update itself, potentially causing unwanted + * additional work. When you close the root node the result will stop + * observing changes, so it is good practice to close the root node when you + * are done with a result, since that will avoid unwanted performance hits. */ readonly attribute nsINavHistoryContainerResultNode root; }; diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp index 89905f23b8f..c1f600e0b78 100644 --- a/toolkit/components/places/src/nsNavHistoryResult.cpp +++ b/toolkit/components/places/src/nsNavHistoryResult.cpp @@ -499,7 +499,8 @@ nsNavHistoryContainerResultNode::CloseContainer(PRBool aUpdateView) // recursively close all child containers for (PRInt32 i = 0; i < mChildren.Count(); i ++) { - if (mChildren[i]->IsContainer() && mChildren[i]->GetAsContainer()->mExpanded) + if (mChildren[i]->IsContainer() && + mChildren[i]->GetAsContainer()->mExpanded) mChildren[i]->GetAsContainer()->CloseContainer(PR_FALSE); } @@ -508,17 +509,25 @@ nsNavHistoryContainerResultNode::CloseContainer(PRBool aUpdateView) nsresult rv; if (IsDynamicContainer()) { // notify dynamic containers that we are closing - nsCOMPtr svc = do_GetService(mDynamicContainerType.get(), &rv); + nsCOMPtr svc = + do_GetService(mDynamicContainerType.get(), &rv); if (NS_SUCCEEDED(rv)) svc->OnContainerNodeClosed(this); } + nsNavHistoryResult* result = GetResult(); if (aUpdateView) { - nsNavHistoryResult* result = GetResult(); NS_ENSURE_TRUE(result, NS_ERROR_FAILURE); if (result->GetView()) result->GetView()->ContainerClosed(this); } + + // If this is the root container of a result, we can tell the result to stop + // observing changes, otherwise the result will stay in memory and updates + // itself till it is cycle collected. + if (result->mRootNode == this) + result->StopObserving(); + return NS_OK; } @@ -2376,12 +2385,17 @@ nsNavHistoryQueryResultNode::FillChildren() PRUint16 sortType = GetSortType(); - // The default SORT_BY_NONE sorts by the bookmark index (position), - // which we do not have for history queries - if (mOptions->QueryType() != nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY || - sortType != nsINavHistoryQueryOptions::SORT_BY_NONE) { - // once we've computed all tree stats, we can sort, because containers will - // then have proper visit counts and dates + if (mResult->mNeedsToApplySortingMode) { + // We should repopulate container and then apply sortingMode. To avoid + // sorting 2 times we simply do that here. + mResult->SetSortingMode(mResult->mSortingMode); + } + else if (mOptions->QueryType() != nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY || + sortType != nsINavHistoryQueryOptions::SORT_BY_NONE) { + // The default SORT_BY_NONE sorts by the bookmark index (position), + // which we do not have for history queries. + // Once we've computed all tree stats, we can sort, because containers will + // then have proper visit counts and dates. SortComparator comparator = GetSortingComparator(GetSortType()); if (comparator) { nsCAutoString sortingAnnotation; @@ -3260,13 +3274,20 @@ nsNavHistoryFolderResultNode::FillChildren() // nodes and the result node pointers on the containers FillStats(); - // once we've computed all tree stats, we can sort, because containers will - // then have proper visit counts and dates - SortComparator comparator = GetSortingComparator(GetSortType()); - if (comparator) { - nsCAutoString sortingAnnotation; - GetSortingAnnotation(sortingAnnotation); - RecursiveSort(sortingAnnotation.get(), comparator); + if (mResult->mNeedsToApplySortingMode) { + // We should repopulate container and then apply sortingMode. To avoid + // sorting 2 times we simply do that here. + mResult->SetSortingMode(mResult->mSortingMode); + } + else { + // once we've computed all tree stats, we can sort, because containers will + // then have proper visit counts and dates + SortComparator comparator = GetSortingComparator(GetSortType()); + if (comparator) { + nsCAutoString sortingAnnotation; + GetSortingAnnotation(sortingAnnotation); + RecursiveSort(sortingAnnotation.get(), comparator); + } } // if we are limiting our results remove items from the end of the @@ -3863,7 +3884,8 @@ nsNavHistoryResult::nsNavHistoryResult(nsNavHistoryContainerResultNode* aRoot) : mIsHistoryObserver(PR_FALSE), mIsBookmarkFolderObserver(PR_FALSE), mIsAllBookmarksObserver(PR_FALSE), - mBatchInProgress(PR_FALSE) + mBatchInProgress(PR_FALSE), + mNeedsToApplySortingMode(PR_FALSE) { mRootNode->mResult = this; } @@ -3874,6 +3896,33 @@ nsNavHistoryResult::~nsNavHistoryResult() mBookmarkFolderObservers.Enumerate(&RemoveBookmarkFolderObserversCallback, nsnull); } +void +nsNavHistoryResult::StopObserving() +{ + if (mIsBookmarkFolderObserver || mIsAllBookmarksObserver) { + nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); + if (bookmarks) { + bookmarks->RemoveObserver(this); + mIsBookmarkFolderObserver = PR_FALSE; + mIsAllBookmarksObserver = PR_FALSE; + } + } + if (mIsHistoryObserver) { + nsNavHistory* history = nsNavHistory::GetHistoryService(); + if (history) { + history->RemoveObserver(this); + mIsHistoryObserver = PR_FALSE; + } + } + + // We stop observing when root node is closed, but when reopening it result + // will be out of sync. Ensure we will call FillChildren again in such a + // case. + if (mRootNode->IsQuery()) + mRootNode->GetAsQuery()->ClearChildren(PR_TRUE); + else if (mRootNode->IsFolder()) + mRootNode->GetAsFolder()->ClearChildren(PR_TRUE); +} // nsNavHistoryResult::Init // @@ -4117,6 +4166,12 @@ nsNavHistoryResult::SetSortingMode(PRUint16 aSortingMode) mSortingMode = aSortingMode; + if (!mRootNode->mExpanded) { + // Need to do this later when node will be expanded. + mNeedsToApplySortingMode = PR_TRUE; + return NS_OK; + } + // actually do sorting nsNavHistoryContainerResultNode::SortComparator comparator = nsNavHistoryContainerResultNode::GetSortingComparator(aSortingMode); diff --git a/toolkit/components/places/src/nsNavHistoryResult.h b/toolkit/components/places/src/nsNavHistoryResult.h index ba3464fd71d..9d69a1130f3 100644 --- a/toolkit/components/places/src/nsNavHistoryResult.h +++ b/toolkit/components/places/src/nsNavHistoryResult.h @@ -155,6 +155,7 @@ public: void RemoveHistoryObserver(nsNavHistoryQueryResultNode* aNode); void RemoveBookmarkFolderObserver(nsNavHistoryFolderResultNode* aNode, PRInt64 aFolder); void RemoveAllBookmarksObserver(nsNavHistoryQueryResultNode* aNode); + void StopObserving(); // returns the view. NOT-ADDREFED. May be NULL if there is no view nsINavHistoryResultViewer* GetView() const @@ -176,6 +177,10 @@ public: // One of nsNavHistoryQueryOptions.SORY_BY_* This is initialized to mOptions.sortingMode, // but may be overridden if the user clicks on one of the columns. PRUint16 mSortingMode; + // If root node is closed and we try to apply a sortingMode, it would not + // work. So we will apply it when the node will be reopened and populated. + // This var states the fact we need to apply sortingMode in such a situation. + PRBool mNeedsToApplySortingMode; // The sorting annotation to be used for in SORT_BY_ANNOTATION_* modes nsCString mSortingAnnotation; diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js index e6c3b095b5c..c7a0ce9deb0 100644 --- a/toolkit/components/places/src/utils.js +++ b/toolkit/components/places/src/utils.js @@ -551,7 +551,11 @@ var PlacesUtils = { this.value += aStr; } }; - self.serializeNodeAsJSONToOutputStream(convertNode(aNode), writer, true, aForceCopy); + var node = convertNode(aNode); + self.serializeNodeAsJSONToOutputStream(node, writer, true, aForceCopy); + // Convert node could pass an open container node. + if (self.nodeIsContainer(node)) + node.containerOpen = false; return writer.value; case this.TYPE_X_MOZ_URL: function gatherDataUrl(bNode) { @@ -564,7 +568,13 @@ var PlacesUtils = { // ignore containers and separators - items without valid URIs return ""; } - return gatherDataUrl(convertNode(aNode)); + var node = convertNode(aNode); + var dataUrl = gatherDataUrl(node); + // Convert node could pass an open container node. + if (self.nodeIsContainer(node)) + node.containerOpen = false; + return dataUrl; + case this.TYPE_HTML: function gatherDataHtml(bNode) { @@ -605,7 +615,12 @@ var PlacesUtils = { return "
" + NEWLINE; return ""; } - return gatherDataHtml(convertNode(aNode)); + var node = convertNode(aNode); + var dataHtml = gatherDataHtml(node); + // Convert node could pass an open container node. + if (self.nodeIsContainer(node)) + node.containerOpen = false; + return dataHtml; } // case this.TYPE_UNICODE: function gatherDataText(bNode) { @@ -634,7 +649,12 @@ var PlacesUtils = { return ""; } - return gatherDataText(convertNode(aNode)); + var node = convertNode(aNode); + var dataText = gatherDataText(node); + // Convert node could pass an open container node. + if (self.nodeIsContainer(node)) + node.containerOpen = false; + return dataText; }, /**