Bug 1512654 - Properly display search matches and make them functional on the new QuantumBar. r=dao
authorMark Banner <standard8@mozilla.com>
Fri, 18 Jan 2019 10:40:22 +0000
changeset 511525 96880bb35099158e47b4add1dade4f483ba5a4b0
parent 511524 0eae1c157b3e3642b4cabe1424923d761675c39b
child 511526 da5caa27480c7b10fd4a279b3d505dcb693853aa
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)
reviewersdao
bugs1512654
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 1512654 - Properly display search matches and make them functional on the new QuantumBar. r=dao This implements the correct display of search matches, except for the fact that the "Search with" text should not be shown by default only on hover. The actual selection of the result should work the same as for the quantum bar, and have the same telemetry hooked up. Depends on D16617 Differential Revision: https://phabricator.services.mozilla.com/D16697
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarMatch.jsm
browser/components/urlbar/UrlbarView.jsm
browser/components/urlbar/tests/unit/test_providerOpenTabs.js
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -287,16 +287,18 @@ class UrlbarInput {
     //   event, this.userSelectionBehavior);
 
     let where = this._whereToOpen(event);
     let openParams = {
       postData: null,
       allowInheritPrincipal: false,
     };
 
+    let url = result.payload.url;
+
     switch (result.type) {
       case UrlbarUtils.MATCH_TYPE.TAB_SWITCH: {
         if (this._overrideDefaultAction(event)) {
           where = "current";
           break;
         }
 
         this.handleRevert();
@@ -306,46 +308,65 @@ class UrlbarInput {
         };
 
         if (this.window.switchToTabHavingURI(Services.io.newURI(result.payload.url), false, loadOpts) &&
             prevTab.isEmpty) {
           this.window.gBrowser.removeTab(prevTab);
         }
         return;
       }
-      case UrlbarUtils.MATCH_TYPE.SEARCH:
-        // TODO: port _parseAndRecordSearchEngineLoad.
-        return;
+      case UrlbarUtils.MATCH_TYPE.SEARCH: {
+        const actionDetails = {
+          isSuggestion: !!result.payload.suggestion,
+          alias: result.payload.keyword,
+        };
+        const engine = Services.search.getEngineByName(result.payload.engine);
+
+        [url, openParams.postData] = this._getSearchQueryUrl(
+          engine, result.payload.suggestion || result.payload.query);
+        this._recordSearch(engine, event, actionDetails);
+        break;
+      }
       case UrlbarUtils.MATCH_TYPE.OMNIBOX:
         // Give the extension control of handling the command.
         ExtensionSearchHandler.handleInputEntered(result.payload.keyword,
                                                   result.payload.content,
                                                   where);
         return;
     }
 
-    this._loadURL(result.payload.url, where, openParams);
+    this._loadURL(url, where, openParams);
   }
 
   /**
    * Called by the view when moving through results with the keyboard.
    *
    * @param {UrlbarMatch} result The result that was selected.
    */
   setValueFromResult(result) {
-    // FIXME: This is wrong, not all the matches have a url. For example
-    // extension matches will call into the extension code rather than loading
-    // a url. That means we likely can't use the url as our value.
-    let val = result.payload.url;
-    let uri;
-    try {
-      uri = Services.io.newURI(val);
-    } catch (ex) {}
-    if (uri) {
-      val = this.window.losslessDecodeURI(uri);
+    let val;
+
+    switch (result.type) {
+      case UrlbarUtils.MATCH_TYPE.SEARCH:
+        val = result.payload.suggestion || result.payload.query;
+        break;
+      default: {
+        // FIXME: This is wrong, not all the other matches have a url. For example
+        // extension matches will call into the extension code rather than loading
+        // a url. That means we likely can't use the url as our value.
+        val = result.payload.url;
+        let uri;
+        try {
+          uri = Services.io.newURI(val);
+        } catch (ex) {}
+        if (uri) {
+          val = this.window.losslessDecodeURI(uri);
+        }
+        break;
+      }
     }
     this.value = val;
   }
 
   /**
    * Starts a query based on the user input.
    *
    * @param {number} [options.lastKey]
@@ -514,16 +535,65 @@ class UrlbarInput {
   _overrideDefaultAction(event) {
     return event.shiftKey ||
            event.altKey ||
            (AppConstants.platform == "macosx" ?
               event.metaKey : event.ctrlKey);
   }
 
   /**
+   * Get the url to load for the search query and records in telemetry that it
+   * is being loaded.
+   *
+   * @param {nsISearchEngine} engine
+   *   The engine to generate the query for.
+   * @param {string} query
+   *   The query string to search for.
+   * @returns {array}
+   *   Returns an array containing the query url (string) and the
+   *    post data (object).
+   */
+  _getSearchQueryUrl(engine, query) {
+    let submission = engine.getSubmission(query, null, "keyword");
+    return [submission.uri.spec, submission.postData];
+  }
+
+  /**
+   * Get the url to load for the search query and records in telemetry that it
+   * is being loaded.
+   *
+   * @param {nsISearchEngine} engine
+   *   The engine to generate the query for.
+   * @param {Event} event
+   *   The event that triggered this query.
+   * @param {object} searchActionDetails
+   *   The details associated with this search query.
+   * @param {boolean} searchActionDetails.isSuggestion
+   *   True if this query was initiated from a suggestion from the search engine.
+   * @param {alias} searchActionDetails.alias
+   *   True if this query was initiated via a search alias.
+   */
+  _recordSearch(engine, event, searchActionDetails = {}) {
+    const isOneOff = this.view.oneOffSearchButtons.maybeRecordTelemetry(event);
+    // Infer the type of the event which triggered the search.
+    let eventType = "unknown";
+    if (event instanceof KeyboardEvent) {
+      eventType = "key";
+    } else if (event instanceof MouseEvent) {
+      eventType = "mouse";
+    }
+    // Augment the search action details object.
+    let details = searchActionDetails;
+    details.isOneOff = isOneOff;
+    details.type = eventType;
+
+    this.window.BrowserSearch.recordSearchInTelemetry(engine, "urlbar", details);
+  }
+
+  /**
    * Loads the url in the appropriate place.
    *
    * @param {string} url
    *   The URL to open.
    * @param {string} openUILinkWhere
    *   Where we expect the result to be opened.
    * @param {object} params
    *   The parameters related to how and where the result will be opened.
--- a/browser/components/urlbar/UrlbarMatch.jsm
+++ b/browser/components/urlbar/UrlbarMatch.jsm
@@ -96,19 +96,21 @@ class UrlbarMatch {
   get _titleAndHighlights() {
     switch (this.type) {
       case UrlbarUtils.MATCH_TYPE.TAB_SWITCH:
       case UrlbarUtils.MATCH_TYPE.URL:
       case UrlbarUtils.MATCH_TYPE.OMNIBOX:
       case UrlbarUtils.MATCH_TYPE.REMOTE_TAB:
         return this.payload.title ?
                [this.payload.title, this.payloadHighlights.title] :
-               ["", []];
+               [this.payload.url || "", this.payloadHighlights.url || []];
       case UrlbarUtils.MATCH_TYPE.SEARCH:
-        return [this.payload.engine, this.payloadHighlights.engine];
+        return this.payload.suggestion ?
+               [this.payload.suggestion, this.payloadHighlights.suggestion] :
+               [this.payload.query, this.payloadHighlights.query];
       default:
         return ["", []];
     }
   }
 
   /**
    * Returns an icon url.
    * @returns {string} url of the icon.
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@@ -3,19 +3,24 @@
  * 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, {
+  Services: "resource://gre/modules/Services.jsm",
   UrlbarUtils: "resource:///modules/UrlbarUtils.jsm",
 });
 
+XPCOMUtils.defineLazyGetter(this, "bundle", function() {
+  return Services.strings.createBundle("chrome://global/locale/autocomplete.properties");
+});
+
 /**
  * Receives and displays address bar autocomplete results.
  */
 class UrlbarView {
   /**
    * @param {UrlbarInput} input
    *   The UrlbarInput instance belonging to this UrlbarView instance.
    */
@@ -271,32 +276,37 @@ class UrlbarView {
     } else {
       favicon.src = result.payload.icon || "chrome://mozapps/skin/places/defaultFavicon.svg";
     }
     content.appendChild(favicon);
 
     let title = this._createElement("span");
     title.className = "urlbarView-title";
     this._addTextContentWithHighlights(
-      title,
-      ...(result.title ?
-          [result.title, result.titleHighlights] :
-          [result.payload.url || "", result.payloadHighlights.url || []])
-    );
+      title, result.title, result.titleHighlights);
     content.appendChild(title);
 
     let secondary = this._createElement("span");
     secondary.className = "urlbarView-secondary";
-    if (result.type == UrlbarUtils.MATCH_TYPE.TAB_SWITCH) {
-      secondary.classList.add("urlbarView-action");
-      this._addTextContentWithHighlights(secondary, "Switch to Tab", []);
-    } else {
-      secondary.classList.add("urlbarView-url");
-      this._addTextContentWithHighlights(secondary, result.payload.url || "",
-                                         result.payloadHighlights.url || []);
+    switch (result.type) {
+      case UrlbarUtils.MATCH_TYPE.TAB_SWITCH:
+        secondary.classList.add("urlbarView-action");
+        secondary.textContent = bundle.GetStringFromName("switchToTab2");
+        break;
+      case UrlbarUtils.MATCH_TYPE.SEARCH:
+        secondary.classList.add("urlbarView-action");
+        secondary.textContent =
+          bundle.formatStringFromName("searchWithEngine",
+                                      [result.payload.engine], 1);
+        break;
+      default:
+        secondary.classList.add("urlbarView-url");
+        this._addTextContentWithHighlights(secondary, result.payload.url || "",
+                                           result.payloadHighlights.url || []);
+        break;
     }
     content.appendChild(secondary);
 
     this._rows.appendChild(item);
   }
 
   /**
    * Adds text content to a node, placing substrings that should be highlighted
--- a/browser/components/urlbar/tests/unit/test_providerOpenTabs.js
+++ b/browser/components/urlbar/tests/unit/test_providerOpenTabs.js
@@ -17,17 +17,17 @@ add_task(async function test_openTabs() 
   let context = createContext();
   let matchCount = 0;
   let callback = function(provider, match) {
     matchCount++;
     Assert.equal(provider, UrlbarProviderOpenTabs, "Got the expected provider");
     Assert.equal(match.type, UrlbarUtils.MATCH_TYPE.TAB_SWITCH,
                  "Got the expected match type");
     Assert.equal(match.payload.url, url, "Got the expected url");
-    Assert.equal(match.title, "", "Got the expected title");
+    Assert.equal(match.payload.title, undefined, "Got the expected title");
   };
 
   await UrlbarProviderOpenTabs.startQuery(context, callback);
   Assert.equal(matchCount, 1, "Found the expected number of matches");
   // Sanity check that this doesn't throw.
   UrlbarProviderOpenTabs.cancelQuery(context);
   Assert.equal(UrlbarProviderOpenTabs.queries.size, 0,
     "All the queries have been removed");