Bug 1423896 - When excluding queries within places queries, we shouldn't exclude folder shortcuts. r?mak draft
authorMark Banner <standard8@mozilla.com>
Fri, 12 Jan 2018 16:15:25 +0000
changeset 751993 be119e873718839cdf64787039aa48b22a4db881
parent 751903 e1954b02d9e39bdb7c1f17aa95ca9cad5d5c14ae
child 751994 69a5030a0e29f10c59f2bcf5dc358bfe4b6ba703
push id98127
push userbmo:standard8@mozilla.com
push dateWed, 07 Feb 2018 11:39:15 +0000
reviewersmak
bugs1423896
milestone60.0a1
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]