Bug 1423896 - When excluding queries within places queries, we shouldn't exclude folder shortcuts. r=mak
authorMark Banner <standard8@mozilla.com>
Fri, 12 Jan 2018 16:15:25 +0000
changeset 402793 4321514ab05fda787bd17484067cb577c4bfa21f
parent 402792 12de5643ae0ae6ad6a97797f2a07fe856f9b66e1
child 402794 2211efa3ab47fb129834953439d743bb5a89f524
push id99659
push useraciure@mozilla.com
push dateWed, 07 Feb 2018 22:33:57 +0000
treeherdermozilla-inbound@5ceb1098fef3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1423896
milestone60.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 1423896 - When excluding queries within places queries, we shouldn't exclude folder shortcuts. r=mak MozReview-Commit-ID: 810igliCov8
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/tests/queries/test_excludeQueries.js
toolkit/components/places/tests/queries/xpcshell.ini
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -1385,17 +1385,18 @@ bool IsOptimizableHistoryQuery(const nsC
   return true;
 }
 
 static
 bool NeedToFilterResultSet(const nsCOMArray<nsNavHistoryQuery>& aQueries,
                              nsNavHistoryQueryOptions *aOptions)
 {
   uint16_t resultType = aOptions->ResultType();
-  return resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS;
+  return resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS ||
+         aOptions->ExcludeQueries();
 }
 
 // ** Helper class for ConstructQueryString **/
 
 class PlacesSQLQueryBuilder
 {
 public:
   PlacesSQLQueryBuilder(const nsCString& aConditions,
@@ -3152,17 +3153,19 @@ private:
 
 nsresult
 nsNavHistory::QueryToSelectClause(nsNavHistoryQuery* aQuery, // const
                                   nsNavHistoryQueryOptions* aOptions,
                                   int32_t aQueryIndex,
                                   nsCString* aClause)
 {
   bool hasIt;
-  bool excludeQueries = aOptions->ExcludeQueries();
+  // We don't use the value from options here - we post filter if that
+  // is set.
+  bool excludeQueries = false;
 
   ConditionBuilder clause(aQueryIndex);
 
   if ((NS_SUCCEEDED(aQuery->GetHasBeginTime(&hasIt)) && hasIt) ||
     (NS_SUCCEEDED(aQuery->GetHasEndTime(&hasIt)) && hasIt)) {
     clause.Condition("EXISTS (SELECT 1 FROM moz_historyvisits "
                               "WHERE place_id = h.id");
     // begin time
@@ -3301,17 +3304,17 @@ nsNavHistory::QueryToSelectClause(nsNavH
       if (i < includeFolders.Length() - 1) {
         clause.Str(",");
       }
     }
     clause.Str(")");
   }
 
   if (excludeQueries) {
-    // Serching by terms implicitly exclude queries.
+    // Serching by terms implicitly exclude queries and folder shortcuts.
     clause.Condition("NOT h.url_hash BETWEEN hash('place', 'prefix_lo') AND "
                                             "hash('place', 'prefix_hi')");
   }
 
   clause.GetClauseString(*aClause);
   return NS_OK;
 }
 
@@ -3551,35 +3554,39 @@ nsNavHistory::FilterResultSet(nsNavHisto
   nsNavBookmarks *bookmarks = nsNavBookmarks::GetBookmarksService();
   NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
 
   // parse the search terms
   nsTArray<nsTArray<nsString>*> terms;
   ParseSearchTermsFromQueries(aQueries, &terms);
 
   uint16_t resultType = aOptions->ResultType();
+  bool excludeQueries = aOptions->ExcludeQueries();
   for (int32_t nodeIndex = 0; nodeIndex < aSet.Count(); nodeIndex++) {
-    // exclude-queries is implicit when searching, we're only looking at
-    // plan URI nodes
-    if (!aSet[nodeIndex]->IsURI())
+    if (excludeQueries && aSet[nodeIndex]->IsQuery()) {
       continue;
+    }
 
     // RESULTS_AS_TAG_CONTENTS returns a set ordered by place_id and
-    // lastModified. So, to remove duplicates, we can retain the first result
-    // for each uri.
+    // lastModified. The set may contain duplicate, and to remove them we can
+    // just retain the first result.
     if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS &&
-        nodeIndex > 0 && aSet[nodeIndex]->mURI == aSet[nodeIndex-1]->mURI)
+        (!aSet[nodeIndex]->IsURI() ||
+         nodeIndex > 0 && aSet[nodeIndex]->mURI == aSet[nodeIndex-1]->mURI)) {
       continue;
+    }
 
     if (aSet[nodeIndex]->mItemId != -1 && aQueryNode &&
         aQueryNode->mItemId == aSet[nodeIndex]->mItemId) {
       continue;
     }
 
-    // Append the node only if it matches one of the queries.
+    // If there are search terms, we are already getting only uri nodes,
+    // thus we don't need to filter node types. Though, we must check for
+    // matching terms.
     bool appendNode = false;
     for (int32_t queryIndex = 0;
          queryIndex < aQueries.Count() && !appendNode; queryIndex++) {
 
       if (terms[queryIndex]->Length()) {
         // Filter based on search terms.
         // Convert title and url for the current node to UTF16 strings.
         NS_ConvertUTF8toUTF16 nodeTitle(aSet[nodeIndex]->mTitle);
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/queries/test_excludeQueries.js
@@ -0,0 +1,76 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+var bm;
+var fakeQuery;
+var folderShortcut;
+
+add_task(async function setup() {
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  bm = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: "http://example.com/",
+    title: "a bookmark"
+  });
+  fakeQuery = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: "place:terms=foo",
+    title: "a bookmark"
+  });
+  folderShortcut = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: "place:folder=TOOLBAR",
+    title: "a bookmark"
+  });
+
+  checkBookmarkObject(bm);
+  checkBookmarkObject(fakeQuery);
+  checkBookmarkObject(folderShortcut);
+});
+
+add_task(async function test_bookmarks_excludeQueries() {
+  // When excluding queries, we exclude actual queries, but not folder shortcuts.
+  let expectedGuids = [bm.guid, folderShortcut.guid];
+
+  let query = PlacesUtils.history.getNewQuery();
+  let options = PlacesUtils.history.getNewQueryOptions();
+  options.excludeQueries = true;
+  options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
+
+  let root = PlacesUtils.history.executeQuery(query, options).root;
+  root.containerOpen = true;
+
+  Assert.equal(root.childCount, expectedGuids.length, "Checking root child count");
+  for (let i = 0; i < expectedGuids.length; i++) {
+    Assert.equal(root.getChild(i).bookmarkGuid, expectedGuids[i],
+      "should have got the expected item");
+  }
+
+  root.containerOpen = false;
+});
+
+add_task(async function test_search_excludesQueries() {
+  // Searching implicity removes queries and folder shortcuts even if excludeQueries
+  // is not specified.
+  let expectedGuids = [bm.guid];
+
+  let query = PlacesUtils.history.getNewQuery();
+  query.searchTerms = "bookmark";
+
+  let options = PlacesUtils.history.getNewQueryOptions();
+  options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
+
+  let root = PlacesUtils.history.executeQuery(query, options).root;
+  root.containerOpen = true;
+
+  Assert.equal(root.childCount, expectedGuids.length, "Checking root child count");
+  for (let i = 0; i < expectedGuids.length; i++) {
+    Assert.equal(root.getChild(i).bookmarkGuid, expectedGuids[i],
+      "should have got the expected item");
+  }
+
+  root.containerOpen = false;
+});
--- a/toolkit/components/places/tests/queries/xpcshell.ini
+++ b/toolkit/components/places/tests/queries/xpcshell.ini
@@ -4,16 +4,17 @@ skip-if = toolkit == 'android'
 
 [test_415716.js]
 [test_abstime-annotation-domain.js]
 [test_abstime-annotation-uri.js]
 [test_async.js]
 skip-if = (os == 'win' && ccov) # Bug 1423667
 [test_bookmarks.js]
 [test_containersQueries_sorting.js]
+[test_excludeQueries.js]
 [test_history_queries_tags_liveUpdate.js]
 [test_history_queries_titles_liveUpdate.js]
 [test_onlyBookmarked.js]
 [test_options_inherit.js]
 [test_queryMultipleFolder.js]
 [test_querySerialization.js]
 [test_redirects.js]
 [test_results-as-tag-contents-query.js]