Bug 1645521 - Part 2 - Allow for multiple heuristic providers and enable ProviderHeuristicFallback. r=adw

Differential Revision: https://phabricator.services.mozilla.com/D80293

browser/components/extensions/test/xpcshell/test_ext_urlbar.js

@@ -188,7 +188,7 @@ add_task(async function test_registerProvider() {
 });
 
 // Adds a single active provider that returns many kinds of results.  This also
-// checks that the heuristic result from the built-in UnifiedComplete provider
+// checks that the heuristic result from the built-in HeuristicFallback provider
 // is included.
 add_task(async function test_onProviderResultsRequested() {
   let ext = ExtensionTestUtils.loadExtension({
@@ -276,7 +276,7 @@ add_task(async function test_onProviderResultsRequested() {
 
   // Check the results.
   let expectedResults = [
-    // The first result should be a search result returned by UnifiedComplete.
+    // The first result should be a search result returned by HeuristicFallback.
     {
       type: UrlbarUtils.RESULT_TYPE.SEARCH,
       source: UrlbarUtils.RESULT_SOURCE.SEARCH,

browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm

@@ -41,6 +41,15 @@ function groupFromResult(result) {
   }
 }
 
+// Breaks ties among heuristic results. Providers higher up the list are higher
+// priority.
+const heuristicOrder = [
+  // Test providers are handled in sort(),
+  // Extension providers are handled in sort(),
+  "UrlbarProviderSearchTips",
+  "UnifiedComplete",
+  "HeuristicFallback",
+];
 /**
  * Class used to create a muxer.
  * The muxer receives and sorts results in a UrlbarQueryContext.
@@ -54,11 +63,19 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
     return "UnifiedComplete";
   }
 
+  /* eslint-disable complexity */
   /**
    * Sorts results in the given UrlbarQueryContext.
    *
+   * sort() is not suitable to be broken up into smaller functions or to rely
+   * on more convenience functions. It exists to efficiently group many
+   * conditions into just three loops. As a result, we must disable complexity
+   * linting.
+   *
    * @param {UrlbarQueryContext} context
    *   The query context.
+   * @returns {boolean} If results were successfully sorted. This return value
+   *   is a stopgap and can be removed when bug 1648468 lands.
    */
   sort(context) {
     // This method is called multiple times per keystroke, so it should be as
@@ -70,19 +87,42 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
 
     // Capture information about the heuristic result to dedupe results from the
     // heuristic more quickly.
+    let topHeuristicRank = Infinity;
+    for (let result of context.allHeuristicResults) {
+      // Determine the highest-ranking heuristic result.
+      if (!result.heuristic) {
+        continue;
+      }
+
+      // + 2 to reserve the highest-priority slots for test and extension
+      // providers.
+      let heuristicRank = heuristicOrder.indexOf(result.providerName) + 2;
+      // Extension and test provider names vary widely and aren't suitable
+      // for a static safelist like heuristicOrder.
+      if (result.providerType == UrlbarUtils.PROVIDER_TYPE.EXTENSION) {
+        heuristicRank = 1;
+      } else if (result.providerName.startsWith("TestProvider")) {
+        heuristicRank = 0;
+      } else if (heuristicRank - 2 == -1) {
+        throw new Error(
+          `Heuristic result returned by unexpected provider: ${result.providerName}`
+        );
+      }
+      // Replace in case of ties, which would occur if a provider sent two
+      // heuristic results.
+      if (heuristicRank <= topHeuristicRank) {
+        topHeuristicRank = heuristicRank;
+        context.heuristicResult = result;
+      }
+    }
+
     let heuristicResultQuery;
-    let heuristicResultOmniboxContent;
     if (context.heuristicResult) {
       if (
         context.heuristicResult.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
         context.heuristicResult.payload.query
       ) {
         heuristicResultQuery = context.heuristicResult.payload.query.toLocaleLowerCase();
-      } else if (
-        context.heuristicResult.type == UrlbarUtils.RESULT_TYPE.OMNIBOX &&
-        context.heuristicResult.payload.content
-      ) {
-        heuristicResultOmniboxContent = context.heuristicResult.payload.content.toLocaleLowerCase();
       }
     }
 
@@ -95,10 +135,17 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
       UrlbarPrefs.get("maxHistoricalSearchSuggestions"),
       context.maxResults
     );
+    let hasUnifiedComplete = false;
 
     // Do the first pass through the results.  We only collect info for the
     // second pass here.
     for (let result of context.results) {
+      // Keep track of whether UnifiedComplete has returned results. We will
+      // quit early if it hasn't.
+      if (result.providerName == "UnifiedComplete") {
+        hasUnifiedComplete = true;
+      }
+
       // The "Search in a Private Window" result should only be shown when there
       // are other results and all of them are searches.  It should not be shown
       // if the user typed an alias because that's an explicit engine choice.
@@ -139,9 +186,24 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
       }
     }
 
+    // Quit early if we're still waiting on UnifiedComplete. If it turns out
+    // UnifiedComplete doesn't need to return any results, it will call sort()
+    // regardless to unblock showing results.
+    if (
+      !hasUnifiedComplete &&
+      context.pendingHeuristicProviders.has("UnifiedComplete")
+    ) {
+      return false;
+    }
+
     // Do the second pass through results to build the list of unsorted results.
     let unsortedResults = [];
     for (let result of context.results) {
+      // Exclude low-ranked heuristic results.
+      if (result.heuristic && result != context.heuristicResult) {
+        continue;
+      }
+
       // Exclude "Search in a Private Window" as determined in the first pass.
       if (
         result.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
@@ -214,15 +276,6 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
         }
       }
 
-      // Exclude omnibox results that dupe the heuristic.
-      if (
-        !result.heuristic &&
-        result.type == UrlbarUtils.RESULT_TYPE.OMNIBOX &&
-        result.payload.content == heuristicResultOmniboxContent
-      ) {
-        continue;
-      }
-
       // Include this result.
       unsortedResults.push(result);
     }
@@ -272,6 +325,7 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
     }
 
     context.results = sortedResults;
+    return true;
   }
 
   /**

browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm

@@ -63,7 +63,7 @@ class ProviderHeuristicFallback extends UrlbarProvider {
    * @returns {boolean} Whether this provider should be invoked for the search.
    */
   isActive(queryContext) {
-    return false; // TODO: Set to true in future part of this patch.
+    return true;
   }
 
   /**

browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm

@@ -90,8 +90,17 @@ class ProviderUnifiedComplete extends UrlbarProvider {
         acResult,
         urls
       );
-      for (let result of results) {
-        addCallback(this, result);
+      // The Muxer gives UnifiedComplete special status and waits for it to
+      // return results before sorting them. We need to know that
+      // UnifiedComplete has finished even if it isn't returning results, so we
+      // call addCallback with an empty result here, which is something only
+      // UnifiedComplete is allowed to do.
+      if (!results.length) {
+        addCallback(this, null);
+      } else {
+        for (let result of results) {
+          addCallback(this, result);
+        }
       }
     });
     this.queries.delete(queryContext);

browser/components/urlbar/UrlbarProvidersManager.jsm

@@ -366,6 +366,7 @@ class Query {
     let queryPromises = [];
     for (let provider of activeProviders) {
       if (provider.type == UrlbarUtils.PROVIDER_TYPE.HEURISTIC) {
+        this.context.pendingHeuristicProviders.add(provider.name);
         queryPromises.push(
           provider.tryMethod("startQuery", this.context, this.add.bind(this))
         );
@@ -436,13 +437,29 @@ class Query {
     if (!(provider instanceof UrlbarProvider)) {
       throw new Error("Invalid provider passed to the add callback");
     }
+
+    // When this set is empty, we can display heuristic results early. We remove
+    // the provider from the list without checking result.heuristic since
+    // heuristic providers don't necessarily have to return heuristic results.
+    // We expect a provider with type HEURISTIC will return its heuristic
+    // result(s) first.
+    this.context.pendingHeuristicProviders.delete(provider.name);
+
     // Stop returning results as soon as we've been canceled.
     if (this.canceled) {
       return;
     }
+
+    let addResult = true;
+
+    if (!result) {
+      addResult = false;
+    }
+
     // Check if the result source should be filtered out. Pay attention to the
     // heuristic result though, that is supposed to be added regardless.
     if (
+      addResult &&
       !this.acceptableSources.includes(result.source) &&
       !result.heuristic &&
       // Treat form history as searches for the purpose of acceptableSources.
@@ -450,35 +467,60 @@ class Query {
         result.source != UrlbarUtils.RESULT_SOURCE.HISTORY ||
         !this.acceptableSources.includes(UrlbarUtils.RESULT_SOURCE.SEARCH))
     ) {
-      return;
+      addResult = false;
     }
 
     // Filter out javascript results for safety. The provider is supposed to do
     // it, but we don't want to risk leaking these out.
     if (
+      addResult &&
       result.type != UrlbarUtils.RESULT_TYPE.KEYWORD &&
       result.payload.url &&
       result.payload.url.startsWith("javascript:") &&
       !this.context.searchString.startsWith("javascript:") &&
       UrlbarPrefs.get("filter.javascript")
     ) {
-      return;
+      addResult = false;
     }
 
+    // We wait on UnifiedComplete to return a heuristic result. If it has no
+    // heuristic result to return, we still need to sort and display results, so
+    // we follow the usual codepath to muxer.sort. We only offer this for
+    // UnifiedComplete, so we check the provider's name here. This is a stopgap
+    // measure until bug 1648468 lands.
+    if (!addResult) {
+      if (provider.name == "UnifiedComplete") {
+        this._notifyResultsFromProvider(provider);
+      }
+      return;
+    }
     result.providerName = provider.name;
+    result.providerType = provider.type;
     this.context.results.push(result);
     if (result.heuristic) {
-      this.context.heuristicResult = result;
+      this.context.allHeuristicResults.push(result);
     }
 
     this._notifyResultsFromProvider(provider);
   }
 
   _notifyResultsFromProvider(provider) {
-    // If the provider is not of heuristic type, chunk results, to improve the
-    // dataflow and reduce UI flicker.
+    // We create two chunking timers: one for heuristic results, and one for
+    // other results. We expect heuristic providers to return their heuristic
+    // results before other results/providers in most cases. When all heuristic
+    // providers have returned some results, we fire the heuristic timer early.
+    // If the timer fires first, we stop waiting on the remaining heuristic
+    // providers.
+    // Both timers are used to reduce UI flicker.
     if (provider.type == UrlbarUtils.PROVIDER_TYPE.HEURISTIC) {
-      this._notifyResults();
+      if (!this._heuristicProviderTimer) {
+        this._heuristicProviderTimer = new SkippableTimer({
+          name: "Heuristic provider timer",
+          callback: () => this._notifyResults(),
+          time: CHUNK_RESULTS_DELAY_MS,
+          logger,
+        });
+      }
     } else if (!this._chunkTimer) {
       this._chunkTimer = new SkippableTimer({
         name: "Query chunk timer",
@@ -487,16 +529,33 @@ class Query {
         logger,
       });
     }
+    // If all active heuristic providers have returned results, we can skip the
+    // heuristic results timer and start showing results immediately.
+    if (
+      this._heuristicProviderTimer &&
+      !this.context.pendingHeuristicProviders.size
+    ) {
+      this._heuristicProviderTimer.fire().catch(Cu.reportError);
+    }
   }
 
   _notifyResults() {
-    this.muxer.sort(this.context);
+    let sorted = this.muxer.sort(this.context);
+
+    if (this._heuristicProviderTimer) {
+      this._heuristicProviderTimer.cancel().catch(Cu.reportError);
+      this._heuristicProviderTimer = null;
+    }
 
     if (this._chunkTimer) {
       this._chunkTimer.cancel().catch(Cu.reportError);
       this._chunkTimer = null;
     }
 
+    if (!sorted) {
+      return;
+    }
+
     // Before the muxer.sort call above, this.context.results should never be
     // empty since this method is called when results are added.  But the muxer
     // may have excluded one or more results from the final sorted list.  For

browser/components/urlbar/UrlbarUtils.jsm

@@ -651,7 +651,7 @@ var UrlbarUtils = {
         "usercontextid"
       ),
       allowSearchSuggestions: false,
-      providers: ["UnifiedComplete"],
+      providers: ["UnifiedComplete", "HeuristicFallback"],
     });
     await UrlbarProvidersManager.startQuery(context);
     if (!context.heuristicResult) {
@@ -1000,6 +1000,8 @@ class UrlbarQueryContext {
     }
 
     this.lastResultCount = 0;
+    this.allHeuristicResults = [];
+    this.pendingHeuristicProviders = new Set();
     this.userContextId =
       options.userContextId ||
       Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID;

browser/components/urlbar/tests/UrlbarTestUtils.jsm

@@ -598,7 +598,7 @@ class TestProvider extends UrlbarProvider {
    */
   constructor({
     results,
-    name = "TestProvider" + Math.floor(Math.random() * 100000),
+    name = Math.floor(Math.random() * 100000),
     type = UrlbarUtils.PROVIDER_TYPE.PROFILE,
     priority = 0,
     addTimeout = 0,
@@ -613,7 +613,7 @@ class TestProvider extends UrlbarProvider {
     this._onCancel = onCancel;
   }
   get name() {
-    return this._name;
+    return "TestProvider" + this._name;
   }
   get type() {
     return this._type;

browser/components/urlbar/tests/browser/browser_updateRows.js

@@ -21,19 +21,19 @@ add_task(async function urlToTip() {
   ]);
 
   // Add a provider that returns a tip result when the search string is "testx".
+  let tipResult = new UrlbarResult(
+    UrlbarUtils.RESULT_TYPE.TIP,
+    UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
+    {
+      text: "This is a test tip.",
+      buttonText: "OK",
+      helpUrl: "http://example.com/",
+      type: "test",
+    }
+  );
+  tipResult.suggestedIndex = 1;
   let provider = new UrlbarTestUtils.TestProvider({
-    results: [
-      new UrlbarResult(
-        UrlbarUtils.RESULT_TYPE.TIP,
-        UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
-        {
-          text: "This is a test tip.",
-          buttonText: "OK",
-          helpUrl: "http://example.com/",
-          type: "test",
-        }
-      ),
-    ],
+    results: [tipResult],
   });
   provider.isActive = context => context.searchString == "testx";
   UrlbarProvidersManager.registerProvider(provider);
@@ -122,19 +122,19 @@ add_task(async function tipToURL() {
 
   // Add a provider that returns a tip result when the search string is "test"
   // or "testxx".
+  let tipResult = new UrlbarResult(
+    UrlbarUtils.RESULT_TYPE.TIP,
+    UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
+    {
+      text: "This is a test tip.",
+      buttonText: "OK",
+      helpUrl: "http://example.com/",
+      type: "test",
+    }
+  );
+  tipResult.suggestedIndex = 1;
   let provider = new UrlbarTestUtils.TestProvider({
-    results: [
-      new UrlbarResult(
-        UrlbarUtils.RESULT_TYPE.TIP,
-        UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
-        {
-          text: "This is a test tip.",
-          buttonText: "OK",
-          helpUrl: "http://example.com/",
-          type: "test",
-        }
-      ),
-    ],
+    results: [tipResult],
   });
   provider.isActive = context =>
     ["test", "testxx"].includes(context.searchString);

browser/components/urlbar/tests/unit/test_providersManager_filtering.js

@@ -359,30 +359,16 @@ add_task(async function test_filter_priority() {
   /**
    * A test provider.
    */
-  class TestProvider extends UrlbarProvider {
+  class TestProvider extends UrlbarTestUtils.TestProvider {
     constructor(priority, shouldBeInvoked, namePart = "") {
       super();
       this._priority = priority;
-      this._name = `Provider-${priority}` + namePart;
+      this._name = `${priority}` + namePart;
       this._shouldBeInvoked = shouldBeInvoked;
     }
-    get name() {
-      return this._name;
-    }
-    get type() {
-      return UrlbarUtils.PROVIDER_TYPE.PROFILE;
-    }
-    isActive(context) {
-      return true;
-    }
-    getPriority(context) {
-      return this._priority;
-    }
     async startQuery(context, add) {
       Assert.ok(this._shouldBeInvoked, `${this.name} was invoked`);
     }
-    cancelQuery(context) {}
-    pickResult(result) {}
   }
 
   // Test all possible orderings of the providers to make sure the logic that

browser/modules/test/browser/browser_UsageTelemetry_urlbar_tip.js

@@ -122,7 +122,7 @@ class TipProvider extends UrlbarProvider {
     this._results = results;
   }
   get name() {
-    return "TestTipProvider";
+    return "TestProviderTip";
   }
   get type() {
     return UrlbarUtils.PROVIDER_TYPE.PROFILE;