Bug 1636583 - Make the urlbar always go through pickResult. r=adw
☠☠ backed out by 5d62e7f9ed7b ☠ ☠
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 21 May 2020 15:00:08 +0000
changeset 531608 a2e636ff03c2b4ee74efc96bc040a4b0a725c963
parent 531607 660b7de8921523970782ef9ae793293d960bde30
child 531609 c7970fe81668ac038b60375b4fdcd0de85eab454
push id37441
push userapavel@mozilla.com
push dateFri, 22 May 2020 21:38:53 +0000
treeherdermozilla-central@d6abd35b54ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1636583
milestone78.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 1636583 - Make the urlbar always go through pickResult. r=adw This changes the urlbar to always generate a result and then confirm it through pickResult. This way we obtain a consistent behavior independently from whether the view has a result or an action like Paste&Go happened. Before this we used to go through getShortcutOrURIAndPostData, that implements only a part of the urlbar logic, often causing different behavior depending on the view state, and thus requiring constant maintenance to sync it up. In a follow-up bug we will evaluate the complete removal of getShortcutOrURIAndPostData in favor of direct calls to UrlbarUtils.getHeuristicResultFor(). This also moves up a bit closer to always pass a final url to the docshell, and stop trying to do complex URIFixup calls in it. For now we still rely on its fix-ups for browser.fixup.dns_first_for_single_words, where we pass a url, and if it's invalid it will instead search. See UrlbarUtils.RESULT_TYPE.URL handling in pickResult(). Differential Revision: https://phabricator.services.mozilla.com/D75911
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarProvidersManager.jsm
browser/components/urlbar/UrlbarUtils.jsm
browser/components/urlbar/tests/browser/browser.ini
browser/components/urlbar/tests/browser/browser_handleCommand_fallback.js
browser/components/urlbar/tests/browser/browser_loadRace.js
docshell/test/browser/browser_search_notification.js
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -483,48 +483,45 @@ class UrlbarInput {
     } catch (ex) {}
     if (isValidUrl) {
       this._loadURL(url, where, openParams);
       return;
     }
 
     // This is not a URL and there's no selected element, because likely the
     // view is closed, or paste&go was used.
+    // We must act consistently here, having or not an open view should not
+    // make a difference if the search string is the same.
 
-    // If we have a result for the current value, just use it.
+    // If we have a result for the current value, we can just use it.
     if (this._resultForCurrentValue) {
       this.pickResult(this._resultForCurrentValue, event);
       return;
     }
 
-    // Otherwise, fall back to an imperfect clone of what the heuristic result
-    // would do.
-    // TODO: Run a search to get just the heuristic result here, and then use
-    // pickResult.
-
+    // Otherwise, we must fetch the heuristic result for the current value.
     // 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 => {
+    UrlbarUtils.getHeuristicResultFor(url).then(newResult => {
       // 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);
+        this.pickResult(newResult, event, null, browser);
       }
     });
+    // Don't add further handling here, the getHeuristicResultFor call above is
+    // our last resort.
   }
 
   handleRevert() {
     this.window.gBrowser.userTypedValue = null;
     this.setURI(null, true);
     if (this.value && this.focused) {
       this.select();
     }
@@ -545,18 +542,24 @@ class UrlbarInput {
   }
 
   /**
    * Called when a result is picked.
    *
    * @param {UrlbarResult} result The result that was picked.
    * @param {Event} event The event that picked the result.
    * @param {DOMElement} element the picked view element, if available.
+   * @param {object} browser The browser to use for the load.
    */
-  pickResult(result, event, element = null) {
+  pickResult(
+    result,
+    event,
+    element = null,
+    browser = this.window.gBrowser.selectedBrowser
+  ) {
     let originalUntrimmedValue = this.untrimmedValue;
     let isCanonized = this.setValueFromResult(result, event);
     let where = this._whereToOpen(event);
     let openParams = {
       allowInheritPrincipal: false,
     };
 
     let selIndex = result.rowIndex;
@@ -567,17 +570,17 @@ class UrlbarInput {
     this.controller.recordSelectedResult(event, result);
 
     if (isCanonized) {
       this.controller.engagementEvent.record(event, {
         numChars: this._lastSearchString.length,
         selIndex,
         selType: "canonized",
       });
-      this._loadURL(this.value, where, openParams);
+      this._loadURL(this.value, where, openParams, browser);
       return;
     }
 
     let { url, postData } = UrlbarUtils.getUrlFromResult(result);
     openParams.postData = postData;
 
     switch (result.type) {
       case UrlbarUtils.RESULT_TYPE.URL: {
@@ -799,20 +802,26 @@ class UrlbarInput {
     }
 
     this.controller.engagementEvent.record(event, {
       numChars: this._lastSearchString.length,
       selIndex,
       selType: this.controller.engagementEvent.typeFromElement(element),
     });
 
-    this._loadURL(url, where, openParams, {
-      source: result.source,
-      type: result.type,
-    });
+    this._loadURL(
+      url,
+      where,
+      openParams,
+      {
+        source: result.source,
+        type: result.type,
+      },
+      browser
+    );
   }
 
   /**
    * Called by the view when moving through results with the keyboard, and when
    * picking a result.
    *
    * @param {UrlbarResult} [result]
    *   The result that was selected or picked, null if no result was selected.
--- a/browser/components/urlbar/UrlbarProvidersManager.jsm
+++ b/browser/components/urlbar/UrlbarProvidersManager.jsm
@@ -156,19 +156,19 @@ class ProvidersManager {
     let muxerName = typeof muxer == "string" ? muxer : muxer.name;
     logger.info(`Unregistering muxer ${muxerName}`);
     this.muxers.delete(muxerName);
   }
 
   /**
    * Starts querying.
    * @param {object} queryContext The query context object
-   * @param {object} controller a UrlbarController instance
+   * @param {object} [controller] a UrlbarController instance
    */
-  async startQuery(queryContext, controller) {
+  async startQuery(queryContext, controller = null) {
     logger.info(`Query start ${queryContext.searchString}`);
 
     // Define the muxer to use.
     let muxerName = queryContext.muxer || DEFAULT_MUXER;
     logger.info(`Using muxer ${muxerName}`);
     let muxer = this.muxers.get(muxerName);
     if (!muxer) {
       throw new Error(`Muxer with name ${muxerName} not found`);
@@ -482,17 +482,19 @@ class Query {
     for (let i = 0; i < this.context.results.length; i++) {
       resultCount -= UrlbarUtils.getSpanForResult(this.context.results[i]);
       if (resultCount < 0) {
         this.context.results.splice(i, this.context.results.length - i);
         break;
       }
     }
 
-    this.controller.receiveResults(this.context);
+    if (this.controller) {
+      this.controller.receiveResults(this.context);
+    }
   }
 }
 
 /**
  * Updates in place the sources for a given UrlbarQueryContext.
  * @param {UrlbarQueryContext} context The query context to examine
  */
 function updateSourcesIfEmpty(context) {
--- a/browser/components/urlbar/UrlbarUtils.jsm
+++ b/browser/components/urlbar/UrlbarUtils.jsm
@@ -17,21 +17,23 @@ var EXPORTED_SYMBOLS = [
   "SkippableTimer",
 ];
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
 XPCOMUtils.defineLazyModuleGetters(this, {
   BrowserUtils: "resource://gre/modules/BrowserUtils.jsm",
+  BrowserWindowTracker: "resource:///modules/BrowserWindowTracker.jsm",
   PrivateBrowsingUtils: "resource://gre/modules/PrivateBrowsingUtils.jsm",
   PlacesUIUtils: "resource:///modules/PlacesUIUtils.jsm",
   PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
   Services: "resource://gre/modules/Services.jsm",
   UrlbarPrefs: "resource:///modules/UrlbarPrefs.jsm",
+  UrlbarProvidersManager: "resource:///modules/UrlbarProvidersManager.jsm",
 });
 
 var UrlbarUtils = {
   // Extensions are allowed to add suggestions if they have registered a keyword
   // with the omnibox API. This is the maximum number of suggestions an extension
   // is allowed to add for a given search string.
   // This value includes the heuristic result.
   MAXIMUM_ALLOWED_EXTENSION_MATCHES: 6,
@@ -564,16 +566,47 @@ var UrlbarUtils = {
    * URIFixup::KeywordURIFixup
    * @param {string} value
    * @returns {boolean} Whether the value looks like a single word host.
    */
   looksLikeSingleWordHost(value) {
     let str = value.trim();
     return this.REGEXP_SINGLE_WORD.test(str);
   },
+
+  /**
+   * Runs a search for the given string, and returns the heuristic result.
+   * @param {string} searchString The string to search for.
+   * @param {nsIDOMWindow} window The window requesting it.
+   * @returns {UrlbarResult} an heuristic result.
+   */
+  async getHeuristicResultFor(
+    searchString,
+    window = BrowserWindowTracker.getTopWindow()
+  ) {
+    if (!searchString) {
+      throw new Error("Must pass a non-null search string");
+    }
+    let context = new UrlbarQueryContext({
+      allowAutofill: false,
+      isPrivate: PrivateBrowsingUtils.isWindowPrivate(window),
+      maxResults: 1,
+      searchString,
+      userContextId: window.gBrowser.selectedBrowser.getAttribute(
+        "usercontextid"
+      ),
+      allowSearchSuggestions: false,
+      providers: ["UnifiedComplete"],
+    });
+    await UrlbarProvidersManager.startQuery(context);
+    if (!context.heuristicResult) {
+      throw new Error("There should always be an heuristic result");
+    }
+    return context.heuristicResult;
+  },
 };
 
 XPCOMUtils.defineLazyGetter(UrlbarUtils.ICON, "DEFAULT", () => {
   return PlacesUtils.favicons.defaultFavicon.spec;
 });
 
 XPCOMUtils.defineLazyGetter(UrlbarUtils, "strings", () => {
   return Services.strings.createBundle(
--- a/browser/components/urlbar/tests/browser/browser.ini
+++ b/browser/components/urlbar/tests/browser/browser.ini
@@ -57,16 +57,17 @@ support-files =
 [browser_dns_first_for_single_words.js]
 skip-if = verify && os == 'linux' # Bug 1581635
 [browser_downArrowKeySearch.js]
 [browser_dragdropURL.js]
 [browser_empty_search.js]
 [browser_enter.js]
 [browser_enterAfterMouseOver.js]
 [browser_focusedCmdK.js]
+[browser_handleCommand_fallback.js]
 [browser_hashChangeProxyState.js]
 [browser_heuristicNotAddedFirst.js]
 [browser_ime_composition.js]
 [browser_inputHistory.js]
 [browser_inputHistory_emptystring.js]
 [browser_keepStateAcrossTabSwitches.js]
 [browser_keywordBookmarklets.js]
 [browser_keyword_override.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/urlbar/tests/browser/browser_handleCommand_fallback.js
@@ -0,0 +1,88 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Tests that the fallback paths of handleCommand (no view and no previous
+ * result) work consistently against the normal case of picking the heuristic
+ * result.
+ */
+
+const TEST_STRINGS = [
+  "test",
+  "test/",
+  "test.com",
+  "test.invalid",
+  "moz",
+  "moz test",
+  "@moz test",
+  "keyword",
+  "keyword test",
+  "test/test/",
+  "test /test/",
+];
+
+add_task(async function() {
+  sandbox = sinon.createSandbox();
+  let engine = await Services.search.addEngineWithDetails("MozSearch", {
+    alias: "moz",
+    method: "GET",
+    template: "http://example.com/?q={searchTerms}",
+  });
+  let engine2 = await Services.search.addEngineWithDetails("MozSearch2", {
+    alias: "@moz",
+    method: "GET",
+    template: "http://example.com/?q={searchTerms}",
+  });
+  let bm = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: "http://example.com/?q=%s",
+    title: "test",
+  });
+  await PlacesUtils.keywords.insert({
+    keyword: "keyword",
+    url: "http://example.com/?q=%s",
+  });
+  registerCleanupFunction(async () => {
+    sandbox.restore();
+    await Services.search.removeEngine(engine);
+    await Services.search.removeEngine(engine2);
+    await PlacesUtils.bookmarks.remove(bm);
+  });
+
+  async function promiseLoadURL() {
+    return new Promise(resolve => {
+      sandbox.stub(gURLBar, "_loadURL").callsFake(function() {
+        sandbox.restore();
+        // The last arguments are optional and apply only to some cases, so we
+        // could not use deepEqual with them.
+        resolve(Array.from(arguments).slice(0, 3));
+      });
+    });
+  }
+
+  // Run the string through a normal search where the user types the string
+  // and confirms the heuristic result, store the arguments to _loadURL, then
+  // confirm the same string without a view and without an input event, and
+  // compare the arguments.
+  for (let value of TEST_STRINGS) {
+    let promise = promiseLoadURL();
+    await UrlbarTestUtils.promiseAutocompleteResultPopup({
+      window,
+      waitForFocus,
+      value,
+    });
+    EventUtils.synthesizeKey("KEY_Enter");
+    let args = await promise;
+    Assert.ok(args.length, "Sanity check");
+    // Close the panel and confirm again.
+    promise = promiseLoadURL();
+    await UrlbarTestUtils.promisePopupClose(window);
+    EventUtils.synthesizeKey("KEY_Enter");
+    Assert.deepEqual(await promise, args, "Check arguments are coherent");
+    // Set the value directly and Enter.
+    promise = promiseLoadURL();
+    gURLBar.value = value;
+    EventUtils.synthesizeKey("KEY_Enter");
+    Assert.deepEqual(await promise, args, "Check arguments are coherent");
+  }
+});
--- a/browser/components/urlbar/tests/browser/browser_loadRace.js
+++ b/browser/components/urlbar/tests/browser/browser_loadRace.js
@@ -17,60 +17,71 @@ add_task(async function setup() {
 
 async function checkShortcutLoading(modifierKeys) {
   let deferred = PromiseUtils.defer();
   let tab = await BrowserTestUtils.openNewForegroundTab({
     gBrowser,
     opening: "about:robots",
   });
 
-  // We stub getShortcutOrURIAndPostData so that we can guarentee it doesn't resolve
-  // until after we've loaded a new page.
+  // We stub getHeuristicResultFor to guarentee it doesn't resolve until after
+  // we've loaded a new page.
+  let original = UrlbarUtils.getHeuristicResultFor;
   sandbox
-    .stub(UrlbarUtils, "getShortcutOrURIAndPostData")
-    .callsFake(() => deferred.promise);
+    .stub(UrlbarUtils, "getHeuristicResultFor")
+    .callsFake(async searchString => {
+      await deferred.promise;
+      return original.call(this, searchString);
+    });
 
+  // This load will be blocked until the deferred is resolved.
+  // Use a string that will be interepreted as a local URL to avoid hitting the
+  // network.
   gURLBar.focus();
-  gURLBar.value = "search";
+  gURLBar.value = "example.com";
   gURLBar.userTypedValue = true;
   EventUtils.synthesizeKey("KEY_Enter", modifierKeys);
 
   Assert.ok(
-    UrlbarUtils.getShortcutOrURIAndPostData.calledOnce,
-    "should have called getShortcutOrURIAndPostData"
+    UrlbarUtils.getHeuristicResultFor.calledOnce,
+    "should have called getHeuristicResultFor"
   );
 
+  // Now load a different page.
   BrowserTestUtils.loadURI(tab.linkedBrowser, "about:license");
   await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
-
-  let openedTab;
-  function listener(event) {
-    openedTab = event.target;
-  }
-  window.addEventListener("TabOpen", listener);
+  Assert.equal(gBrowser.visibleTabs.length, 2, "Should have 2 tabs");
 
-  deferred.resolve({
-    url: "https://example.com/1/",
-    postData: {},
-    mayInheritPrincipal: true,
-  });
-
-  await deferred.promise;
-
+  // Now that the new page has loaded, unblock the previous urlbar load.
+  deferred.resolve();
   if (modifierKeys) {
-    Assert.ok(openedTab, "Should have attempted to open the shortcut page");
-    BrowserTestUtils.removeTab(openedTab);
-  } else {
+    let openedTab = await new Promise(resolve => {
+      window.addEventListener(
+        "TabOpen",
+        event => {
+          resolve(event.target);
+        },
+        { once: true }
+      );
+    });
+    await BrowserTestUtils.browserLoaded(openedTab.linkedBrowser);
     Assert.ok(
-      !openedTab,
-      "Should have not attempted to open the shortcut page"
+      openedTab.linkedBrowser.currentURI.spec.includes("example.com"),
+      "Should have attempted to open the shortcut page"
     );
+    BrowserTestUtils.removeTab(openedTab);
   }
 
-  window.removeEventListener("TabOpen", listener);
+  Assert.equal(
+    tab.linkedBrowser.currentURI.spec,
+    "about:license",
+    "Tab url should not have changed"
+  );
+  Assert.equal(gBrowser.visibleTabs.length, 2, "Should still have 2 tabs");
+
   BrowserTestUtils.removeTab(tab);
   sandbox.restore();
 }
 
 add_task(async function test_location_change_stops_load() {
   await checkShortcutLoading();
 });
 
--- a/docshell/test/browser/browser_search_notification.js
+++ b/docshell/test/browser/browser_search_notification.js
@@ -1,12 +1,19 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 add_task(async function() {
+  // Our search would be handled by the urlbar normally and not by the docshell,
+  // thus we must force going through dns first, so that the urlbar thinks
+  // the value may be a url, and asks the docshell to visit it.
+  // On NS_ERROR_UNKNOWN_HOST the docshell will fix it up.
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.fixup.dns_first_for_single_words", true]],
+  });
   const kSearchEngineID = "test_urifixup_search_engine";
   const kSearchEngineURL = "http://localhost/?search={searchTerms}";
   await Services.search.addEngineWithDetails(kSearchEngineID, {
     method: "get",
     template: kSearchEngineURL,
   });
 
   let oldDefaultEngine = await Services.search.getDefault();
@@ -29,24 +36,20 @@ add_task(async function() {
     if (engine) {
       await Services.search.removeEngine(engine);
     }
   });
 
   let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
   gBrowser.selectedTab = tab;
 
-  gURLBar.value = "firefox health report";
+  gURLBar.value = "firefox";
   gURLBar.handleCommand();
 
   let [subject, data] = await TestUtils.topicObserved("keyword-search");
 
   let engine = Services.search.defaultEngine;
   Assert.ok(engine, "Have default search engine.");
   Assert.equal(engine, subject, "Notification subject is engine.");
-  Assert.equal(
-    data,
-    "firefox health report",
-    "Notification data is search term."
-  );
+  Assert.equal(data, "firefox", "Notification data is search term.");
 
   gBrowser.removeTab(tab);
 });