Bug 1643475 - Enable form history / historical searches on 78 release and put them behind the same prefs as remote suggestions. r=mak

We didn't talk specifically about private browsing in the meeting today, but we
did say we want to treat form history like remote suggestions, so this patch
checks `queryContext.allowSearchSuggestions` and `queryContext.isPrivate` in
addition to the `browser.urlbar.suggest.searches` and
`browser.search.suggest.enabled` prefs.

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

browser/app/profile/firefox.js

@@ -286,11 +286,7 @@ pref("browser.urlbar.maxRichResults", 10);
 pref("browser.urlbar.delay", 50);
 
 // The maximum number of historical search results to show.
-#ifdef EARLY_BETA_OR_EARLIER
 pref("browser.urlbar.maxHistoricalSearchSuggestions", 2);
-#else
-pref("browser.urlbar.maxHistoricalSearchSuggestions", 0);
-#endif
 
 // 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

browser/components/urlbar/UrlbarProviderSearchSuggestions.jsm

@@ -131,12 +131,24 @@ class ProviderSearchSuggestions extends UrlbarProvider {
       return false;
     }
 
-    return this._formHistoryCount || this._allowRemoteSuggestions(queryContext);
+    return (
+      this._getFormHistoryCount(queryContext) ||
+      this._allowRemoteSuggestions(queryContext)
+    );
   }
 
-  _allowRemoteSuggestions(queryContext) {
-    // Check preferences and other values that immediately prohibit remote
-    // suggestions.
+  /**
+   * Returns whether suggestions in general are allowed for a given query
+   * context.  If this returns false, then we shouldn't fetch either form
+   * history or remote suggestions.  Otherwise further checks are necessary to
+   * determine whether to allow either form history or remote suggestions; see
+   * _getFormHistoryCount and _allowRemoteSuggestions.
+   *
+   * @param {object} queryContext The query context object
+   * @returns {boolean} True if suggestions in general are allowed and false if
+   *   not.
+   */
+  _allowSuggestions(queryContext) {
     if (
       !queryContext.allowSearchSuggestions ||
       !UrlbarPrefs.get("suggest.searches") ||
@@ -146,6 +158,20 @@ class ProviderSearchSuggestions extends UrlbarProvider {
     ) {
       return false;
     }
+    return true;
+  }
+
+  /**
+   * Returns whether remote suggestions are allowed for a given query context.
+   *
+   * @param {object} queryContext The query context object
+   * @returns {boolean} True if remote suggestions are allowed and false if not.
+   */
+  _allowRemoteSuggestions(queryContext) {
+    // Check whether suggestions in general are allowed.
+    if (!this._allowSuggestions(queryContext)) {
+      return false;
+    }
 
     // Skip all remaining checks and allow remote suggestions at this point if
     // the user used a search engine token alias.  We want "@engine query" to
@@ -324,11 +350,23 @@ class ProviderSearchSuggestions extends UrlbarProvider {
     this.queries.delete(queryContext);
   }
 
-  get _formHistoryCount() {
+  /**
+   * Returns the number of form history entries we should fetch from the
+   * suggestions controller for a given query context.
+   *
+   * @param {object} queryContext The query context object
+   * @returns {number} The number of form history results we should fetch.
+   */
+  _getFormHistoryCount(queryContext) {
+    if (!this._allowSuggestions(queryContext)) {
+      return 0;
+    }
+
     let count = UrlbarPrefs.get("maxHistoricalSearchSuggestions");
     if (!count) {
       return 0;
     }
+
     // If there's a form history entry that equals the search string, the search
     // suggestions controller will include it, and we'll make a result for it.
     // If the heuristic result ends up being a search result, the muxer will
@@ -346,13 +384,15 @@ class ProviderSearchSuggestions extends UrlbarProvider {
 
     this._suggestionsController = new SearchSuggestionController();
     this._suggestionsController.formHistoryParam = queryContext.formHistoryName;
-    this._suggestionsController.maxLocalResults = this._formHistoryCount;
+    this._suggestionsController.maxLocalResults = this._getFormHistoryCount(
+      queryContext
+    );
 
     let allowRemote = this._allowRemoteSuggestions(queryContext);
 
     // Request maxResults + 1 remote suggestions for the same reason we request
     // maxHistoricalSearchSuggestions + 1 form history entries; see
-    // _formHistoryCount.  We allow for the possibility that the engine may
+    // _getFormHistoryCount.  We allow for the possibility that the engine may
     // return a suggestion that's the same as the search string.
     this._suggestionsController.maxRemoteResults = allowRemote
       ? queryContext.maxResults + 1

browser/components/urlbar/tests/browser/browser_remove_match.js

@@ -53,9 +53,20 @@ add_task(async function test_remove_history() {
 
 add_task(async function test_remove_form_history() {
   await SpecialPowers.pushPrefEnv({
-    set: [["browser.urlbar.maxHistoricalSearchSuggestions", 1]],
+    set: [
+      ["browser.urlbar.suggest.searches", true],
+      ["browser.urlbar.maxHistoricalSearchSuggestions", 1],
+    ],
   });
 
+  await Services.search.addEngineWithDetails("test", {
+    method: "GET",
+    template: "http://example.com/?q={searchTerms}",
+  });
+  let engine = Services.search.getEngineByName("test");
+  let originalEngine = await Services.search.getDefault();
+  await Services.search.setDefault(engine);
+
   let formHistoryValue = "foobar";
   await UrlbarTestUtils.formHistory.add([formHistoryValue]);
 
@@ -124,6 +135,8 @@ add_task(async function test_remove_form_history() {
   );
 
   await SpecialPowers.popPrefEnv();
+  await Services.search.setDefault(originalEngine);
+  await Services.search.removeEngine(engine);
 });
 
 // We shouldn't be able to remove a bookmark item.

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

@@ -141,7 +141,6 @@ add_task(async function disabled_urlbarSuggestions() {
     context,
     matches: [
       makeSearchResult(context, { engineName: ENGINE_NAME, heuristic: true }),
-      ...makeExpectedFormHistoryResults(context),
     ],
   });
   await cleanUpSuggestions();
@@ -155,7 +154,6 @@ add_task(async function disabled_allSuggestions() {
     context,
     matches: [
       makeSearchResult(context, { engineName: ENGINE_NAME, heuristic: true }),
-      ...makeExpectedFormHistoryResults(context),
     ],
   });
   await cleanUpSuggestions();
@@ -170,7 +168,6 @@ add_task(async function disabled_privateWindow() {
     context,
     matches: [
       makeSearchResult(context, { engineName: ENGINE_NAME, heuristic: true }),
-      ...makeExpectedFormHistoryResults(context),
     ],
   });
   await cleanUpSuggestions();