Bug 1626946 - Remove search suggestions that dupe a search history result. r=adw

Differential Revision: https://phabricator.services.mozilla.com/D71094

browser/app/profile/firefox.js

@@ -273,6 +273,11 @@ pref("browser.urlbar.delay", 50);
 // The maximum number of historical search results to show.
 pref("browser.urlbar.maxHistoricalSearchSuggestions", 0);
 
+// When true, URLs in the user's history that look like search result pages
+// are styled to look like search engine results instead of the usual history
+// results.
+pref("browser.urlbar.restyleSearches", false);
+
 // The default behavior for the urlbar can be configured to use any combination
 // of the match filters with each additional filter adding more results (union).
 pref("browser.urlbar.suggest.history",              true);

browser/components/extensions/test/xpcshell/test_ext_urlbar.js

@@ -287,6 +287,7 @@ add_task(async function test_onProviderResultsRequested() {
         engine: "Test engine",
         suggestion: undefined,
         keyword: undefined,
+        isSearchHistory: false,
         icon: "",
         keywordOffer: false,
       },

browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm

@@ -55,9 +55,17 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
    * @param {UrlbarQueryContext} context The query context.
    */
   sort(context) {
+    // Remove search suggestions that are duplicates of search history results.
+    context.results = this._dedupeSearchHistoryAndSuggestions(context.results);
+
     // A Search in a Private Window result should only be shown when there are
     // other results, and all of them are searches. It should also not be shown
     // if the user typed an alias, because it's an explicit search engine choice.
+    // We don't show it if there is a search history result. This is because
+    // search history results are RESULT_TYPE.SEARCH but they arrive faster than
+    // search suggestions in most cases because they are being fetched locally.
+    // This leads to the private search result flickering as the suggestions
+    // load in after the search history result.
     let searchInPrivateWindowIndex = context.results.findIndex(
       r => r.type == UrlbarUtils.RESULT_TYPE.SEARCH && r.payload.inPrivateWindow
     );
@@ -74,7 +82,6 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
       // Remove the result.
       context.results.splice(searchInPrivateWindowIndex, 1);
     }
-
     if (!context.results.length) {
       return;
     }
@@ -135,6 +142,75 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
     }
     context.results = sortedResults;
   }
+
+  /**
+   * Takes a list of results and dedupes search history and suggestions. Prefers
+   * search history. Also removes duplicate search history results.
+   * @param {array} results
+   *   A list of results from a UrlbarQueryContext.
+   * @returns {array}
+   *   The deduped list of results.
+   */
+  _dedupeSearchHistoryAndSuggestions(results) {
+    if (
+      !UrlbarPrefs.get("restyleSearches") ||
+      !UrlbarPrefs.get("browser.search.suggest.enabled") ||
+      !UrlbarPrefs.get("suggest.searches")
+    ) {
+      return results;
+    }
+
+    let suggestionResults = [];
+    // historyEnginesBySuggestion maps:
+    //   suggestion ->
+    //     set of engines providing that suggestion from search history
+    let historyEnginesBySuggestion = new Map();
+    for (let i = 0; i < results.length; i++) {
+      let result = results[i];
+      if (
+        !result.heuristic &&
+        groupFromResult(result) == UrlbarUtils.RESULT_GROUP.SUGGESTION
+      ) {
+        if (result.payload.isSearchHistory) {
+          let historyEngines = historyEnginesBySuggestion.get(
+            result.payload.suggestion
+          );
+          if (!historyEngines) {
+            historyEngines = new Set();
+            historyEnginesBySuggestion.set(
+              result.payload.suggestion,
+              historyEngines
+            );
+          }
+          historyEngines.add(result.payload.engine);
+        } else {
+          // Unshift so that we iterate and remove in reverse order below.
+          suggestionResults.unshift([result, i]);
+        }
+      }
+    }
+    for (
+      let i = 0;
+      historyEnginesBySuggestion.size && i < suggestionResults.length;
+      i++
+    ) {
+      let [result, index] = suggestionResults[i];
+      let historyEngines = historyEnginesBySuggestion.get(
+        result.payload.suggestion
+      );
+      if (historyEngines && historyEngines.has(result.payload.engine)) {
+        // This suggestion result has the same suggestion and engine as a search
+        // history result.
+        results.splice(index, 1);
+        historyEngines.delete(result.payload.engine);
+        if (!historyEngines.size) {
+          historyEnginesBySuggestion.delete(result.payload.suggestion);
+        }
+      }
+    }
+
+    return results;
+  }
 }
 
 var UrlbarMuxerUnifiedComplete = new MuxerUnifiedComplete();

browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm

@@ -358,6 +358,7 @@ class ProviderSearchSuggestions extends UrlbarProvider {
               suggestion: [result.suggestion, UrlbarUtils.HIGHLIGHT.SUGGESTED],
               keyword: [alias ? alias : undefined, UrlbarUtils.HIGHLIGHT.TYPED],
               query: [searchString.trim(), UrlbarUtils.HIGHLIGHT.TYPED],
+              isSearchHistory: false,
               icon: [
                 engine.iconURI && !result.suggestion ? engine.iconURI.spec : "",
               ],

browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm

@@ -213,6 +213,7 @@ function makeUrlbarResult(tokens, info) {
               action.params.searchQuery.trim(),
               UrlbarUtils.HIGHLIGHT.TYPED,
             ],
+            isSearchHistory: !!action.params.isSearchHistory,
             icon: [info.icon],
             keywordOffer,
           })

browser/components/urlbar/UrlbarUtils.jsm

@@ -609,6 +609,9 @@ UrlbarUtils.RESULT_PAYLOAD_SCHEMA = {
       query: {
         type: "string",
       },
+      isSearchHistory: {
+        type: "boolean",
+      },
       suggestion: {
         type: "string",
       },

browser/components/urlbar/UrlbarView.jsm

@@ -1383,7 +1383,7 @@ class UrlbarView {
       if (
         result.type != UrlbarUtils.RESULT_TYPE.SEARCH ||
         (!result.heuristic &&
-          !result.payload.suggestion &&
+          (!result.payload.suggestion || result.payload.isSearchHistory) &&
           (!result.payload.inPrivateWindow || result.payload.isPrivateEngine))
       ) {
         continue;

browser/components/urlbar/tests/UrlbarTestUtils.jsm

@@ -160,6 +160,7 @@ var UrlbarTestUtils = {
         keyword: result.payload.keyword,
         query: result.payload.query,
         suggestion: result.payload.suggestion,
+        isSearchHistory: result.payload.isSearchHistory,
         inPrivateWindow: result.payload.inPrivateWindow,
         isPrivateEngine: result.payload.isPrivateEngine,
       };

browser/components/urlbar/tests/browser/browser.ini

@@ -132,11 +132,14 @@ run-if = e10s
 [browser_removeUnsafeProtocolsFromURLBarPaste.js]
 fail-if = (os == 'linux' && os_version == '18.04')  # Bug 1600182
 [browser_restoreEmptyInput.js]
+[browser_restyleSearches.js]
+support-files =
+  searchSuggestionEngine.xml
+  searchSuggestionEngine2.xml
+  searchSuggestionEngine.sjs
 [browser_resultSpan.js]
 [browser_retainedResultsOnFocus.js]
 [browser_revert.js]
-[browser_search_favicon.js]
-skip-if = true # Bug 1526222 - Doesn't currently work with QuantumBar
 [browser_searchFunction.js]
 [browser_searchSettings.js]
 [browser_searchSingleWordNotification.js]

browser/components/urlbar/tests/browser/browser_action_searchengine.js

@@ -56,6 +56,7 @@ async function testSearch(win, expectedName, expectedBaseUrl) {
       keyword: undefined,
       query: "open a search",
       suggestion: undefined,
+      isSearchHistory: false,
       inPrivateWindow: undefined,
       isPrivateEngine: undefined,
     },

browser/components/urlbar/tests/browser/browser_autoFill_trimURLs.js

@@ -107,6 +107,7 @@ const tests = [
     searchParams: {
       engine: "Google",
       query: "http://",
+      isSearchHistory: false,
     },
   },
   {
@@ -118,6 +119,7 @@ const tests = [
     searchParams: {
       engine: "Google",
       query: "https://",
+      isSearchHistory: false,
     },
   },
   {

browser/components/urlbar/tests/browser/browser_restyleSearches.js

@@ -0,0 +1,357 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Tests restyled search history results.
+ */
+
+const RESTYLE_PREF = "browser.urlbar.restyleSearches";
+const SUGGEST_URLBAR_PREF = "browser.urlbar.suggest.searches";
+const PRIVATE_SEARCH_PREF = "browser.search.separatePrivateDefault.ui.enabled";
+// We use the slow engine to ensure that the search history item appears in
+// results before the equivalent suggestion, to better approximate real-world
+// behavior.
+const TEST_ENGINE_BASENAME = "searchSuggestionEngineSlow.xml";
+const TEST_ENGINE_2_BASENAME = "searchSuggestionEngine2.xml";
+const TEST_QUERY = "test query";
+
+let gSearchUri;
+
+add_task(async function setup() {
+  await SpecialPowers.pushPrefEnv({
+    set: [
+      [SUGGEST_URLBAR_PREF, true],
+      [RESTYLE_PREF, true],
+      [PRIVATE_SEARCH_PREF, false],
+    ],
+  });
+
+  let engine = await SearchTestUtils.promiseNewSearchEngine(
+    getRootDirectory(gTestPath) + TEST_ENGINE_BASENAME
+  );
+  let oldDefaultEngine = await Services.search.getDefault();
+  await Services.search.setDefault(engine);
+
+  gSearchUri = (await Services.search.getDefault()).getSubmission(
+    `${TEST_QUERY}foo`
+  ).uri;
+
+  await PlacesTestUtils.addVisits({
+    uri: gSearchUri.spec,
+    title: `${TEST_QUERY}foo`,
+  });
+
+  registerCleanupFunction(async () => {
+    Services.search.setDefault(oldDefaultEngine);
+    await PlacesUtils.history.clear();
+  });
+});
+
+/**
+ * Tests that a search history item takes the place of and is restyled as its
+ * equivalent search suggestion.
+ */
+add_task(async function restyleSearches() {
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    waitForFocus: SimpleTest.waitForFocus,
+    value: TEST_QUERY,
+  });
+
+  Assert.equal(UrlbarTestUtils.getResultCount(window), 3);
+  let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo`);
+  Assert.ok(
+    result.searchParams.isSearchHistory,
+    "This result should be a restyled search history result."
+  );
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}foo`);
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 2);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}bar`);
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}bar`);
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  await UrlbarTestUtils.promisePopupClose(window);
+});
+
+/**
+ * Tests that equivalent search history and search suggestions both appear when
+ * the restyleSearches pref is off.
+ */
+add_task(async function displaySearchHistoryAndSuggestions() {
+  await SpecialPowers.pushPrefEnv({
+    set: [[RESTYLE_PREF, false]],
+  });
+
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    waitForFocus: SimpleTest.waitForFocus,
+    value: TEST_QUERY,
+  });
+
+  // We should now also have the search history result.
+  Assert.equal(UrlbarTestUtils.getResultCount(window), 4);
+
+  let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo`);
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}foo`);
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 2);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}bar`);
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}bar`);
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 3);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.URL);
+  Assert.equal(result.source, UrlbarUtils.RESULT_SOURCE.HISTORY);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo`);
+  Assert.equal(result.url, gSearchUri.spec);
+
+  await SpecialPowers.popPrefEnv();
+  await UrlbarTestUtils.promisePopupClose(window);
+});
+
+/**
+ * Tests that search history results from non-default engines do not replace
+ * equivalent search suggestions from different engines.
+ *
+ * Note: The search history result from the original engine should still appear
+ *       restyled.
+ */
+add_task(async function alternateEngine() {
+  let engine2 = await SearchTestUtils.promiseNewSearchEngine(
+    getRootDirectory(gTestPath) + TEST_ENGINE_2_BASENAME
+  );
+  let previousTestEngine = await Services.search.getDefault();
+  await Services.search.setDefault(engine2);
+
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    waitForFocus: SimpleTest.waitForFocus,
+    value: TEST_QUERY,
+  });
+
+  // We should have the search history item from the original engine, restyled
+  // as a suggestion.
+  Assert.equal(UrlbarTestUtils.getResultCount(window), 4);
+
+  let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo`);
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}foo`);
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "browser_searchSuggestionEngine2 searchSuggestionEngine2.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 2);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}bar`);
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}bar`);
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "browser_searchSuggestionEngine2 searchSuggestionEngine2.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 3);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo`);
+  Assert.ok(
+    result.searchParams.isSearchHistory,
+    "This result should be a restyled search history result."
+  );
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}foo`);
+  // Note the comparison with searchSuggestionEngineSlow.xml, not
+  // searchSuggestionEngine2.xml.
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  await Services.search.setDefault(previousTestEngine);
+  await UrlbarTestUtils.promisePopupClose(window);
+});
+
+/**
+ * Tests that when the user types part of a search history URL but not the query
+ * in that URL, we show a normal history result that is not restyled.
+ */
+add_task(async function onlyRestyleWhenQueriesMatch() {
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    waitForFocus: SimpleTest.waitForFocus,
+    value: "mochi",
+  });
+
+  // We should now also have the search history result.
+  Assert.equal(UrlbarTestUtils.getResultCount(window), 4);
+
+  let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, "mochifoo");
+  Assert.equal(result.searchParams.suggestion, "mochifoo");
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 2);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, "mochibar");
+  Assert.equal(result.searchParams.suggestion, "mochibar");
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 3);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.URL);
+  Assert.equal(result.source, UrlbarUtils.RESULT_SOURCE.HISTORY);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo`);
+  Assert.equal(result.url, gSearchUri.spec);
+
+  await UrlbarTestUtils.promisePopupClose(window);
+});
+
+/**
+ * Tests that search history items that do not exactly match a search suggestion
+ * URL still appear in results, do not replace suggestions, and are not restyled.
+ *
+ * Note: This test should run last because it adds a new history item.
+ */
+add_task(async function searchHistoryNotExactMatch() {
+  let irrelevantSearchUri = `${gSearchUri.spec}&irrelevantParameter=1`;
+
+  // This history item will be removed when the setup test runs the cleanup
+  // function.
+  await PlacesTestUtils.addVisits({
+    uri: irrelevantSearchUri,
+    title: `${TEST_QUERY}foo irrelevant`,
+  });
+
+  await UrlbarTestUtils.promiseAutocompleteResultPopup({
+    window,
+    waitForFocus: SimpleTest.waitForFocus,
+    value: TEST_QUERY,
+  });
+
+  // We should have the irrelevant history result, but the history result that
+  // is an exact match should have replaced the equivalent suggestion.
+  Assert.equal(UrlbarTestUtils.getResultCount(window), 4);
+
+  let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo`);
+  Assert.ok(
+    result.searchParams.isSearchHistory,
+    "This result should be a restyled search history result."
+  );
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}foo`);
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 2);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}bar`);
+  Assert.equal(result.searchParams.suggestion, `${TEST_QUERY}bar`);
+  Assert.ok(
+    !result.searchParams.isSearchHistory,
+    "This result should be a normal search suggestion."
+  );
+  Assert.equal(
+    result.displayed.action,
+    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
+      "searchSuggestionEngineSlow.xml",
+    ]),
+    "Should have the correct action text"
+  );
+
+  result = await UrlbarTestUtils.getDetailsOfResultAt(window, 3);
+  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.URL);
+  Assert.equal(result.source, UrlbarUtils.RESULT_SOURCE.HISTORY);
+  Assert.equal(result.displayed.title, `${TEST_QUERY}foo irrelevant`);
+  Assert.equal(result.url, irrelevantSearchUri);
+
+  await UrlbarTestUtils.promisePopupClose(window);
+});

browser/components/urlbar/tests/browser/browser_search_favicon.js

@@ -1,67 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-/**
- * Tests that a restyled search result is correctly displayed.
- */
-
-var gOriginalEngine;
-var gEngine;
-var gRestyleSearchesPref = "browser.urlbar.restyleSearches";
-
-registerCleanupFunction(async () => {
-  Services.prefs.clearUserPref(gRestyleSearchesPref);
-  await Services.search.setDefault(gOriginalEngine);
-  await Services.search.removeEngine(gEngine);
-  return PlacesUtils.history.clear();
-});
-
-add_task(async function() {
-  Services.prefs.setBoolPref(gRestyleSearchesPref, true);
-
-  // This test is sensitive to the mouse position hovering awesome
-  // bar elements, so make sure it doesnt
-  await EventUtils.synthesizeNativeMouseMove(document.documentElement, 0, 0);
-});
-
-add_task(async function() {
-  await Services.search.addEngineWithDetails("SearchEngine", {
-    method: "GET",
-    template: "http://s.example.com/search",
-  });
-  gEngine = Services.search.getEngineByName("SearchEngine");
-  gEngine.addParam("q", "{searchTerms}", null);
-  gOriginalEngine = await Services.search.getDefault();
-  await Services.search.setDefault(gEngine);
-
-  await PlacesTestUtils.addVisits({
-    uri: "http://s.example.com/search?q=foobar&client=1",
-    title: "Foo - SearchEngine Search",
-  });
-
-  await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:mozilla");
-
-  // The first autocomplete result has the action searchengine, while
-  // the second result is the "search favicon" element.
-  await UrlbarTestUtils.promiseAutocompleteResultPopup({
-    window,
-    waitForFocus: SimpleTest.waitForFocus,
-    value: "foo",
-  });
-  let result = await UrlbarTestUtils.getDetailsOfResultAt(window, 1);
-
-  Assert.equal(result.type, UrlbarUtils.RESULT_TYPE.SEARCH);
-  Assert.equal(result.displayed.title, "foobar");
-
-  Assert.equal(
-    result.displayed.action,
-    UrlbarUtils.strings.formatStringFromName("searchWithEngine", [
-      "SearchEngine",
-    ]),
-    "Should have the correct action text"
-  );
-
-  gBrowser.removeCurrentTab();
-});

browser/components/urlbar/tests/unit/head.js

@@ -278,6 +278,7 @@ function makeSearchResult(
         typeof query != "undefined" ? query : queryContext.searchString.trim(),
         UrlbarUtils.HIGHLIGHT.TYPED,
       ],
+      isSearchHistory: false,
       icon: [engineIconUri ? engineIconUri : ""],
       keywordOffer,
     })

toolkit/components/places/PlacesSearchAutocompleteProvider.jsm

@@ -299,8 +299,9 @@ var PlacesSearchAutocompleteProvider = Object.freeze({
    * @return An object with the following properties, or null if the URL does
    *         not represent a search result:
    *         {
-   *           engineName: The display name of the search engine.
+   *           engine: The search engine, as an nsISearchEngine.
    *           terms: The originally sought terms extracted from the URI.
+   *           termsParameterName: The engine's search-string parameter.
    *         }
    *
    * @remarks The asynchronous ensureInitialized function must be called before
@@ -317,8 +318,9 @@ var PlacesSearchAutocompleteProvider = Object.freeze({
     let parseUrlResult = Services.search.parseSubmissionURL(url);
     return (
       parseUrlResult.engine && {
-        engineName: parseUrlResult.engine.name,
+        engine: parseUrlResult.engine,
         terms: parseUrlResult.terms,
+        termsParameterName: parseUrlResult.termsParameterName,
       }
     );
   },

toolkit/components/places/UnifiedComplete.jsm

@@ -1913,10 +1913,8 @@ Search.prototype = {
       return;
     }
 
-    // Do not apply the special style if the user is doing a search from the
-    // location bar but the entered terms match an irrelevant portion of the
-    // URL. For example, "https://www.google.com/search?q=terms&client=firefox"
-    // when searching for "Firefox".
+    // Here we check that the user typed all or part of the search string in the
+    // search history result.
     let terms = parseResult.terms.toLowerCase();
     if (
       this._searchTokens.length &&
@@ -1925,15 +1923,46 @@ Search.prototype = {
       return;
     }
 
+    // The URL for the search suggestion formed by the user's typed query.
+    let [typedSuggestionUrl] = UrlbarUtils.getSearchQueryUrl(
+      parseResult.engine,
+      this._searchTokens.map(t => t.value).join(" ")
+    );
+
+    let historyParams = new URL(match.value).searchParams;
+    let typedParams = new URL(typedSuggestionUrl).searchParams;
+
+    // Checking the two URLs have the same query parameters with the same
+    // values, or a subset value in the case of the query parameter.
+    // Parameter order doesn't matter.
+    if (
+      Array.from(historyParams).length != Array.from(typedParams).length ||
+      !Array.from(historyParams.entries()).every(
+        ([key, value]) =>
+          // We want to match all non-search-string GET parameters exactly, to avoid
+          // restyling non-first pages of search results, or image results as web
+          // results.
+          // We let termsParameterName through because we already checked that the
+          // typed query is a subset of the search history query above with
+          // this._searchTokens.every(...).
+          key == parseResult.termsParameterName ||
+          value === typedParams.get(key)
+      )
+    ) {
+      return;
+    }
+
     // Turn the match into a searchengine action with a favicon.
     match.value = makeActionUrl("searchengine", {
-      engineName: parseResult.engineName,
+      engineName: parseResult.engine.name,
       input: parseResult.terms,
+      searchSuggestion: parseResult.terms,
       searchQuery: parseResult.terms,
+      isSearchHistory: true,
     });
-    match.comment = parseResult.engineName;
+    match.comment = parseResult.engine.name;
     match.icon = match.icon || match.iconUrl;
-    match.style = "action searchengine favicon";
+    match.style = "action searchengine favicon suggestion";
   },
 
   _addMatch(match) {

toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js

@@ -445,6 +445,9 @@ function makeSearchMatch(input, extra = {}) {
     params.searchSuggestion = extra.searchSuggestion;
     style.push("suggestion");
   }
+  if ("isSearchHistory" in extra) {
+    params.isSearchHistory = extra.isSearchHistory;
+  }
   return {
     uri: makeActionURI("searchengine", params),
     title: params.engineName,

toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js

@@ -163,7 +163,7 @@ add_task(async function test_parseSubmissionURL_basic() {
   let result = PlacesSearchAutocompleteProvider.parseSubmissionURL(
     submissionURL
   );
-  Assert.equal(result.engineName, engine.name);
+  Assert.equal(result.engine.name, engine.name);
   Assert.equal(result.terms, "terms");
 
   result = PlacesSearchAutocompleteProvider.parseSubmissionURL(

toolkit/components/places/tests/unifiedcomplete/test_search_engine_restyle.js

@@ -11,42 +11,50 @@ add_task(async function test_searchEngine() {
   engine.addParam("q", "{searchTerms}", null);
   registerCleanupFunction(async () => Services.search.removeEngine(engine));
 
-  let uri1 = NetUtil.newURI("http://s.example.com/search?q=Terms&client=1");
-  let uri2 = NetUtil.newURI("http://s.example.com/search?q=Terms&client=2");
+  let uri = NetUtil.newURI("http://s.example.com/search?q=Terms");
   await PlacesTestUtils.addVisits({
-    uri: uri1,
-    title: "Terms - SearchEngine Search",
-  });
-  await PlacesTestUtils.addBookmarkWithDetails({
-    uri: uri2,
+    uri,
     title: "Terms - SearchEngine Search",
   });
 
-  info("Past search terms should be styled, unless bookmarked");
+  info("Past search terms should be styled.");
   Services.prefs.setBoolPref("browser.urlbar.restyleSearches", true);
   await check_autocomplete({
     search: "term",
     matches: [
       makeSearchMatch("Terms", {
         engineName: "SearchEngine",
+        searchSuggestion: "Terms",
+        isSearchHistory: true,
         style: ["favicon"],
       }),
+    ],
+  });
+
+  info("Bookmarked past searches should not be restyled");
+  await PlacesTestUtils.addBookmarkWithDetails({
+    uri,
+    title: "Terms - SearchEngine Search",
+  });
+
+  await check_autocomplete({
+    search: "term",
+    matches: [
       {
-        uri: uri2,
+        uri,
         title: "Terms - SearchEngine Search",
         style: ["bookmark"],
       },
     ],
   });
 
+  await PlacesUtils.bookmarks.eraseEverything();
+
   info("Past search terms should not be styled if restyling is disabled");
   Services.prefs.setBoolPref("browser.urlbar.restyleSearches", false);
   await check_autocomplete({
     search: "term",
-    matches: [
-      { uri: uri1, title: "Terms - SearchEngine Search" },
-      { uri: uri2, title: "Terms - SearchEngine Search", style: ["bookmark"] },
-    ],
+    matches: [{ uri, title: "Terms - SearchEngine Search" }],
   });
 
   await cleanup();

toolkit/components/search/SearchService.jsm

@@ -481,9 +481,16 @@ var gInitialized = false;
 var gReinitializing = false;
 
 // nsISearchParseSubmissionResult
-function ParseSubmissionResult(engine, terms, termsOffset, termsLength) {
+function ParseSubmissionResult(
+  engine,
+  terms,
+  termsParameterName,
+  termsOffset,
+  termsLength
+) {
   this._engine = engine;
   this._terms = terms;
+  this._termsParameterName = termsParameterName;
   this._termsOffset = termsOffset;
   this._termsLength = termsLength;
 }
@@ -494,6 +501,9 @@ ParseSubmissionResult.prototype = {
   get terms() {
     return this._terms;
   },
+  get termsParameterName() {
+    return this._termsParameterName;
+  },
   get termsOffset() {
     return this._termsOffset;
   },
@@ -504,7 +514,7 @@ ParseSubmissionResult.prototype = {
 };
 
 const gEmptyParseSubmissionResult = Object.freeze(
-  new ParseSubmissionResult(null, "", -1, 0)
+  new ParseSubmissionResult(null, "", "", -1, 0)
 );
 
 /**
@@ -3577,7 +3587,14 @@ SearchService.prototype = {
       return gEmptyParseSubmissionResult;
     }
 
-    return new ParseSubmissionResult(mapEntry.engine, terms, offset, length);
+    let submission = new ParseSubmissionResult(
+      mapEntry.engine,
+      terms,
+      mapEntry.termsParameterName,
+      offset,
+      length
+    );
+    return submission;
   },
 
   // nsIObserver

toolkit/components/search/nsISearchService.idl

@@ -197,6 +197,11 @@ interface nsISearchParseSubmissionResult : nsISupports
    */
   readonly attribute AString terms;
 
+  /**
+   * The name of the query parameter used by `engine` for queries. E.g. "q".
+   */
+  readonly attribute AString termsParameterName;
+
   /**
    * The offset of the string |terms| in the URL passed in to
    * nsISearchEngine::parseSubmissionURL, or -1 if the URL does not represent