Bug 1423896 - When excluding queries within places queries, we shouldn't exclude folder shortcuts. r?mak
MozReview-Commit-ID: 810igliCov8
--- 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]