Bug 652379 - place:folder=-1 returns a non-empty result.
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 28 Apr 2011 19:59:39 +0200
changeset 68765 e7c50e928098fc3540c56113011e55bfe4e7c27f
parent 68764 38487eaeb27c3c9135e76b937b0f6139efa9558c
child 68766 5f97ac170bfaea43eb5cef7e05975c4edc5957d0
push id19737
push usermak77@bonardo.net
push dateFri, 29 Apr 2011 14:06:46 +0000
treeherdermozilla-central@39b60d7d00c9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs652379
milestone6.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 652379 - place:folder=-1 returns a non-empty result. r=dietrich
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/nsNavHistoryResult.h
toolkit/components/places/tests/unit/test_broken_folderShortcut_result.js
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -2856,32 +2856,35 @@ nsNavHistory::ExecuteQueries(nsINavHisto
   // concrete queries array
   nsCOMArray<nsNavHistoryQuery> queries;
   for (PRUint32 i = 0; i < aQueryCount; i ++) {
     nsCOMPtr<nsNavHistoryQuery> query = do_QueryInterface(aQueries[i], &rv);
     NS_ENSURE_SUCCESS(rv, rv);
     queries.AppendObject(query);
   }
 
-  nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
-  NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
-  // root node
+  // Create the root node.
   nsRefPtr<nsNavHistoryContainerResultNode> rootNode;
   PRInt64 folderId = GetSimpleBookmarksQueryFolder(queries, options);
   if (folderId) {
-    // In the simple case where we're just querying children of a single bookmark
-    // folder, we can more efficiently generate results.
+    // In the simple case where we're just querying children of a single
+    // bookmark folder, we can more efficiently generate results.
+    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
+    NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
     nsRefPtr<nsNavHistoryResultNode> tempRootNode;
     rv = bookmarks->ResultNodeForContainer(folderId, options,
                                            getter_AddRefs(tempRootNode));
-    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
-                     "Generating a generic empty node for a broken query!");
     if (NS_SUCCEEDED(rv)) {
       rootNode = tempRootNode->GetAsContainer();
     }
+    else {
+      NS_WARNING("Generating a generic empty node for a broken query!");
+      // This is a perf hack to generate an empty query that skips filtering.
+      options->SetExcludeItems(PR_TRUE);
+    }
   }
 
   if (!rootNode) {
     // Either this is not a folder shortcut, or is a broken one.  In both cases
     // just generate a query node.
     rootNode = new nsNavHistoryQueryResultNode(EmptyCString(), EmptyCString(),
                                                queries, options);
   }
@@ -6142,19 +6145,20 @@ nsNavHistory::FilterResultSet(nsNavHisto
       }
 
       // Filter bookmarks on parent folder.
       // RESULTS_AS_TAG_CONTENTS changes bookmarks' parents, so we cannot filter
       // this kind of result based on the parent.
       if (includeFolders[queryIndex]->Length() != 0 &&
           resultType != nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS) {
         // Filter out the node if its parent is in the excludeFolders
-        // cache.
-        if (excludeFolders[queryIndex]->Contains(parentId))
+        // cache or it has no parent.
+        if (excludeFolders[queryIndex]->Contains(parentId) || parentId == -1) {
           continue;
+        }
 
         if (!includeFolders[queryIndex]->Contains(parentId)) {
           // If parent is not found in current includeFolders cache, we check
           // its ancestors.
           PRInt64 ancestor = parentId;
           PRBool belongs = PR_FALSE;
           nsTArray<PRInt64> ancestorFolders;
 
@@ -6528,16 +6532,18 @@ nsNavHistory::QueryRowToResult(PRInt64 i
 
   if (NS_FAILED(rv)) {
     NS_WARNING("Generating a generic empty node for a broken query!");
     // This is a broken query, that either did not parse or points to not
     // existing data.  We don't want to return failure since that will kill the
     // whole result.  Instead make a generic empty query node.
     *aNode = new nsNavHistoryQueryResultNode(aTitle, aFavicon, aURI);
     (*aNode)->mItemId = itemId;
+    // This is a perf hack to generate an empty query that skips filtering.
+    (*aNode)->GetAsQuery()->Options()->SetExcludeItems(PR_TRUE);
     NS_ADDREF(*aNode);
   }
 
   return NS_OK;
 }
 
 
 // nsNavHistory::VisitIdToResultNode
@@ -6911,17 +6917,17 @@ GetSimpleBookmarksQueryFolder(const nsCO
 
   // RESULTS_AS_TAG_CONTENTS is quite similar to a folder shortcut, but it must
   // not be treated like that, since it needs all query options.
   if(aOptions->ResultType() == nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS)
     return 0;
 
   // Don't care about onlyBookmarked flag, since specifying a bookmark
   // folder is inferring onlyBookmarked.
-  NS_ASSERTION(query->Folders()[0] > 0, "bad folder id");
+
   return query->Folders()[0];
 }
 
 
 // ParseSearchTermsFromQueries
 //
 //    Construct a matrix of search terms from the given queries array.
 //    All of the query objects are ORed together. Within a query, all the terms
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -2336,30 +2336,33 @@ nsNavHistoryQueryResultNode::~nsNavHisto
  * expandQueries is unset.
  */
 PRBool
 nsNavHistoryQueryResultNode::CanExpand()
 {
   if (IsContainersQuery())
     return PR_TRUE;
 
-  // If we are child of an ExcludeItems parent or root, we should not expand.
+  // If ExcludeItems is set on the root or on the node itself, don't expand.
   if ((mResult && mResult->mRootNode->mOptions->ExcludeItems()) ||
-      (mParent && mParent->mOptions->ExcludeItems()))
+      Options()->ExcludeItems())
     return PR_FALSE;
 
+  // Check the ancestor container.
   nsNavHistoryQueryOptions* options = GetGeneratingOptions();
   if (options) {
     if (options->ExcludeItems())
       return PR_FALSE;
     if (options->ExpandQueries())
       return PR_TRUE;
   }
+
   if (mResult && mResult->mRootNode == this)
     return PR_TRUE;
+
   return PR_FALSE;
 }
 
 
 /**
  * Some query with a particular result type can contain other queries.  They
  * must be always expandable
  */
--- a/toolkit/components/places/nsNavHistoryResult.h
+++ b/toolkit/components/places/nsNavHistoryResult.h
@@ -711,19 +711,19 @@ NS_DEFINE_STATIC_IID_ACCESSOR(nsNavHisto
 //    Overridden container type for complex queries over history and/or
 //    bookmarks. This keeps itself in sync by listening to history and
 //    bookmark notifications.
 
 class nsNavHistoryQueryResultNode : public nsNavHistoryContainerResultNode,
                                     public nsINavHistoryQueryResultNode
 {
 public:
-  nsNavHistoryQueryResultNode(const nsACString& aQueryURI,
-                              const nsACString& aTitle,
-                              const nsACString& aIconURI);
+  nsNavHistoryQueryResultNode(const nsACString& aTitle,
+                              const nsACString& aIconURI,
+                              const nsACString& aQueryURI);
   nsNavHistoryQueryResultNode(const nsACString& aTitle,
                               const nsACString& aIconURI,
                               const nsCOMArray<nsNavHistoryQuery>& aQueries,
                               nsNavHistoryQueryOptions* aOptions);
   nsNavHistoryQueryResultNode(const nsACString& aTitle,
                               const nsACString& aIconURI,
                               PRTime aTime,
                               const nsCOMArray<nsNavHistoryQuery>& aQueries,
--- a/toolkit/components/places/tests/unit/test_broken_folderShortcut_result.js
+++ b/toolkit/components/places/tests/unit/test_broken_folderShortcut_result.js
@@ -1,44 +1,76 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 function run_test() {
   PlacesUtils.bookmarks.insertBookmark(
     PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("http://1.moz.org/"),
     PlacesUtils.bookmarks.DEFAULT_INDEX, "Bookmark 1"
   );
-  let id = PlacesUtils.bookmarks.insertBookmark(
+  let id1 = PlacesUtils.bookmarks.insertBookmark(
     PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("place:folder=1234"),
-    PlacesUtils.bookmarks.DEFAULT_INDEX, "Shortcut"
+    PlacesUtils.bookmarks.DEFAULT_INDEX, "Shortcut 1"
+  );
+  let id2 = PlacesUtils.bookmarks.insertBookmark(
+    PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("place:folder=-1"),
+    PlacesUtils.bookmarks.DEFAULT_INDEX, "Shortcut 2"
   );
   PlacesUtils.bookmarks.insertBookmark(
     PlacesUtils.unfiledBookmarksFolderId, NetUtil.newURI("http://2.moz.org/"),
     PlacesUtils.bookmarks.DEFAULT_INDEX, "Bookmark 2"
   );
 
+  // Add also a simple visit.
+  PlacesUtils.history.addVisit(
+    NetUtil.newURI("http://3.moz.org/"), Date.now() * 1000, null,
+    PlacesUtils.history.TRANSITION_TYPED, false, 0
+  );
+
   // Query containing a broken folder shortcuts among results.
   let query = PlacesUtils.history.getNewQuery();
   query.setFolders([PlacesUtils.unfiledBookmarksFolderId], 1);
   let options = PlacesUtils.history.getNewQueryOptions();
   let root = PlacesUtils.history.executeQuery(query, options).root;
   root.containerOpen = true;
-  do_check_eq(root.childCount, 3);
+
+  do_check_eq(root.childCount, 4);
+
   let shortcut = root.getChild(1);
   do_check_eq(shortcut.uri, "place:folder=1234");
   PlacesUtils.asContainer(shortcut);
   shortcut.containerOpen = true;
   do_check_eq(shortcut.childCount, 0);
   shortcut.containerOpen = false;
   // Remove the broken shortcut while the containing result is open.
-  PlacesUtils.bookmarks.removeItem(id);
+  PlacesUtils.bookmarks.removeItem(id1);
+  do_check_eq(root.childCount, 3);
+
+  shortcut = root.getChild(1);
+  do_check_eq(shortcut.uri, "place:folder=-1");
+  PlacesUtils.asContainer(shortcut);
+  shortcut.containerOpen = true;
+  do_check_eq(shortcut.childCount, 0);
+  shortcut.containerOpen = false;
+  // Remove the broken shortcut while the containing result is open.
+  PlacesUtils.bookmarks.removeItem(id2);
   do_check_eq(root.childCount, 2);
+
   root.containerOpen = false;
 
   // Broken folder shortcut as root node.
   let query = PlacesUtils.history.getNewQuery();
   query.setFolders([1234], 1);
   let options = PlacesUtils.history.getNewQueryOptions();
   let root = PlacesUtils.history.executeQuery(query, options).root;
   root.containerOpen = true;
   do_check_eq(root.childCount, 0);
   root.containerOpen = false;
+
+  // Broken folder shortcut as root node with folder=-1.
+  query = PlacesUtils.history.getNewQuery();
+  query.setFolders([-1], 1);
+  options = PlacesUtils.history.getNewQueryOptions();
+  root = PlacesUtils.history.executeQuery(query, options).root;
+  root.containerOpen = true;
+  do_check_eq(root.childCount, 0);
+  root.containerOpen = false;
 }