Bug 1520368 - Clarify the queryContext.autofill property and add an enableAutofill property. r=mak
authorDrew Willcoxon <adw@mozilla.com>
Wed, 16 Jan 2019 17:02:26 +0000
changeset 511216 7f25f8bee25bcd1f5da9ef7cc8c4026560f9d356
parent 511215 fb629855a3bbed86fb68cb7d2e42f9486f7f81be
child 511217 7db49cc01bb7e20dc4bdb6d1a1e6a5663aa9ba98
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1520368, 1520342
milestone66.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 1520368 - Clarify the queryContext.autofill property and add an enableAutofill property. r=mak This patch is based on the patch in bug 1520342. I made the UnifiedComplete provider manually check `context.enableAutofill` before setting `context.autofill`. If we end up with other providers setting `autofill`, they'd have to be careful to check `enableAutofill` too. Maybe it would be better to have a `context.autofill` getter that always returns false when `enableAutofill` is false, or a setter that forces it to be false in that case? Anyway, I opted for a simple approach in this patch. The patch also rearranges properties so that they're listed in alphabetical order. Not really necessary, but I think it's easier to pick out properties that way, and it's a logical order for adding more properties. Differential Revision: https://phabricator.services.mozilla.com/D16639
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
browser/components/urlbar/UrlbarUtils.jsm
browser/components/urlbar/tests/unit/head.js
browser/components/urlbar/tests/unit/test_QueryContext.js
browser/components/urlbar/tests/unit/test_UrlbarController_unit.js
browser/docs/AddressBar.rst
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -6,17 +6,16 @@
 
 var EXPORTED_SYMBOLS = ["QueryContext", "UrlbarController"];
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   // BrowserUsageTelemetry: "resource:///modules/BrowserUsageTelemetry.jsm",
   PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
-  UrlbarPrefs: "resource:///modules/UrlbarPrefs.jsm",
   UrlbarProvidersManager: "resource:///modules/UrlbarProvidersManager.jsm",
   UrlbarUtils: "resource:///modules/UrlbarUtils.jsm",
 });
 
 const TELEMETRY_1ST_RESULT = "PLACES_AUTOCOMPLETE_1ST_RESULT_TIME_MS";
 const TELEMETRY_6_FIRST_RESULTS = "PLACES_AUTOCOMPLETE_6_FIRST_RESULTS_TIME_MS";
 
 /**
@@ -87,18 +86,16 @@ class UrlbarController {
    */
   async startQuery(queryContext) {
     // Cancel any running query.
     if (this._lastQueryContext) {
       this.cancelQuery(this._lastQueryContext);
     }
     this._lastQueryContext = queryContext;
 
-    queryContext.autofill = UrlbarPrefs.get("autoFill");
-
     queryContext.lastTelemetryResultCount = 0;
     TelemetryStopwatch.start(TELEMETRY_1ST_RESULT, queryContext);
     TelemetryStopwatch.start(TELEMETRY_6_FIRST_RESULTS, queryContext);
 
     this._notify("onQueryStarted", queryContext);
     await this.manager.startQuery(queryContext, this);
     this._notify("onQueryFinished", queryContext);
   }
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -353,22 +353,23 @@ class UrlbarInput {
    * @param {number} [options.lastKey]
    *   The last key the user entered (as a key code).
    */
   startQuery({
     searchString = "",
     lastKey = null,
   } = {}) {
     this.controller.startQuery(new QueryContext({
-      searchString,
+      enableAutofill: UrlbarPrefs.get("autoFill"),
+      isPrivate: this.isPrivate,
       lastKey,
       maxResults: UrlbarPrefs.get("maxRichResults"),
-      isPrivate: this.isPrivate,
+      muxer: "UnifiedComplete",
       providers: ["UnifiedComplete"],
-      muxer: "UnifiedComplete",
+      searchString,
     }));
   }
 
   typeRestrictToken(char) {
     this.inputField.value = char + " ";
 
     let event = this.document.createEvent("UIEvents");
     event.initUIEvent("input", true, false, this.window, 0);
--- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
@@ -190,17 +190,19 @@ function convertResultToMatches(context,
     });
     // Should not happen, but better safe than sorry.
     if (!match) {
       continue;
     }
     matches.push(match);
     // Manage autofill and preselected properties for the first match.
     if (i == 0) {
-      if (style.includes("autofill") && result.defaultIndex == 0) {
+      if (style.includes("autofill") &&
+          result.defaultIndex == 0 &&
+          context.enableAutofill) {
         context.autofill = true;
       }
       if (style.includes("heuristic")) {
         context.preselected = true;
       }
     }
   }
   return {matches, done};
--- a/browser/components/urlbar/UrlbarUtils.jsm
+++ b/browser/components/urlbar/UrlbarUtils.jsm
@@ -247,43 +247,41 @@ class QueryContext {
    *   in the case of the user opening the popup via the mouse.
    * @param {number} options.lastKey
    *   The last key the user entered (as a key code). Could be null if the search
    *   was started via the mouse.
    * @param {boolean} options.isPrivate
    *   Set to true if this query was started from a private browsing window.
    * @param {number} options.maxResults
    *   The maximum number of results that will be displayed for this query.
-   * @param {boolean} [options.autofill]
-   *   Whether or not to include autofill results. Optional, as this is normally
-   *   set by the UrlbarController.
+   * @param {boolean} options.enableAutofill
+   *   Whether or not to include autofill results.
    */
   constructor(options = {}) {
     this._checkRequiredOptions(options, [
-      "searchString",
+      "enableAutofill",
+      "isPrivate",
       "lastKey",
       "maxResults",
-      "isPrivate",
+      "searchString",
     ]);
 
     if (isNaN(parseInt(options.maxResults))) {
       throw new Error(`Invalid maxResults property provided to QueryContext`);
     }
 
     if (options.providers &&
         (!Array.isArray(options.providers) || !options.providers.length)) {
       throw new Error(`Invalid providers list`);
     }
 
     if (options.sources &&
         (!Array.isArray(options.sources) || !options.sources.length)) {
       throw new Error(`Invalid sources list`);
     }
-
-    this.autofill = !!options.autofill;
   }
 
   /**
    * Checks the required options, saving them as it goes.
    *
    * @param {object} options The options object to check.
    * @param {array} optionNames The names of the options to check for.
    * @throws {Error} Throws if there is a missing option.
--- a/browser/components/urlbar/tests/unit/head.js
+++ b/browser/components/urlbar/tests/unit/head.js
@@ -37,20 +37,21 @@ Services.scriptloader.loadSubScript("res
 
 /**
  * @param {string} searchString The search string to insert into the context.
  * @param {object} properties Overrides for the default values.
  * @returns {QueryContext} Creates a dummy query context with pre-filled required options.
  */
 function createContext(searchString = "foo", properties = {}) {
   let context = new QueryContext({
-    searchString,
+    enableAutofill: UrlbarPrefs.get("autoFill"),
+    isPrivate: true,
     lastKey: searchString ? searchString[searchString.length - 1] : "",
     maxResults: UrlbarPrefs.get("maxRichResults"),
-    isPrivate: true,
+    searchString,
   });
   return Object.assign(context, properties);
 }
 
 /**
  * Waits for the given notification from the supplied controller.
  *
  * @param {UrlbarController} controller The controller to wait for a response from.
--- a/browser/components/urlbar/tests/unit/test_QueryContext.js
+++ b/browser/components/urlbar/tests/unit/test_QueryContext.js
@@ -1,50 +1,61 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 add_task(function test_constructor() {
   Assert.throws(() => new QueryContext(),
-    /Missing or empty searchString provided to QueryContext/,
+    /Missing or empty enableAutofill provided to QueryContext/,
     "Should throw with no arguments");
 
   Assert.throws(() => new QueryContext({
-    searchString: "foo",
+    enableAutofill: true,
+    isPrivate: false,
     maxResults: 1,
-    isPrivate: false,
+    searchString: "foo",
   }), /Missing or empty lastKey provided to QueryContext/,
     "Should throw with a missing lastKey parameter");
 
   Assert.throws(() => new QueryContext({
-    searchString: "foo",
+    enableAutofill: true,
+    isPrivate: false,
     lastKey: "b",
-    isPrivate: false,
+    searchString: "foo",
   }), /Missing or empty maxResults provided to QueryContext/,
     "Should throw with a missing maxResults parameter");
 
   Assert.throws(() => new QueryContext({
-    searchString: "foo",
+    enableAutofill: true,
     lastKey: "b",
     maxResults: 1,
+    searchString: "foo",
   }), /Missing or empty isPrivate provided to QueryContext/,
     "Should throw with a missing isPrivate parameter");
 
-  let qc = new QueryContext({
-    searchString: "foo",
+  Assert.throws(() => new QueryContext({
+    isPrivate: false,
     lastKey: "b",
     maxResults: 1,
+    searchString: "foo",
+  }), /Missing or empty enableAutofill provided to QueryContext/,
+    "Should throw with a missing enableAutofill parameter");
+
+  let qc = new QueryContext({
+    enableAutofill: false,
     isPrivate: true,
-    autofill: false,
+    lastKey: "b",
+    maxResults: 1,
+    searchString: "foo",
   });
 
-  Assert.equal(qc.searchString, "foo",
-    "Should have saved the correct value for searchString");
+  Assert.strictEqual(qc.enableAutofill, false,
+    "Should have saved the correct value for enableAutofill");
+  Assert.strictEqual(qc.isPrivate, true,
+    "Should have saved the correct value for isPrivate");
   Assert.equal(qc.lastKey, "b",
     "Should have saved the correct value for lastKey");
   Assert.equal(qc.maxResults, 1,
     "Should have saved the correct value for maxResults");
-  Assert.strictEqual(qc.isPrivate, true,
-    "Should have saved the correct value for isPrivate");
-  Assert.strictEqual(qc.autofill, false,
-    "Should have saved the correct value for autofill");
+  Assert.equal(qc.searchString, "foo",
+    "Should have saved the correct value for searchString");
 });
--- a/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js
+++ b/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js
@@ -132,43 +132,41 @@ add_task(function test_handle_query_star
   const context = createContext();
   controller.startQuery(context);
 
   Assert.equal(fPM.startQuery.callCount, 1,
     "Should have called startQuery once");
   Assert.equal(fPM.startQuery.args[0].length, 2,
     "Should have called startQuery with two arguments");
 
-  assertContextMatches(fPM.startQuery.args[0][0], {
-    autofill: true,
-  });
+  assertContextMatches(fPM.startQuery.args[0][0], {});
   Assert.equal(fPM.startQuery.args[0][1], controller,
     "Should have passed the controller as the second argument");
 
-
   Assert.equal(generalListener.onQueryStarted.callCount, 1,
     "Should have called onQueryStarted for the listener");
   Assert.deepEqual(generalListener.onQueryStarted.args[0], [context],
     "Should have called onQueryStarted with the context");
 
   sandbox.resetHistory();
 });
 
-add_task(function test_handle_query_starts_search_sets_autofill() {
-  Services.prefs.setBoolPref("browser.urlbar.autoFill", false);
+add_task(function test_handle_query_starts_search_sets_enableAutofill() {
+  let originalValue = Services.prefs.getBoolPref("browser.urlbar.autoFill");
+  Services.prefs.setBoolPref("browser.urlbar.autoFill", !originalValue);
 
   controller.startQuery(createContext());
 
   Assert.equal(fPM.startQuery.callCount, 1,
     "Should have called startQuery once");
   Assert.equal(fPM.startQuery.args[0].length, 2,
     "Should have called startQuery with two arguments");
 
   assertContextMatches(fPM.startQuery.args[0][0], {
-    autofill: false,
+    enableAutofill: !originalValue,
   });
   Assert.equal(fPM.startQuery.args[0][1], controller,
     "Should have passed the controller as the second argument");
 
   sandbox.resetHistory();
 
   Services.prefs.clearUserPref("browser.urlbar.autoFill");
 });
--- a/browser/docs/AddressBar.rst
+++ b/browser/docs/AddressBar.rst
@@ -50,40 +50,41 @@ The QueryContext
 
 The *QueryContext* object describes a single instance of a search.
 It is augmented as it progresses through the system, with various information:
 
 .. highlight:: JavaScript
 .. code::
 
   QueryContext {
-    searchString; // {string} The user typed string.
+    enableAutofill; // {boolean} Whether or not to include autofill results.
+    isPrivate; // {boolean} Whether the search started in a private context.
     lastKey; // {string} The last key pressed by the user. This can affect the
              // behavior, for example by not autofilling again when the user
              // hit backspace.
     maxResults; // {integer} The maximum number of results requested. It is
                 // possible to request more results than the shown ones, and
                 // do additional filtering at the View level.
-    isPrivate; // {boolean} Whether the search started in a private context.
+    searchString; // {string} The user typed string.
     userContextId; // {integer} The user context ID (containers feature).
 
     // Optional properties.
     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} If provided is the list of sources, as defined by
              // MATCH_SOURCE.*, that can be returned by the model.
 
     // Properties added by the Model.
+    autofill; // {boolean} whether the first match is an autofill match.
+    preselected; // {boolean} whether the first match should be preselected.
+    results; // {array} list of UrlbarMatch objects.
     tokens; // {array} tokens extracted from the searchString, each token is an
             // object in the form {type, value}.
-    results; // {array} list of UrlbarMatch objects.
-    preselected; // {boolean} whether the first match should be preselected.
-    autofill; // {boolean} whether the first match is an autofill match.
   }
 
 
 The Model
 =========
 
 The *Model* is the component responsible for retrieving search results based on
 the user's input, and sorting them accordingly to their importance.