Bug 1599842 - Restrict to a specific search engine from the QueryContext. r=adw
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 20 Dec 2019 09:56:06 +0000
changeset 508014 55a088881ca63b90308dbb79a80ea30df43788db
parent 508013 0433c276ce9b1bdb8c76b91084479910ffa927ed
child 508015 b4319aae5a4cabe5fa248aee3617222f3dbfc91f
push id103746
push usermak77@bonardo.net
push dateFri, 20 Dec 2019 10:42:55 +0000
treeherderautoland@55a088881ca6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1599842
milestone73.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1599842 - Restrict to a specific search engine from the QueryContext. r=adw This is the last part needed to be able to restrict results without an explicit typed token (either restriction or alias). Note this is all preparatory work, there isn't a design for a feature using this yet, but we know at a certain point we want a more usable representation of aliases and restriction tokens and eventually a mode picker UI (search button). Differential Revision: https://phabricator.services.mozilla.com/D57781
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarProviderPrivateSearch.jsm
browser/components/urlbar/UrlbarUtils.jsm
browser/components/urlbar/docs/overview.rst
browser/components/urlbar/tests/unit/test_UrlbarQueryContext_restrictSource.js
toolkit/components/places/UnifiedComplete.jsm
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -409,17 +409,19 @@ class UrlbarController {
           result.autofill
         ) {
           if (result.type == UrlbarUtils.RESULT_TYPE.SEARCH) {
             // Speculative connect only if search suggestions are enabled.
             if (
               UrlbarPrefs.get("suggest.searches") &&
               UrlbarPrefs.get("browser.search.suggest.enabled")
             ) {
-              let engine = Services.search.defaultEngine;
+              let engine = Services.search.getEngineByName(
+                result.payload.engine
+              );
               UrlbarUtils.setupSpeculativeConnection(
                 engine,
                 this.browserWindow
               );
             }
           } else if (result.autofill) {
             UrlbarUtils.setupSpeculativeConnection(url, this.browserWindow);
           }
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -395,29 +395,39 @@ class UrlbarInput {
       numChars,
       selIndex: this.view.selectedRowIndex,
       selType,
     });
 
     try {
       new URL(url);
     } catch (ex) {
+      // This is not a URL, so it must be a search or a keyword.
+
+      // TODO (Bug 1604927): If the urlbar results are restricted to a specific
+      // engine, here we must search with that specific engine; indeed the
+      // docshell wouldn't know about our engine restriction.
+      // Also remember to invoke this._recordSearch, after replacing url with
+      // the appropriate engine submission url.
+
       let browser = this.window.gBrowser.selectedBrowser;
       let lastLocationChange = browser.lastLocationChange;
-
       UrlbarUtils.getShortcutOrURIAndPostData(url).then(data => {
+        // Because this happens asynchronously, we must verify that the browser
+        // location did not change in the meanwhile.
         if (
           where != "current" ||
           browser.lastLocationChange == lastLocationChange
         ) {
           openParams.postData = data.postData;
           openParams.allowInheritPrincipal = data.mayInheritPrincipal;
           this._loadURL(data.url, where, openParams, null, browser);
         }
       });
+      // Bail out, because we will handle the _loadURL call asynchronously.
       return;
     }
 
     this._loadURL(url, where, openParams);
   }
 
   handleRevert() {
     this.window.gBrowser.userTypedValue = null;
--- a/browser/components/urlbar/UrlbarProviderPrivateSearch.jsm
+++ b/browser/components/urlbar/UrlbarProviderPrivateSearch.jsm
@@ -120,17 +120,19 @@ class ProviderPrivateSearch extends Urlb
         .filter(t => t.type != UrlbarTokenizer.TYPE.RESTRICT_SEARCH)
         .map(t => t.value)
         .join(" ");
     }
 
     let instance = {};
     this.queries.set(queryContext, instance);
 
-    let engine = await Services.search.getDefaultPrivate();
+    let engine = queryContext.engineName
+      ? Services.search.getEngineByName(queryContext.engineName)
+      : await Services.search.getDefaultPrivate();
     let isPrivateEngine =
       separatePrivateDefault && engine != (await Services.search.getDefault());
     logger.info(`isPrivateEngine: ${isPrivateEngine}`);
 
     // This is a delay added before returning results, to avoid flicker.
     // Our result must appear only when all results are searches, but if search
     // results arrive first, then the muxer would insert our result and then
     // immediately remove it when non-search results arrive.
--- a/browser/components/urlbar/UrlbarUtils.jsm
+++ b/browser/components/urlbar/UrlbarUtils.jsm
@@ -539,43 +539,47 @@ class UrlbarQueryContext {
    * @param {number} options.maxResults
    *   The maximum number of results that will be displayed for this query.
    * @param {boolean} options.allowAutofill
    *   Whether or not to allow providers to include autofill results.
    * @param {number} options.userContextId
    *   The container id where this context was generated, if any.
    * @param {array} [options.sources]
    *   A list of acceptable UrlbarUtils.RESULT_SOURCE for the context.
+   * @param {string} [options.engineName]
+   *   If sources is restricting to just SEARCH, this property can be used to
+   *   pick a specific search engine, by setting it to the name under which the
+   *   engine is registered with the search service.
    */
   constructor(options = {}) {
     this._checkRequiredOptions(options, [
       "allowAutofill",
       "isPrivate",
       "maxResults",
       "searchString",
     ]);
 
     if (isNaN(parseInt(options.maxResults))) {
       throw new Error(
         `Invalid maxResults property provided to UrlbarQueryContext`
       );
     }
 
-    if (options.providers) {
-      if (!Array.isArray(options.providers) || !options.providers.length) {
-        throw new Error(`Invalid providers list`);
+    // Manage optional properties of options.
+    for (let [prop, checkFn] of [
+      ["providers", v => Array.isArray(v) && v.length],
+      ["sources", v => Array.isArray(v) && v.length],
+      ["engineName", v => typeof v == "string" && !!v.length],
+    ]) {
+      if (options[prop]) {
+        if (!checkFn(options[prop])) {
+          throw new Error(`Invalid value for option "${prop}"`);
+        }
+        this[prop] = options[prop];
       }
-      this.providers = options.providers;
-    }
-
-    if (options.sources) {
-      if (!Array.isArray(options.sources) || !options.sources.length) {
-        throw new Error(`Invalid sources list`);
-      }
-      this.sources = options.sources;
     }
 
     this.lastResultCount = 0;
     this.userContextId =
       options.userContextId ||
       Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID;
   }
 
--- a/browser/components/urlbar/docs/overview.rst
+++ b/browser/components/urlbar/docs/overview.rst
@@ -41,16 +41,20 @@ It is augmented as it progresses through
     muxer; // {string} Name of a registered muxer. Muxers can be registered
            // through the UrlbarProvidersManager.
     providers; // {array} List of registered provider names. Providers can be
                // registered through the UrlbarProvidersManager.
     sources: {array} list of accepted UrlbarUtils.RESULT_SOURCE for the context.
             // This allows to switch between different search modes. If not
             // provided, a default will be generated by the Model, depending on
             // the search string.
+    engineName: // {string} if sources is restricting to just SEARCH, this
+                // property can be used to pick a specific search engine, by
+                // setting it to the name under which the engine is registered
+                // with the search service.
 
     // Properties added by the Model.
     results; // {array} list of UrlbarResult objects.
     tokens; // {array} tokens extracted from the searchString, each token is an
             // object in the form {type, value, lowerCaseValue}.
   }
 
 
--- a/browser/components/urlbar/tests/unit/test_UrlbarQueryContext_restrictSource.js
+++ b/browser/components/urlbar/tests/unit/test_UrlbarQueryContext_restrictSource.js
@@ -67,22 +67,70 @@ add_task(async function test_restriction
   results = await get_results({
     sources: [UrlbarUtils.RESULT_SOURCE.SEARCH],
     searchString: "match",
   });
   Assert.ok(
     !results.some(r => r.payload.engine != "engine-suggestions.xml"),
     "All the results should be search results"
   );
+
+  info("search restrict should ignore restriction token");
+  results = await get_results({
+    sources: [UrlbarUtils.RESULT_SOURCE.SEARCH],
+    searchString: `${UrlbarTokenizer.RESTRICT.BOOKMARKS} match`,
+  });
+  Assert.ok(
+    !results.some(r => r.payload.engine != "engine-suggestions.xml"),
+    "All the results should be search results"
+  );
+  Assert.equal(
+    results[0].payload.query,
+    `${UrlbarTokenizer.RESTRICT.BOOKMARKS} match`,
+    "The restriction token should be ignored and not stripped"
+  );
+
+  info("search restrict with alias");
+  let aliasEngine = await Services.search.addEngineWithDetails("Test", {
+    alias: "match",
+    template: "http://example.com/?search={searchTerms}",
+  });
+  registerCleanupFunction(async function() {
+    await Services.search.removeEngine(aliasEngine);
+  });
+  results = await get_results({
+    sources: [UrlbarUtils.RESULT_SOURCE.SEARCH],
+    searchString: "match this",
+  });
+  Assert.ok(
+    !results.some(r => r.payload.engine != "engine-suggestions.xml"),
+    "All the results should be search results and the alias should be ignored"
+  );
+  Assert.equal(
+    results[0].payload.query,
+    `match this`,
+    "The restriction token should be ignored and not stripped"
+  );
+
+  info("search restrict with other engine");
+  results = await get_results({
+    sources: [UrlbarUtils.RESULT_SOURCE.SEARCH],
+    searchString: "match",
+    engineName: "Test",
+  });
+  Assert.ok(
+    !results.some(r => r.payload.engine != "Test"),
+    "All the results should be search results from the Test engine"
+  );
 });
 
 async function get_results(test) {
   let controller = UrlbarTestUtils.newMockController();
   let queryContext = createContext(test.searchString, {
     allowAutofill: false,
     isPrivate: false,
     maxResults: 10,
     sources: test.sources,
+    engineName: test.engineName,
   });
   await controller.startQuery(queryContext);
-  info(JSON.stringify(queryContext.results));
   return queryContext.results;
 }
--- a/toolkit/components/places/UnifiedComplete.jsm
+++ b/toolkit/components/places/UnifiedComplete.jsm
@@ -1,14 +1,14 @@
 /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
  * vim: sw=2 ts=2 sts=2 expandtab
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-/* eslint complexity: ["error", 50] */
+/* eslint complexity: ["error", 53] */
 
 "use strict";
 
 // Constants
 
 const MS_PER_DAY = 86400000; // 24 * 60 * 60 * 1000
 
 // AutoComplete query type constants.
@@ -736,41 +736,50 @@ function Search(
   if (
     queryContext &&
     queryContext.restrictSource &&
     sourceToBehaviorMap.has(queryContext.restrictSource)
   ) {
     this._searchTokens = tokens;
     this._behavior = 0;
     this.setBehavior("restrict");
-    this.setBehavior(sourceToBehaviorMap.get(queryContext.restrictSource));
+    let behavior = sourceToBehaviorMap.get(queryContext.restrictSource);
+    this.setBehavior(behavior);
+    if (behavior == "search" && queryContext.engineName) {
+      this._engineName = queryContext.engineName;
+    }
+    if (behavior != "search") {
+      prohibitSearchSuggestions = true;
+    }
+    // When we are in restrict mode, all the tokens are valid for searching, so
+    // there is no _heuristicToken.
+    this._heuristicToken = null;
   } else {
     this._searchTokens = this.filterTokens(tokens);
+    // The heuristic token is the first filtered search token, but only when it's
+    // actually the first thing in the search string.  If a prefix or restriction
+    // character occurs first, then the heurstic token is null.  We use the
+    // heuristic token to help determine the heuristic result.  It may be a Places
+    // keyword, a search engine alias, an extension keyword, or simply a URL or
+    // part of the search string the user has typed.  We won't know until we
+    // create the heuristic result.
+    let firstToken = !!this._searchTokens.length && this._searchTokens[0].value;
+    this._heuristicToken =
+      firstToken && this._trimmedOriginalSearchString.startsWith(firstToken)
+        ? firstToken
+        : null;
   }
 
   // Set the right JavaScript behavior based on our preference.  Note that the
   // preference is whether or not we should filter JavaScript, and the
   // behavior is if we should search it or not.
   if (!UrlbarPrefs.get("filter.javascript")) {
     this.setBehavior("javascript");
   }
 
-  // The heuristic token is the first filtered search token, but only when it's
-  // actually the first thing in the search string.  If a prefix or restriction
-  // character occurs first, then the heurstic token is null.  We use the
-  // heuristic token to help determine the heuristic result.  It may be a Places
-  // keyword, a search engine alias, an extension keyword, or simply a URL or
-  // part of the search string the user has typed.  We won't know until we
-  // create the heuristic result.
-  let firstToken = !!this._searchTokens.length && this._searchTokens[0].value;
-  this._heuristicToken =
-    firstToken && this._trimmedOriginalSearchString.startsWith(firstToken)
-      ? firstToken
-      : null;
-
   this._keywordSubstitute = null;
 
   this._prohibitSearchSuggestions = prohibitSearchSuggestions;
 
   this._listener = autocompleteListener;
   this._autocompleteSearch = autocompleteSearch;
 
   // Create a new result to add eventual matches.  Note we need a result
@@ -978,21 +987,25 @@ Search.prototype = {
     // Used by stop() to interrupt an eventual running statement.
     this.interrupt = () => {
       // Interrupt any ongoing statement to run the search sooner.
       if (!UrlbarProvidersManager.interruptLevel) {
         conn.interrupt();
       }
     };
 
-    // Since we call the synchronous parseSubmissionURL function later, we must
-    // wait for the initialization of PlacesSearchAutocompleteProvider first.
-    await PlacesSearchAutocompleteProvider.ensureInitialized();
-    if (!this.pending) {
-      return;
+    if (UrlbarPrefs.get("restyleSearches")) {
+      // This explicit initialization is only necessary for
+      // _maybeRestyleSearchMatch, because it calls the synchronous
+      // parseSubmissionURL that can't wait for async initialization of
+      // PlacesSearchAutocompleteProvider.
+      await PlacesSearchAutocompleteProvider.ensureInitialized();
+      if (!this.pending) {
+        return;
+      }
     }
 
     // For any given search, we run many queries/heuristics:
     // 1) by alias (as defined in SearchService)
     // 2) inline completion from search engine resultDomains
     // 3) inline completion for origins (this._originQuery) or urls (this._urlQuery)
     // 4) directly typed in url (ie, can be navigated to as-is)
     // 5) submission for the current search engine
@@ -1106,20 +1119,22 @@ Search.prototype = {
           UrlbarPrefs.get("maxCharsForSearchSuggestions")
         );
         // Don't add suggestions if the query may expose sensitive information.
         if (!this._prohibitSearchSuggestionsFor(query)) {
           let engine;
           if (this._searchEngineAliasMatch) {
             engine = this._searchEngineAliasMatch.engine;
           } else {
-            engine = await PlacesSearchAutocompleteProvider.currentEngine(
-              this._inPrivateWindow
-            );
-            if (!this.pending) {
+            engine = this._engineName
+              ? Services.search.getEngineByName(this._engineName)
+              : await PlacesSearchAutocompleteProvider.currentEngine(
+                  this._inPrivateWindow
+                );
+            if (!this.pending || !engine) {
               return;
             }
           }
           let alias =
             (this._searchEngineAliasMatch &&
               this._searchEngineAliasMatch.alias) ||
             "";
           searchSuggestionsCompletePromise = this._matchSearchSuggestions(
@@ -1416,37 +1431,37 @@ Search.prototype = {
 
     return false;
   },
 
   async _matchFirstHeuristicResult(conn) {
     // We always try to make the first result a special "heuristic" result.  The
     // heuristics below determine what type of result it will be, if any.
 
-    let hasSearchTerms = !!this._searchTokens.length;
-
-    if (hasSearchTerms) {
+    if (this._heuristicToken) {
       // It may be a keyword registered by an extension.
-      let matched = await this._matchExtensionHeuristicResult();
+      let matched = await this._matchExtensionHeuristicResult(
+        this._heuristicToken
+      );
       if (matched) {
         return true;
       }
     }
 
-    if (this._enableActions && hasSearchTerms) {
+    if (this.pending && this._enableActions && this._heuristicToken) {
       // It may be a search engine with an alias - which works like a keyword.
-      let matched = await this._matchSearchEngineAlias();
+      let matched = await this._matchSearchEngineAlias(this._heuristicToken);
       if (matched) {
         return true;
       }
     }
 
-    if (this.pending && hasSearchTerms) {
+    if (this.pending && this._heuristicToken) {
       // It may be a Places keyword.
-      let matched = await this._matchPlacesKeyword();
+      let matched = await this._matchPlacesKeyword(this._heuristicToken);
       if (matched) {
         return true;
       }
     }
 
     let shouldAutofill = this._shouldAutofill;
 
     if (this.pending && shouldAutofill) {
@@ -1482,17 +1497,17 @@ Search.prototype = {
 
     if (this.pending && shouldAutofill) {
       let matched = await this._matchSearchEngineTokenAliasForAutofill();
       if (matched) {
         return true;
       }
     }
 
-    if (this.pending && hasSearchTerms && this._enableActions) {
+    if (this.pending && this._searchTokens.length && this._enableActions) {
       // If we don't have a result that matches what we know about, then
       // we use a fallback for things we don't know about.
 
       // We may not have auto-filled, but this may still look like a URL.
       // However, even if the input is a valid URL, we may not want to use
       // it as such. This can happen if the host would require whitelisting,
       // but isn't in the whitelist.
       let matched = await this._matchUnknownUrl();
@@ -1644,36 +1659,29 @@ Search.prototype = {
       await conn.executeCached(query, params, (row, cancel) => {
         gotResult = true;
         this._onResultRow(row, cancel);
       });
     }
     return gotResult;
   },
 
-  _matchExtensionHeuristicResult() {
+  _matchExtensionHeuristicResult(keyword) {
     if (
-      this._heuristicToken &&
-      ExtensionSearchHandler.isKeywordRegistered(this._heuristicToken) &&
-      substringAfter(this._originalSearchString, this._heuristicToken)
+      ExtensionSearchHandler.isKeywordRegistered(keyword) &&
+      substringAfter(this._originalSearchString, keyword)
     ) {
-      let description = ExtensionSearchHandler.getDescription(
-        this._heuristicToken
-      );
+      let description = ExtensionSearchHandler.getDescription(keyword);
       this._addExtensionMatch(this._originalSearchString, description);
       return true;
     }
     return false;
   },
 
-  async _matchPlacesKeyword() {
-    if (!this._heuristicToken) {
-      return false;
-    }
-    let keyword = this._heuristicToken;
+  async _matchPlacesKeyword(keyword) {
     let entry = await PlacesUtils.keywords.fetch(keyword);
     if (!entry) {
       return false;
     }
 
     let searchString = substringAfter(
       this._originalSearchString,
       keyword
@@ -1784,22 +1792,17 @@ Search.prototype = {
       comment: engine.name,
       icon: engine.iconURI ? engine.iconURI.spec : null,
       style: "priority-search",
       frecency: Infinity,
     });
     return true;
   },
 
-  async _matchSearchEngineAlias() {
-    if (!this._heuristicToken) {
-      return false;
-    }
-
-    let alias = this._heuristicToken;
+  async _matchSearchEngineAlias(alias) {
     let engine = await PlacesSearchAutocompleteProvider.engineForAlias(alias);
     if (!engine) {
       return false;
     }
 
     this._searchEngineAliasMatch = {
       engine,
       alias,
@@ -1809,19 +1812,21 @@ Search.prototype = {
     this._addSearchEngineMatch(this._searchEngineAliasMatch);
     if (!this._keywordSubstitute) {
       this._keywordSubstitute = engine.getResultDomain();
     }
     return true;
   },
 
   async _matchCurrentSearchEngine() {
-    let engine = await PlacesSearchAutocompleteProvider.currentEngine(
-      this._inPrivateWindow
-    );
+    let engine = this._engineName
+      ? Services.search.getEngineByName(this._engineName)
+      : await PlacesSearchAutocompleteProvider.currentEngine(
+          this._inPrivateWindow
+        );
     if (!engine || !this.pending) {
       return false;
     }
     // Strip a leading search restriction char, because we prepend it to text
     // when the search shortcut is used and it's not user typed. Don't strip
     // other restriction chars, so that it's possible to search for things
     // including one of those (e.g. "c#").
     let query = this._trimmedOriginalSearchString;