Bug 1554038 - Make UrlbarInput::pickResult and UrlbarController::recordSelectedResult take a UrlbarResult instead of an index and remove UrlbarView::getResult. r=adw a=jcristau
authorDão Gottwald <dao@mozilla.com>
Thu, 30 May 2019 01:13:59 +0000
changeset 533528 8536ebb1c16ffc0ae48a1efb51770744db176fe5
parent 533527 e996cd24ede06d7c8c2a822092a7437ba9cfea15
child 533529 e26b8629b145879a5a5949e799f9dc02bf7a3f3e
push id11346
push usermalexandru@mozilla.com
push dateFri, 31 May 2019 16:15:31 +0000
treeherdermozilla-beta@72e634f9360a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, jcristau
bugs1554038
milestone68.0
Bug 1554038 - Make UrlbarInput::pickResult and UrlbarController::recordSelectedResult take a UrlbarResult instead of an index and remove UrlbarView::getResult. r=adw a=jcristau Differential Revision: https://phabricator.services.mozilla.com/D32708
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarInput.jsm
browser/components/urlbar/UrlbarResult.jsm
browser/components/urlbar/UrlbarView.jsm
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -157,17 +157,17 @@ class UrlbarController {
     }
 
     if (queryContext.lastResultCount == 0 && queryContext.results.length) {
       if (queryContext.results[0].autofill) {
         this.input.autofillFirstResult(queryContext.results[0]);
       }
       // The first time we receive results try to connect to the heuristic
       // result.
-      this.speculativeConnect(queryContext, 0, "resultsadded");
+      this.speculativeConnect(queryContext.results[0], queryContext, "resultsadded");
     }
 
     this._notify("onQueryResults", queryContext);
     // Update lastResultCount after notifying, so the view can use it.
     queryContext.lastResultCount = queryContext.results.length;
   }
 
   /**
@@ -351,39 +351,38 @@ class UrlbarController {
         }
         break;
     }
   }
 
   /**
    * Tries to initialize a speculative connection on a result.
    * Speculative connections are only supported for a subset of all the results.
+   * @param {UrlbarResult} result Tthe result to speculative connect to.
    * @param {UrlbarQueryContext} context The queryContext
-   * @param {number} resultIndex index of the result to speculative connect to.
    * @param {string} reason Reason for the speculative connect request.
    * @note speculative connect to:
    *  - Search engine heuristic results
    *  - autofill results
    *  - http/https results
    */
-  speculativeConnect(context, resultIndex, reason) {
+  speculativeConnect(result, context, reason) {
     // Never speculative connect in private contexts.
     if (!this.input || context.isPrivate || context.results.length == 0) {
       return;
     }
-    let result = context.results[resultIndex];
     let {url} = UrlbarUtils.getUrlFromResult(result);
     if (!url) {
       return;
     }
 
     switch (reason) {
       case "resultsadded": {
         // We should connect to an heuristic result, if it exists.
-        if ((resultIndex == 0 && context.preselected) || result.autofill) {
+        if ((result == context.results[0] && context.preselected) || 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;
               UrlbarUtils.setupSpeculativeConnection(engine, this.browserWindow);
             }
           } else if (result.autofill) {
@@ -421,29 +420,27 @@ class UrlbarController {
   }
 
   /**
    * Records details of the selected result in telemetry. We only record the
    * selection behavior, type and index.
    *
    * @param {Event} event
    *   The event which triggered the result to be selected.
-   * @param {number} resultIndex
-   *   The index of the result.
+   * @param {UrlbarResult} result
+   *   The selected result.
    */
-  recordSelectedResult(event, resultIndex) {
-    let result;
+  recordSelectedResult(event, result) {
+    let resultIndex = result ? result.uiIndex : -1;
     let selectedResult = -1;
-
     if (resultIndex >= 0) {
-      result = this.view.getResult(resultIndex);
       // Except for the history popup, the urlbar always has a selection.  The
       // first result at index 0 is the "heuristic" result that indicates what
       // will happen when you press the Enter key.  Treat it as no selection.
-      selectedResult = resultIndex > 0 || !result.heuristic ? resultIndex : -1;
+      selectedResult = (resultIndex > 0 || !result.heuristic) ? resultIndex : -1;
     }
     BrowserUsageTelemetry.recordUrlbarSelectedResultMethod(
       event, selectedResult, this._userSelectionBehavior);
 
     if (!result) {
       return;
     }
 
--- a/browser/components/urlbar/UrlbarInput.jsm
+++ b/browser/components/urlbar/UrlbarInput.jsm
@@ -351,27 +351,27 @@ class UrlbarInput {
       if (selectedOneOff && !selectedOneOff.engine) {
         selectedOneOff.doCommand();
         return;
       }
     }
 
     // Use the selected result if we have one; this is usually the case
     // when the view is open.
-    let index = this.view.selectedIndex;
-    if (!selectedOneOff && index != -1) {
-      this.pickResult(event, index);
+    let result = this.view.selectedResult;
+    if (!selectedOneOff && result) {
+      this.pickResult(result, event);
       return;
     }
 
     let url;
     if (selectedOneOff) {
       // If there's a selected one-off button then load a search using
       // the button's engine.
-      let result = this._resultForCurrentValue;
+      result = this._resultForCurrentValue;
       let searchString =
         (result && (result.payload.suggestion || result.payload.query)) ||
         this._lastSearchString;
       [url, openParams.postData] = UrlbarUtils.getSearchQueryUrl(
         selectedOneOff.engine, searchString);
       this._recordSearch(selectedOneOff.engine, event);
     } else {
       // Use the current value if we don't have a UrlbarResult e.g. because the
@@ -379,22 +379,20 @@ class UrlbarInput {
       url = this.value;
       openParams.postData = null;
     }
 
     if (!url) {
       return;
     }
 
+    this.controller.recordSelectedResult(event, result || this.view.selectedResult);
+
     let where = openWhere || this._whereToOpen(event);
-
     openParams.allowInheritPrincipal = false;
-
-    this.controller.recordSelectedResult(event, index);
-
     url = this._maybeCanonizeURL(event, url) || url.trim();
 
     try {
       new URL(url);
     } catch (ex) {
       let browser = this.window.gBrowser.selectedBrowser;
       let lastLocationChange = browser.lastLocationChange;
 
@@ -418,31 +416,31 @@ class UrlbarInput {
     if (this.value && this.focused) {
       this.select();
     }
   }
 
   /**
    * Called by the view when a result is picked.
    *
+   * @param {UrlbarResult} result The result that was picked.
    * @param {Event} event The event that picked the result.
-   * @param {resultIndex} resultIndex The index of the result that was picked.
    */
-  pickResult(event, resultIndex) {
-    let result = this.view.getResult(resultIndex);
+  pickResult(result, event) {
     let isCanonized = this.setValueFromResult(result, event);
     let where = this._whereToOpen(event);
     let openParams = {
       allowInheritPrincipal: false,
     };
 
     if (!result.payload.isKeywordOffer) {
       this.view.close();
     }
-    this.controller.recordSelectedResult(event, resultIndex);
+
+    this.controller.recordSelectedResult(event, result);
 
     if (isCanonized) {
       this._loadURL(this.value, where, openParams);
       return;
     }
 
     let {url, postData} = UrlbarUtils.getUrlFromResult(result);
     openParams.postData = postData;
--- a/browser/components/urlbar/UrlbarResult.jsm
+++ b/browser/components/urlbar/UrlbarResult.jsm
@@ -49,16 +49,19 @@ class UrlbarResult {
 
     // Source describes which data has been used to derive this result. In case
     // multiple sources are involved, use the more privacy restricted.
     if (!Object.values(UrlbarUtils.RESULT_SOURCE).includes(resultSource)) {
       throw new Error("Invalid result source");
     }
     this.source = resultSource;
 
+    // UrlbarView is responsible for updating this.
+    this.uiIndex = -1;
+
     // May be used to indicate an heuristic result. Heuristic results can bypass
     // source filters in the ProvidersManager, that otherwise may skip them.
     this.heuristic = false;
 
     // The payload contains result data. Some of the data is common across
     // multiple types, but most of it will vary.
     if (!payload || (typeof payload != "object")) {
       throw new Error("Invalid result payload");
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@@ -75,17 +75,17 @@ class UrlbarView {
              this._queryContext.results[0] &&
              this._queryContext.results[0].heuristic);
   }
 
   get selectedIndex() {
     if (!this.isOpen || !this._selected) {
       return -1;
     }
-    return parseInt(this._selected.getAttribute("resultIndex"));
+    return this._selected.result.uiIndex;
   }
 
   set selectedIndex(val) {
     if (!this.isOpen) {
       throw new Error("UrlbarView: Cannot select an item if the view isn't open.");
     }
 
     if (val < 0) {
@@ -109,29 +109,16 @@ class UrlbarView {
   get selectedResult() {
     if (!this.isOpen || !this._selected) {
       return null;
     }
     return this._selected.result;
   }
 
   /**
-   * Gets the result for the index.
-   * @param {number} index
-   *   The index to look up.
-   * @returns {UrlbarResult}
-   */
-  getResult(index) {
-    if (index < 0 || index > this._queryContext.results.length) {
-      throw new Error(`UrlbarView: Index ${index} is out of bounds`);
-    }
-    return this._queryContext.results[index];
-  }
-
-  /**
    * Moves the view selection forward or backward.
    *
    * @param {number} amount
    *   The number of steps to move.
    * @param {boolean} options.reverse
    *   Set to true to select the previous item. By default the next item
    *   will be selected.
    */
@@ -258,25 +245,21 @@ class UrlbarView {
    * Handles removing a result from the view when it is removed from the query,
    * and attempts to select the new result on the same row.
    *
    * This assumes that the result rows are in index order.
    *
    * @param {number} index The index of the result that has been removed.
    */
   onQueryResultRemoved(index) {
-    // Change the index for any rows above the removed index.
-    for (let i = index + 1; i < this._rows.children.length; i++) {
-      let child = this._rows.children[i];
-      child.setAttribute("resultIndex", child.getAttribute("resultIndex") - 1);
-    }
-
     let rowToRemove = this._rows.children[index];
     rowToRemove.remove();
 
+    this._updateIndices();
+
     if (rowToRemove != this._selected) {
       return;
     }
 
     // Select the row at the same index, if possible.
     let newSelectionIndex = index;
     if (index >= this._queryContext.results.length) {
       newSelectionIndex = this._queryContext.results.length - 1;
@@ -477,16 +460,18 @@ class UrlbarView {
       this._updateRow(row, results[resultIndex]);
       // Due to stale rows, we may have more rows than maxResults, thus we must
       // hide them, and we'll revert this when stale rows are removed.
       if (this._rows.children.length >= queryContext.maxResults) {
         row.style.display = "none";
       }
       this._rows.appendChild(row);
     }
+
+    this._updateIndices();
   }
 
   _createRow() {
     let item = this._createElement("div");
     item.className = "urlbarView-row";
     item.setAttribute("role", "option");
     item._elements = new Map;
 
@@ -527,21 +512,18 @@ class UrlbarView {
     url.className = "urlbarView-secondary urlbarView-url";
     content.appendChild(url);
     item._elements.set("url", url);
 
     return item;
   }
 
   _updateRow(item, result) {
-    let resultIndex = this._queryContext.results.indexOf(result);
     item.result = result;
     item.removeAttribute("stale");
-    item.id = "urlbarView-row-" + resultIndex;
-    item.setAttribute("resultIndex", resultIndex);
 
     if (result.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
         !result.payload.isKeywordOffer) {
       item.setAttribute("type", "search");
     } else if (result.type == UrlbarUtils.RESULT_TYPE.REMOTE_TAB) {
       item.setAttribute("type", "remotetab");
     } else if (result.type == UrlbarUtils.RESULT_TYPE.TAB_SWITCH) {
       item.setAttribute("type", "switchtab");
@@ -629,16 +611,24 @@ class UrlbarView {
     } else {
       title.removeAttribute("isurl");
     }
     item._elements.get("action").textContent = action;
 
     item._elements.get("titleSeparator").hidden = !action && !setURL;
   }
 
+  _updateIndices() {
+    for (let i = 0; i < this._rows.children.length; i++) {
+      let item = this._rows.children[i];
+      item.result.uiIndex = i;
+      item.id = "urlbarView-row-" + i;
+    }
+  }
+
   _removeStaleRows() {
     let row = this._rows.lastElementChild;
     while (row) {
       let next = row.previousElementSibling;
       if (row.hasAttribute("stale")) {
         row.remove();
       } else {
         row.style.display = "";
@@ -794,30 +784,30 @@ class UrlbarView {
       return;
     }
 
     let row = event.target;
     while (!row.classList.contains("urlbarView-row")) {
       row = row.parentNode;
     }
     this._selectItem(row, { updateInput: false });
-    this.controller.speculativeConnect(this._queryContext, this.selectedIndex, "mousedown");
+    this.controller.speculativeConnect(this.selectedResult, this._queryContext, "mousedown");
   }
 
   _on_mouseup(event) {
     if (event.button == 2) {
       // Ignore right clicks.
       return;
     }
 
     let row = event.target;
     while (!row.classList.contains("urlbarView-row")) {
       row = row.parentNode;
     }
-    this.input.pickResult(event, parseInt(row.getAttribute("resultIndex")));
+    this.input.pickResult(row.result, event);
   }
 
   _on_overflow(event) {
     if (event.detail == 1 &&
         (event.target.classList.contains("urlbarView-url") ||
          event.target.classList.contains("urlbarView-title"))) {
       event.target.toggleAttribute("overflow", true);
       event.target.setAttribute("title", event.target._tooltip);