Bug 1494662 - Integrate the urlbar provider manager with the controller. r=dao
authorMark Banner <standard8@mozilla.com>
Tue, 02 Oct 2018 14:22:29 +0000
changeset 487566 5780816ec3a9eff36b3d7f1c221c5f58aa6aba47
parent 487565 fb7f5d71b31ad06c7207f0945cd8fdbee003e237
child 487567 5a66c886df15268b57449f869c0fa654b2c2eb0a
push id246
push userfmarier@mozilla.com
push dateSat, 13 Oct 2018 00:15:40 +0000
reviewersdao
bugs1494662
milestone64.0a1
Bug 1494662 - Integrate the urlbar provider manager with the controller. r=dao This picks up naming changes from the provider manager landing, and also makes starting a query be named consistently across the modules. Differential Revision: https://phabricator.services.mozilla.com/D7297
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarView.jsm
browser/components/urlbar/tests/browser/browser_UrlbarInput_unit.js
browser/components/urlbar/tests/unit/head.js
browser/components/urlbar/tests/unit/test_UrlbarController_integration.js
browser/components/urlbar/tests/unit/test_UrlbarController_unit.js
browser/components/urlbar/tests/unit/test_providersManager.js
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -2,42 +2,17 @@
  * 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/. */
 
 "use strict";
 
 var EXPORTED_SYMBOLS = ["QueryContext", "UrlbarController"];
 
 ChromeUtils.import("resource://gre/modules/Services.jsm");
-
-// XXX This is a fake manager to provide a basic integration test whilst we
-// are still constructing the manager.
-// eslint-disable-next-line require-jsdoc
-const ProvidersManager = {
-  queryStart(queryContext, controller) {
-    queryContext.results = [];
-    for (let i = 0; i < queryContext.maxResults; i++) {
-      const SWITCH_TO_TAB = Math.random() < .3;
-      let url = "http://www." + queryContext.searchString;
-      while (Math.random() < .9) {
-        url += queryContext.searchString;
-      }
-      let title = queryContext.searchString;
-      while (Math.random() < .5) {
-        title += queryContext.isPrivate ? " private" : " foo bar";
-      }
-      queryContext.results.push({
-        title,
-        type: SWITCH_TO_TAB ? "switchtotab" : "normal",
-        url,
-      });
-    }
-    controller.receiveResults(queryContext);
-  },
-};
+ChromeUtils.import("resource:///modules/UrlbarProvidersManager.jsm");
 
 /**
  * QueryContext defines a user's autocomplete input from within the Address Bar.
  * It supplements it with details of how the search results should be obtained
  * and what they consist of.
  */
 class QueryContext {
   /**
@@ -105,41 +80,42 @@ class UrlbarController {
    *
    * @param {object} [options]
    *   The initial options for UrlbarController.
    * @param {object} [options.manager]
    *   Optional fake providers manager to override the built-in providers manager.
    *   Intended for use in unit tests only.
    */
   constructor(options = {}) {
-    this.manager = options.manager || ProvidersManager;
+    this.manager = options.manager || UrlbarProvidersManager;
 
     this._listeners = new Set();
   }
 
   /**
    * Takes a query context and starts the query based on the user input.
    *
    * @param {QueryContext} queryContext The query details.
    */
-  handleQuery(queryContext) {
+  async startQuery(queryContext) {
     queryContext.autoFill = Services.prefs.getBoolPref("browser.urlbar.autoFill", true);
 
     this._notify("onQueryStarted", queryContext);
 
-    this.manager.queryStart(queryContext, this);
+    await this.manager.startQuery(queryContext, this);
   }
 
   /**
-   * Cancels an in-progress query.
+   * Cancels an in-progress query. Note, queries may continue running if they
+   * can't be canceled.
    *
    * @param {QueryContext} queryContext The query details.
    */
   cancelQuery(queryContext) {
-    this.manager.queryCancel(queryContext);
+    this.manager.cancelQuery(queryContext);
 
     this._notify("onQueryCancelled", queryContext);
   }
 
   /**
    * Receives results from a query.
    *
    * @param {QueryContext} queryContext The query details.
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -316,17 +316,17 @@ class UrlbarInput {
       event.preventDefault();
     }
   }
 
   _oninput(event) {
     this.valueIsTyped = true;
 
     // XXX Fill in lastKey, and add anything else we need.
-    this.controller.handleQuery(new QueryContext({
+    this.controller.startQuery(new QueryContext({
       searchString: event.target.value,
       lastKey: "",
       maxResults: UrlbarPrefs.get("maxRichResults"),
       isPrivate: this.isPrivate,
     }));
   }
 
   _onselect(event) {
@@ -437,9 +437,8 @@ class CopyCutController {
   isCommandEnabled(command) {
     return this.supportsCommand(command) &&
            (command != "cmd_cut" || !this.urlbar.readOnly) &&
            this.urlbar.selectionStart < this.urlbar.selectionEnd;
   }
 
   onEvent() {}
 }
-
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@@ -1,16 +1,21 @@
 /* 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/. */
 
 "use strict";
 
 var EXPORTED_SYMBOLS = ["UrlbarView"];
 
+ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
+XPCOMUtils.defineLazyModuleGetters(this, {
+  UrlbarUtils: "resource:///modules/UrlbarUtils.jsm",
+});
+
 /**
  * Receives and displays address bar autocomplete results.
  */
 class UrlbarView {
   /**
    * @param {UrlbarInput} urlbar
    *   The UrlbarInput instance belonging to this UrlbarView instance.
    */
@@ -73,16 +78,19 @@ class UrlbarView {
   }
 
   // UrlbarController listener methods.
   onQueryStarted(queryContext) {
     this._rows.textContent = "";
   }
 
   onQueryResults(queryContext) {
+    // XXX For now, clear the results for each set received. We should really
+    // be updating the existing list.
+    this._rows.textContent = "";
     for (let result of queryContext.results) {
       this._addRow(result);
     }
     this.open();
   }
 
   // Private methods below.
 
@@ -92,17 +100,17 @@ class UrlbarView {
 
   _createElement(name) {
     return this.document.createElementNS("http://www.w3.org/1999/xhtml", name);
   }
 
   _addRow(result) {
     let item = this._createElement("div");
     item.className = "urlbarView-row";
-    if (result.type == "switchtotab") {
+    if (result.type == UrlbarUtils.MATCH_TYPE.TAB_SWITCH) {
       item.setAttribute("action", "switch-to-tab");
     }
 
     let content = this._createElement("span");
     content.className = "urlbarView-row-inner";
     item.appendChild(content);
 
     let actionIcon = this._createElement("span");
@@ -110,22 +118,22 @@ class UrlbarView {
     content.appendChild(actionIcon);
 
     let favicon = this._createElement("span");
     favicon.className = "urlbarView-favicon";
     content.appendChild(favicon);
 
     let title = this._createElement("span");
     title.className = "urlbarView-title";
-    title.textContent = result.title;
+    title.textContent = result.title || result.url;
     content.appendChild(title);
 
     let secondary = this._createElement("span");
     secondary.className = "urlbarView-secondary";
-    if (result.type == "switchtotab") {
+    if (result.type == UrlbarUtils.MATCH_TYPE.TAB_SWITCH) {
       secondary.classList.add("urlbarView-action");
       secondary.textContent = "Switch to Tab";
     } else {
       secondary.classList.add("urlbarView-url");
       secondary.textContent = result.url;
     }
     content.appendChild(secondary);
 
--- a/browser/components/urlbar/tests/browser/browser_UrlbarInput_unit.js
+++ b/browser/components/urlbar/tests/browser/browser_UrlbarInput_unit.js
@@ -35,48 +35,48 @@ function assertContextMatches(context, e
 
   for (let [key, value] of Object.entries(expectedValues)) {
     Assert.equal(context[key], value,
       `Should have the expected value for ${key} in the QueryContext`);
   }
 }
 
 /**
- * Checks the result of a handleQuery call on the controller.
+ * Checks the result of a startQuery call on the controller.
  *
  * @param {object} stub The sinon stub that should have been called with the
  *                      QueryContext.
  * @param {object} expectedQueryContextProps
  *                   An object consisting of name/value pairs to check against the
  *                   QueryContext properties.
  */
-function checkHandleQueryCall(stub, expectedQueryContextProps) {
+function checkStartQueryCall(stub, expectedQueryContextProps) {
   Assert.equal(stub.callCount, 1,
-    "Should have called handleQuery on the controller");
+    "Should have called startQuery on the controller");
 
   let args = stub.args[0];
   Assert.equal(args.length, 1,
-    "Should have called handleQuery with one argument");
+    "Should have called startQuery with one argument");
 
   let queryContext = args[0];
   Assert.ok(queryContext instanceof QueryContext,
     "Should have been passed a QueryContext");
 
   for (let [name, value] of Object.entries(expectedQueryContextProps)) {
     Assert.deepEqual(queryContext[name],
      value, `Should have the correct value for queryContext.${name}`);
   }
 }
 
 add_task(async function setup() {
   sandbox = sinon.sandbox.create();
 
   fakeController = new UrlbarController();
 
-  sandbox.stub(fakeController, "handleQuery");
+  sandbox.stub(fakeController, "startQuery");
   sandbox.stub(PrivateBrowsingUtils, "isWindowPrivate").returns(false);
 
   // Open a new window, so we don't affect other tests by adding extra
   // UrbarInput wrappers around the urlbar.
   let gTestRoot = getRootDirectory(gTestPath);
 
   let win = window.openDialog(gTestRoot + "empty.xul",
                     "", "chrome");
@@ -107,17 +107,17 @@ add_task(async function setup() {
 add_task(function test_input_starts_query() {
   input.handleEvent({
     target: {
       value: "search",
     },
     type: "input",
   });
 
-  checkHandleQueryCall(fakeController.handleQuery, {
+  checkStartQueryCall(fakeController.startQuery, {
     searchString: "search",
     isPrivate: false,
     maxResults: UrlbarPrefs.get("maxRichResults"),
   });
 
   sandbox.resetHistory();
 });
 
@@ -130,15 +130,15 @@ add_task(function test_input_with_privat
 
   privateInput.handleEvent({
     target: {
       value: "search",
     },
     type: "input",
   });
 
-  checkHandleQueryCall(fakeController.handleQuery, {
+  checkStartQueryCall(fakeController.startQuery, {
     searchString: "search",
     isPrivate: true,
   });
 
   sandbox.resetHistory();
 });
--- a/browser/components/urlbar/tests/unit/head.js
+++ b/browser/components/urlbar/tests/unit/head.js
@@ -68,8 +68,52 @@ function promiseControllerNotification(c
           };
         }
         return () => false;
       },
     });
     controller.addQueryListener(proxifiedObserver);
   });
 }
+
+
+/**
+ * Helper function to clear the existing providers and register a basic provider
+ * that returns only the results given.
+ *
+ * @param {array} results The results for the provider to return.
+ * @param {function} [cancelCallback] Optional, called when the query provider
+ *                                    receives a cancel instruction.
+ */
+function registerBasicTestProvider(results, cancelCallback) {
+  // First unregister all the existing providers.
+  for (let providers of UrlbarProvidersManager.providers.values()) {
+    for (let provider of providers.values()) {
+      // While here check all providers have name and type.
+      Assert.ok(Object.values(UrlbarUtils.PROVIDER_TYPE).includes(provider.type),
+        `The provider "${provider.name}" should have a valid type`);
+      Assert.ok(provider.name, "All providers should have a name");
+      UrlbarProvidersManager.unregisterProvider(provider);
+    }
+  }
+  UrlbarProvidersManager.registerProvider({
+    get name() {
+      return "TestProvider";
+    },
+    get type() {
+      return UrlbarUtils.PROVIDER_TYPE.PROFILE;
+    },
+    async startQuery(context, add) {
+      Assert.ok(context, "context is passed-in");
+      Assert.equal(typeof add, "function", "add is a callback");
+      this._context = context;
+      for (const result of results) {
+        add(this, result);
+      }
+    },
+    cancelQuery(context) {
+      Assert.equal(this._context, context, "context is the same");
+      if (cancelCallback) {
+        cancelCallback();
+      }
+    },
+  });
+}
--- a/browser/components/urlbar/tests/unit/test_UrlbarController_integration.js
+++ b/browser/components/urlbar/tests/unit/test_UrlbarController_integration.js
@@ -2,17 +2,20 @@
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 /**
  * These tests test the UrlbarController in association with the model.
  */
 
 "use strict";
 
+ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm");
+
 const TEST_URL = "http://example.com";
+const match = new UrlbarMatch(UrlbarUtils.MATCH_TYPE.TAB_SWITCH, { url: TEST_URL });
 let controller;
 
 /**
  * Asserts that the query context has the expected values.
  *
  * @param {QueryContext} context
  * @param {object} expectedValues The expected values for the QueryContext.
  */
@@ -22,34 +25,53 @@ function assertContextMatches(context, e
 
   for (let [key, value] of Object.entries(expectedValues)) {
     Assert.equal(context[key], value,
       `Should have the expected value for ${key} in the QueryContext`);
   }
 }
 
 add_task(async function setup() {
-  await PlacesTestUtils.addVisits(TEST_URL);
-
   controller = new UrlbarController();
 });
 
 add_task(async function test_basic_search() {
   const context = createContext(TEST_URL);
 
+  registerBasicTestProvider([match]);
+
   let startedPromise = promiseControllerNotification(controller, "onQueryStarted");
   let resultsPromise = promiseControllerNotification(controller, "onQueryResults");
 
-  controller.handleQuery(context);
+  controller.startQuery(context);
 
   let params = await startedPromise;
 
   Assert.equal(params[0], context);
 
   params = await resultsPromise;
 
-  Assert.equal(params[0].results.length, Services.prefs.getIntPref("browser.urlbar.maxRichResults"),
-    "Should have given the expected amount of results");
+  Assert.deepEqual(params[0].results, [match],
+    "Should have the expected match");
+});
+
+add_task(async function test_cancel_search() {
+  const context = createContext(TEST_URL);
+
+  let providerCanceledPromise = PromiseUtils.defer();
+  registerBasicTestProvider([match], providerCanceledPromise.resolve);
+
+  let startedPromise = promiseControllerNotification(controller, "onQueryStarted");
+  let cancelPromise = promiseControllerNotification(controller, "onQueryResults");
 
-  for (let result of params[0].results) {
-    Assert.ok(result.url.includes(TEST_URL));
-  }
+  await controller.startQuery(context);
+
+  let params = await startedPromise;
+
+  controller.cancelQuery(context);
+
+  Assert.equal(params[0], context);
+
+  info("Should tell the provider the query is canceled");
+  await providerCanceledPromise;
+
+  params = await cancelPromise;
 });
--- a/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js
+++ b/browser/components/urlbar/tests/unit/test_UrlbarController_unit.js
@@ -29,18 +29,18 @@ function assertContextMatches(context, e
       `Should have the expected value for ${key} in the QueryContext`);
   }
 }
 
 add_task(function setup() {
   sandbox = sinon.sandbox.create();
 
   fPM = {
-    queryStart: sandbox.stub(),
-    queryCancel: sandbox.stub(),
+    startQuery: sandbox.stub(),
+    cancelQuery: sandbox.stub(),
   };
 
   generalListener = {
     onQueryStarted: sandbox.stub(),
     onQueryResults: sandbox.stub(),
     onQueryCancelled: sandbox.stub(),
   };
 
@@ -106,67 +106,67 @@ add_task(function test__notify() {
   // This should succeed without errors.
   controller._notify("onNewFake");
 
   sandbox.resetHistory();
 });
 
 add_task(function test_handle_query_starts_search() {
   const context = createContext();
-  controller.handleQuery(context);
+  controller.startQuery(context);
 
-  Assert.equal(fPM.queryStart.callCount, 1,
-    "Should have called queryStart once");
-  Assert.equal(fPM.queryStart.args[0].length, 2,
-    "Should have called queryStart with two arguments");
+  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.queryStart.args[0][0], {
+  assertContextMatches(fPM.startQuery.args[0][0], {
     autoFill: true,
   });
-  Assert.equal(fPM.queryStart.args[0][1], controller,
+  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);
 
-  controller.handleQuery(createContext());
+  controller.startQuery(createContext());
 
-  Assert.equal(fPM.queryStart.callCount, 1,
-    "Should have called queryStart once");
-  Assert.equal(fPM.queryStart.args[0].length, 2,
-    "Should have called queryStart with two arguments");
+  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.queryStart.args[0][0], {
+  assertContextMatches(fPM.startQuery.args[0][0], {
     autoFill: false,
   });
-  Assert.equal(fPM.queryStart.args[0][1], controller,
+  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");
 });
 
 add_task(function test_cancel_query() {
   const context = createContext();
   controller.cancelQuery(context);
 
-  Assert.equal(fPM.queryCancel.callCount, 1,
-    "Should have called queryCancel once");
-  Assert.equal(fPM.queryCancel.args[0].length, 1,
-    "Should have called queryCancel with one argument");
+  Assert.equal(fPM.cancelQuery.callCount, 1,
+    "Should have called cancelQuery once");
+  Assert.equal(fPM.cancelQuery.args[0].length, 1,
+    "Should have called cancelQuery with one argument");
 
   Assert.equal(generalListener.onQueryCancelled.callCount, 1,
     "Should have called onQueryCancelled for the listener");
   Assert.deepEqual(generalListener.onQueryCancelled.args[0], [context],
     "Should have called onQueryCancelled with the context");
 
   sandbox.resetHistory();
 });
--- a/browser/components/urlbar/tests/unit/test_providersManager.js
+++ b/browser/components/urlbar/tests/unit/test_providersManager.js
@@ -1,42 +1,16 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 add_task(async function test_providers() {
-  // First unregister all the existing providers.
-  for (let providers of UrlbarProvidersManager.providers.values()) {
-    for (let provider of providers.values()) {
-      // While here check all providers have name and type.
-      Assert.ok(Object.values(UrlbarUtils.PROVIDER_TYPE).includes(provider.type),
-        `The provider "${provider.name}" should have a valid type`);
-      Assert.ok(provider.name, "All providers should have a name");
-      UrlbarProvidersManager.unregisterProvider(provider);
-    }
-  }
   let match = new UrlbarMatch(UrlbarUtils.MATCH_TYPE.TAB_SWITCH, { url: "http://mozilla.org/foo/" });
-  UrlbarProvidersManager.registerProvider({
-    get name() {
-      return "TestProvider";
-    },
-    get type() {
-      return UrlbarUtils.PROVIDER_TYPE.PROFILE;
-    },
-    async startQuery(context, add) {
-      Assert.ok(context, "context is passed-in");
-      Assert.equal(typeof add, "function", "add is a callback");
-      this._context = context;
-      add(this, match);
-    },
-    cancelQuery(context) {
-      Assert.equal(this._context, context, "context is the same");
-    },
-  });
+  registerBasicTestProvider([match]);
 
   let context = createContext();
   let controller = new UrlbarController();
   let resultsPromise = promiseControllerNotification(controller, "onQueryResults");
 
   await UrlbarProvidersManager.startQuery(context, controller);
   // Sanity check that this doesn't throw. It should be a no-op since we await
   // for startQuery.