Bug 1529642 - When using a search engine @ alias, the only results that should be shown are search suggestion results, including in private contexts r=mak a=lizzard
authorDrew Willcoxon <adw@mozilla.com>
Tue, 26 Feb 2019 19:49:40 +0000
changeset 516201 3bb0dde9e64f4d299f17781180a805912e861a3f
parent 516200 46f0f4418eddc69b08f223b7766ef55725df3920
child 516202 420d4410f71f9cc7a53c13885c973134e5b3bd9a
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, lizzard
bugs1529642
milestone66.0
Bug 1529642 - When using a search engine @ alias, the only results that should be shown are search suggestion results, including in private contexts r=mak a=lizzard We already have a check for stopping the search when using an @ alais or restricting to suggestions. Move the check outside the block where we add suggestions so that it's used regardless of whether we added suggestions. Update the existing test to run in a private context. Differential Revision: https://phabricator.services.mozilla.com/D21140
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -965,32 +965,36 @@ Search.prototype = {
             }
           }
           let alias =
             this._searchEngineAliasMatch &&
             this._searchEngineAliasMatch.alias ||
             "";
           searchSuggestionsCompletePromise =
             this._matchSearchSuggestions(engine, query, alias);
-          // If the user has used a search engine token alias, then the only
-          // results we want to show are suggestions from that engine, so we're
-          // done.  We're also done if we're restricting results to suggestions.
-          if ((this._searchEngineAliasMatch &&
-               this._searchEngineAliasMatch.isTokenAlias) ||
-              this.hasBehavior("restrict")) {
-            // Wait for the suggestions to be added.
-            await searchSuggestionsCompletePromise;
-            this._cleanUpNonCurrentMatches(null);
-            this._autocompleteSearch.finishSearch(true);
-            return;
-          }
         }
       }
     }
-    // In any case, clear previous suggestions.
+
+    // If the user used a search engine token alias, then the only results we
+    // want to show are suggestions from that engine, so we're done.  We're also
+    // done if we're restricting results to suggestions.
+    if ((this._searchEngineAliasMatch &&
+         this._searchEngineAliasMatch.isTokenAlias) ||
+        (this._enableActions &&
+         this.hasBehavior("search") &&
+         this.hasBehavior("restrict"))) {
+      // Wait for the suggestions to be added.
+      await searchSuggestionsCompletePromise;
+      this._cleanUpNonCurrentMatches(null);
+      this._autocompleteSearch.finishSearch(true);
+      return;
+    }
+
+    // Clear previous search suggestions.
     searchSuggestionsCompletePromise.then(() => {
       this._cleanUpNonCurrentMatches(UrlbarUtils.MATCH_GROUP.SUGGESTION);
     });
 
     // Run the adaptive query first.
     await conn.executeCached(this._adaptiveQuery[0], this._adaptiveQuery[1],
                              this._onResultRow.bind(this));
     if (!this.pending)
--- a/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_search_engine_alias.js
@@ -124,86 +124,116 @@ add_task(async function basicGetAndPost(
   await cleanup();
 });
 
 
 // Uses an engine that provides search suggestions.
 add_task(async function engineWithSuggestions() {
   let engine = await addTestSuggestionsEngine();
 
-  await PlacesTestUtils.addVisits(engine.searchForm);
+  // History matches should not appear with @ aliases, so this visit/match
+  // should not appear when searching with the @ alias below.
+  let historyTitle = "fire";
+  await PlacesTestUtils.addVisits({
+    uri: engine.searchForm,
+    title: historyTitle,
+  });
   let historyMatch = {
     value: "http://localhost:9000/search",
-    comment: "test visit for http://localhost:9000/search",
+    comment: historyTitle,
   };
 
-  // Use a normal alias and then one with an "@".  For the @ alias, the only
-  // matches should be the search suggestions -- no history matches.
-  for (let alias of ["moz", "@moz"]) {
-    engine.alias = alias;
-    Assert.equal(engine.alias, alias);
+  // Search in both a non-private and private context.
+  for (let private of [false, true]) {
+    let searchParam = "enable-actions";
+    if (private) {
+      searchParam += " private-window";
+    }
 
-    let expectedMatches = [
-      makeSearchMatch(`${alias} `, {
-        engineName: SUGGESTIONS_ENGINE_NAME,
-        alias,
-        searchQuery: "",
-        heuristic: true,
-      }),
-    ];
-    if (alias[0] != "@") {
-      expectedMatches.push(historyMatch);
-    }
-    await check_autocomplete({
-      search: alias,
-      searchParam: "enable-actions",
-      matches: expectedMatches,
-    });
+    // Use a normal alias and then one with an "@".  For the @ alias, the only
+    // matches should be the search suggestions -- no history matches.
+    for (let alias of ["moz", "@moz"]) {
+      engine.alias = alias;
+      Assert.equal(engine.alias, alias);
 
-    expectedMatches = [
-      makeSearchMatch(`${alias} `, {
-        engineName: SUGGESTIONS_ENGINE_NAME,
-        alias,
-        searchQuery: "",
-        heuristic: true,
-      }),
-    ];
-    if (alias[0] != "@") {
-      expectedMatches.push(historyMatch);
-    }
-    await check_autocomplete({
-      search: `${alias} `,
-      searchParam: "enable-actions",
-      matches: expectedMatches,
-    });
+      // Search for "alias"
+      let expectedMatches = [
+        makeSearchMatch(`${alias} `, {
+          engineName: SUGGESTIONS_ENGINE_NAME,
+          alias,
+          searchQuery: "",
+          heuristic: true,
+        }),
+      ];
+      if (alias[0] != "@") {
+        expectedMatches.push(historyMatch);
+      }
+      await check_autocomplete({
+        search: alias,
+        searchParam,
+        matches: expectedMatches,
+      });
 
-    await check_autocomplete({
-      search: `${alias} fire`,
-      searchParam: "enable-actions",
-      matches: [
-        makeSearchMatch(`${alias} fire`, {
+      // Search for "alias " (trailing space)
+      expectedMatches = [
+        makeSearchMatch(`${alias} `, {
           engineName: SUGGESTIONS_ENGINE_NAME,
           alias,
-          searchQuery: "fire",
+          searchQuery: "",
           heuristic: true,
         }),
-        makeSearchMatch(`${alias} fire foo`, {
+      ];
+      if (alias[0] != "@") {
+        expectedMatches.push(historyMatch);
+      }
+      await check_autocomplete({
+        search: `${alias} `,
+        searchParam,
+        matches: expectedMatches,
+      });
+
+      // Search for "alias historyTitle" -- Include the history title so that
+      // the history result is eligible to be shown.  Whether or not it's
+      // actually shown depends on the alias: If it's an @ alias, it shouldn't
+      // be shown.
+      expectedMatches = [
+        makeSearchMatch(`${alias} ${historyTitle}`, {
           engineName: SUGGESTIONS_ENGINE_NAME,
           alias,
-          searchQuery: "fire",
-          searchSuggestion: "fire foo",
+          searchQuery: historyTitle,
+          heuristic: true,
         }),
-        makeSearchMatch(`${alias} fire bar`, {
-          engineName: SUGGESTIONS_ENGINE_NAME,
-          alias,
-          searchQuery: "fire",
-          searchSuggestion: "fire bar",
-        }),
-      ],
-    });
+      ];
+      // Suggestions should be shown in a non-private context but not in a
+      // private context.
+      if (!private) {
+        expectedMatches.push(
+          makeSearchMatch(`${alias} ${historyTitle} foo`, {
+            engineName: SUGGESTIONS_ENGINE_NAME,
+            alias,
+            searchQuery: historyTitle,
+            searchSuggestion: `${historyTitle} foo`,
+          }),
+          makeSearchMatch(`${alias} ${historyTitle} bar`, {
+            engineName: SUGGESTIONS_ENGINE_NAME,
+            alias,
+            searchQuery: historyTitle,
+            searchSuggestion: `${historyTitle} bar`,
+          })
+        );
+      }
+      if (alias[0] != "@") {
+        expectedMatches.push(historyMatch);
+      }
+      await check_autocomplete({
+        search: `${alias} ${historyTitle}`,
+        searchParam,
+        matches: expectedMatches,
+      });
+    }
   }
 
   engine.alias = "";
   await cleanup();
 });
 
 
 // When the search is simply "@", the results should be a list of all the "@"