Bug 1594622 - Quantumbar: Remove the context.preselected property and rely on result.heuristic instead r=mak
authorDrew Willcoxon <adw@mozilla.com>
Fri, 08 Nov 2019 18:50:00 +0000
changeset 501351 44afbc73fe15710fcc9d1591d8a783a55473cb04
parent 501350 90ce21e7e469a6418e2d964aba45bbfe26e9e8fa
child 501352 8823aa7d4ff9ea4ea54da4b453fdd17ddefb3b5c
push id114168
push userdluca@mozilla.com
push dateSun, 10 Nov 2019 03:08:55 +0000
treeherdermozilla-inbound@33f64c1ef3e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1594622
milestone72.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 1594622 - Quantumbar: Remove the context.preselected property and rely on result.heuristic instead r=mak Please see bug 1594622 for a description. Differential Revision: https://phabricator.services.mozilla.com/D52120
browser/components/extensions/test/browser/browser_ext_urlbar.js
browser/components/extensions/test/xpcshell/test_ext_urlbar.js
browser/components/urlbar/UrlbarController.jsm
browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm
browser/components/urlbar/UrlbarProviderExtension.jsm
browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
browser/components/urlbar/UrlbarView.jsm
browser/components/urlbar/docs/overview.rst
browser/components/urlbar/tests/unit/test_muxer.js
--- a/browser/components/extensions/test/browser/browser_ext_urlbar.js
+++ b/browser/components/extensions/test/browser/browser_ext_urlbar.js
@@ -77,17 +77,16 @@ add_task(async function setUp() {
 // button.
 add_task(async function tip_onResultPicked_mainButton_noURL_enter() {
   let ext = await loadTipExtension();
   await UrlbarTestUtils.promiseAutocompleteResultPopup({
     window,
     waitForFocus,
     value: "test",
   });
-  EventUtils.synthesizeKey("KEY_ArrowDown");
   EventUtils.synthesizeKey("KEY_Enter");
   await ext.awaitMessage("onResultPicked received");
   await ext.unload();
 });
 
 // Loads a tip extension without a main button URL and clicks the main button.
 add_task(async function tip_onResultPicked_mainButton_noURL_mouse() {
   let ext = await loadTipExtension();
@@ -116,17 +115,16 @@ add_task(async function tip_onResultPick
       value: "test",
     });
     let loadedPromise = BrowserTestUtils.browserLoaded(
       gBrowser.selectedBrowser
     );
     ext.onMessage("onResultPicked received", () => {
       Assert.ok(false, "onResultPicked should not be called");
     });
-    EventUtils.synthesizeKey("KEY_ArrowDown");
     EventUtils.synthesizeKey("KEY_Enter");
     await loadedPromise;
     Assert.equal(gBrowser.currentURI.spec, "http://example.com/");
   });
   await ext.unload();
 });
 
 // Loads a tip extension with a main button URL and clicks the main button.
@@ -166,17 +164,17 @@ add_task(async function tip_onResultPick
       value: "test",
     });
     let loadedPromise = BrowserTestUtils.browserLoaded(
       gBrowser.selectedBrowser
     );
     ext.onMessage("onResultPicked received", () => {
       Assert.ok(false, "onResultPicked should not be called");
     });
-    EventUtils.synthesizeKey("KEY_ArrowDown", { repeat: 2 });
+    EventUtils.synthesizeKey("KEY_ArrowDown");
     EventUtils.synthesizeKey("KEY_Enter");
     await loadedPromise;
     Assert.equal(gBrowser.currentURI.spec, "http://example.com/");
   });
   await ext.unload();
 });
 
 // Loads a tip extension with a help button URL and clicks the help button.
--- a/browser/components/extensions/test/xpcshell/test_ext_urlbar.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_urlbar.js
@@ -815,16 +815,139 @@ add_task(async function test_activeInact
 
   // Check the results.
   Assert.equal(context.results.length, 1);
   Assert.equal(context.results[0].title, "Test result restricting");
 
   await ext.unload();
 });
 
+// Adds a restricting provider that returns a heuristic result.  The actual
+// result created from the extension's result should be a heuristic.
+add_task(async function test_heuristicRestricting() {
+  let ext = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["urlbar"],
+    },
+    isPrivileged: true,
+    incognitoOverride: "spanning",
+    background() {
+      browser.urlbar.onBehaviorRequested.addListener(query => {
+        return "restricting";
+      }, "test");
+      browser.urlbar.onResultsRequested.addListener(query => {
+        return [
+          {
+            type: "url",
+            source: "history",
+            heuristic: true,
+            payload: {
+              title: "Test result",
+              url: "http://example.com/",
+            },
+          },
+        ];
+      }, "test");
+    },
+  });
+  await ext.startup();
+
+  // Check the provider.
+  let provider = UrlbarProvidersManager.getProvider("test");
+  Assert.ok(provider);
+
+  // Run a query.
+  let context = new UrlbarQueryContext({
+    allowAutofill: false,
+    isPrivate: false,
+    maxResults: 10,
+    searchString: "test",
+  });
+  let controller = new UrlbarController({
+    browserWindow: {
+      location: {
+        href: AppConstants.BROWSER_CHROME_URL,
+      },
+    },
+  });
+  await controller.startQuery(context);
+
+  // Check the results.
+  Assert.equal(context.results.length, 1);
+  Assert.ok(context.results[0].heuristic);
+
+  await ext.unload();
+});
+
+// Adds a non-restricting provider that returns a heuristic result.  The actual
+// result created from the extension's result should *not* be a heuristic, and
+// the usual UnifiedComplete heuristic should be present.
+add_task(async function test_heuristicNonRestricting() {
+  let ext = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: ["urlbar"],
+    },
+    isPrivileged: true,
+    incognitoOverride: "spanning",
+    background() {
+      browser.urlbar.onBehaviorRequested.addListener(query => {
+        return "active";
+      }, "test");
+      browser.urlbar.onResultsRequested.addListener(query => {
+        return [
+          {
+            type: "url",
+            source: "history",
+            heuristic: true,
+            payload: {
+              title: "Test result",
+              url: "http://example.com/",
+            },
+          },
+        ];
+      }, "test");
+    },
+  });
+  await ext.startup();
+
+  // Check the provider.
+  let provider = UrlbarProvidersManager.getProvider("test");
+  Assert.ok(provider);
+
+  // Run a query.
+  let context = new UrlbarQueryContext({
+    allowAutofill: false,
+    isPrivate: false,
+    maxResults: 10,
+    searchString: "test",
+  });
+  let controller = new UrlbarController({
+    browserWindow: {
+      location: {
+        href: AppConstants.BROWSER_CHROME_URL,
+      },
+    },
+  });
+  await controller.startQuery(context);
+
+  // Check the results.  The first result should be UnifiedComplete's heuristic.
+  let firstResult = context.results[0];
+  Assert.ok(firstResult.heuristic);
+  Assert.equal(firstResult.type, UrlbarUtils.RESULT_TYPE.SEARCH);
+  Assert.equal(firstResult.source, UrlbarUtils.RESULT_SOURCE.SEARCH);
+  Assert.equal(firstResult.payload.engine, "Test engine");
+
+  // The extension result should be present but not the heuristic.
+  let result = context.results.find(r => r.title == "Test result");
+  Assert.ok(result);
+  Assert.ok(!result.heuristic);
+
+  await ext.unload();
+});
+
 // Adds an active provider that doesn't have a listener for onResultsRequested.
 // No results should be added.
 add_task(async function test_onResultsRequestedNotImplemented() {
   let ext = ExtensionTestUtils.loadExtension({
     manifest: {
       permissions: ["urlbar"],
     },
     isPrivileged: true,
--- a/browser/components/urlbar/UrlbarController.jsm
+++ b/browser/components/urlbar/UrlbarController.jsm
@@ -399,17 +399,17 @@ class UrlbarController {
     if (!url) {
       return;
     }
 
     switch (reason) {
       case "resultsadded": {
         // We should connect to an heuristic result, if it exists.
         if (
-          (result == context.results[0] && context.preselected) ||
+          (result == context.results[0] && result.heuristic) ||
           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")
             ) {
--- a/browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm
@@ -70,23 +70,21 @@ class MuxerUnifiedComplete extends Urlba
     ) {
       // Remove the result.
       context.results.splice(searchInPrivateWindowIndex, 1);
     }
 
     if (!context.results.length) {
       return;
     }
-    // Look for an heuristic result.  If it's a preselected search result, use
-    // search buckets, otherwise use normal buckets.
+    // Look for an heuristic result.  If it's a search result, use search
+    // buckets, otherwise use normal buckets.
     let heuristicResult = context.results.find(r => r.heuristic);
     let buckets =
-      context.preselected &&
-      heuristicResult &&
-      heuristicResult.type == UrlbarUtils.RESULT_TYPE.SEARCH
+      heuristicResult && heuristicResult.type == UrlbarUtils.RESULT_TYPE.SEARCH
         ? UrlbarPrefs.get("matchBucketsSearch")
         : UrlbarPrefs.get("matchBuckets");
     logger.debug(`Buckets: ${buckets}`);
     // These results have a suggested index and should be moved if possible.
     // The sorting is important, to avoid messing up indices later when we'll
     // insert these results.
     let reshuffleResults = context.results
       .filter(r => r.suggestedIndex >= 0)
@@ -103,18 +101,17 @@ class MuxerUnifiedComplete extends Urlba
         }
         if (handled.has(result)) {
           // Already handled.
           continue;
         }
 
         if (
           group == UrlbarUtils.RESULT_GROUP.HEURISTIC &&
-          result == heuristicResult &&
-          context.preselected
+          result == heuristicResult
         ) {
           // Handle the heuristic result.
           sortedResults.unshift(result);
           handled.add(result);
           slots--;
         } else if (group == RESULT_TYPE_TO_GROUP.get(result.type)) {
           // If there's no suggestedIndex, insert the result now, otherwise
           // we'll handle it later.
--- a/browser/components/urlbar/UrlbarProviderExtension.jsm
+++ b/browser/components/urlbar/UrlbarProviderExtension.jsm
@@ -313,16 +313,23 @@ class UrlbarProviderExtension extends Ur
     let result = new UrlbarResult(
       UrlbarProviderExtension.RESULT_TYPES[extResult.type],
       UrlbarProviderExtension.SOURCE_TYPES[extResult.source],
       ...UrlbarResult.payloadAndSimpleHighlights(
         context.tokens,
         extResult.payload || {}
       )
     );
+    if (extResult.heuristic && this.behavior == "restricting") {
+      // The muxer chooses the final heuristic result by taking the first one
+      // that claims to be the heuristic.  We don't want extensions to clobber
+      // UnifiedComplete's heuristic, so we allow this only if the provider is
+      // restricting.
+      result.heuristic = extResult.heuristic;
+    }
     if (extResult.suggestedIndex !== undefined) {
       result.suggestedIndex = extResult.suggestedIndex;
     }
     return result;
   }
 }
 
 // Maps extension result type enums to internal result types.
--- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
+++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm
@@ -213,38 +213,34 @@ function convertResultToMatches(context,
       comment: result.getCommentAt(i),
       firstToken: context.tokens[0],
       isHeuristic,
     });
     // Should not happen, but better safe than sorry.
     if (!match) {
       continue;
     }
-    // Manage autofill and preselected properties for the first match.
-    if (isHeuristic) {
-      if (style.includes("autofill") && result.defaultIndex == 0) {
-        let autofillValue = result.getValueAt(i);
-        if (
-          autofillValue
-            .toLocaleLowerCase()
-            .startsWith(context.searchString.toLocaleLowerCase())
-        ) {
-          match.autofill = {
-            value:
-              context.searchString +
-              autofillValue.substring(context.searchString.length),
-            selectionStart: context.searchString.length,
-            selectionEnd: autofillValue.length,
-          };
-        }
+    // Manage autofill for the first match.
+    if (isHeuristic && style.includes("autofill") && result.defaultIndex == 0) {
+      let autofillValue = result.getValueAt(i);
+      if (
+        autofillValue
+          .toLocaleLowerCase()
+          .startsWith(context.searchString.toLocaleLowerCase())
+      ) {
+        match.autofill = {
+          value:
+            context.searchString +
+            autofillValue.substring(context.searchString.length),
+          selectionStart: context.searchString.length,
+          selectionEnd: autofillValue.length,
+        };
       }
-
-      context.preselected = true;
-      match.heuristic = true;
     }
+    match.heuristic = isHeuristic;
     matches.push(match);
   }
   return { matches, done };
 }
 
 /**
  * Creates a new UrlbarResult from the provided data.
  * @param {array} tokens the search tokens.
--- a/browser/components/urlbar/UrlbarView.jsm
+++ b/browser/components/urlbar/UrlbarView.jsm
@@ -437,20 +437,22 @@ class UrlbarView {
   onQueryResults(queryContext) {
     this._queryContext = queryContext;
 
     if (!this.isOpen) {
       this.clear();
     }
     this._updateResults(queryContext);
 
-    let isFirstPreselectedResult = false;
+    let isHeuristicResult = false;
     if (queryContext.lastResultCount == 0) {
-      if (queryContext.preselected) {
-        isFirstPreselectedResult = true;
+      let firstResult = queryContext.results[0];
+
+      if (firstResult.heuristic) {
+        isHeuristicResult = true;
         this._selectElement(this._getFirstSelectableElement(), {
           updateInput: false,
           setAccessibleFocus: this.controller._userSelectionBehavior == "arrow",
         });
       } else {
         // Clear the selection when we get a new set of results.
         this._selectElement(null, {
           updateInput: false,
@@ -464,25 +466,25 @@ class UrlbarView {
         trimmedValue &&
           trimmedValue[0] != "@" &&
           (trimmedValue[0] != UrlbarTokenizer.RESTRICT.SEARCH ||
             trimmedValue.length != 1)
       );
 
       // The input field applies autofill on input, without waiting for results.
       // Once we get results, we can ask it to correct wrong predictions.
-      this.input.maybeClearAutofillPlaceholder(queryContext.results[0]);
+      this.input.maybeClearAutofillPlaceholder(firstResult);
     }
 
     this._openPanel();
 
-    if (isFirstPreselectedResult) {
-      // The first, preselected result may be a search alias result, so apply
-      // formatting if necessary.  Conversely, the first result of the previous
-      // query may have been an alias, so remove formatting if necessary.
+    if (isHeuristicResult) {
+      // The heuristic result may be a search alias result, so apply formatting
+      // if necessary.  Conversely, the heuristic result of the previous query
+      // may have been an alias, so remove formatting if necessary.
       this.input.formatValue();
     }
   }
 
   /**
    * 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.
    *
--- a/browser/components/urlbar/docs/overview.rst
+++ b/browser/components/urlbar/docs/overview.rst
@@ -41,17 +41,16 @@ It is augmented as it progresses through
     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
              // RESULT_SOURCE.*, that can be returned by the model.
 
     // Properties added by the Model.
-    preselected; // {boolean} whether the first result should be preselected.
     results; // {array} list of UrlbarResult objects.
     tokens; // {array} tokens extracted from the searchString, each token is an
             // object in the form {type, value, lowerCaseValue}.
     acceptableSources; // {array} list of UrlbarUtils.RESULT_SOURCE that the
                        // model will accept for this context.
   }
 
 
--- a/browser/components/urlbar/tests/unit/test_muxer.js
+++ b/browser/components/urlbar/tests/unit/test_muxer.js
@@ -110,17 +110,16 @@ add_task(async function test_preselected
       { url: "http://mozilla.org/c" }
     ),
   ];
   matches[1].heuristic = true;
 
   let providerName = registerBasicTestProvider(matches);
   let context = createContext(undefined, {
     providers: [providerName],
-    preselected: true,
   });
   let controller = new UrlbarController({
     browserWindow: {
       location: {
         href: AppConstants.BROWSER_CHROME_URL,
       },
     },
   });
@@ -168,17 +167,16 @@ add_task(async function test_preselected
   ];
   matches2[1].heuristic = true;
 
   let provider1Name = registerBasicTestProvider(matches1);
   let provider2Name = registerBasicTestProvider(matches2);
 
   let context = createContext(undefined, {
     providers: [provider1Name, provider2Name],
-    preselected: true,
   });
   let controller = new UrlbarController({
     browserWindow: {
       location: {
         href: AppConstants.BROWSER_CHROME_URL,
       },
     },
   });