Bug 1852442 - Remove delay for heuristic result. r=mak

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

browser/components/urlbar/UrlbarEventBufferer.sys.mjs

@@ -27,7 +27,7 @@ const DEFERRED_KEY_CODES = new Set([
 const QUERY_STATUS = {
   UKNOWN: 0,
   RUNNING: 1,
-  RUNNING_GOT_RESULTS: 2,
+  RUNNING_GOT_ALL_HEURISTIC_RESULTS: 2,
   COMPLETE: 3,
 };
 
@@ -49,8 +49,8 @@ export class UrlbarEventBufferer {
   // Maximum time events can be deferred for. In automation providers can be
   // quite slow, thus we need a longer timeout to avoid intermittent failures.
   // Note: to avoid handling events too early, this timer should be larger than
-  // UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS.
-  static DEFERRING_TIMEOUT_MS = Cu.isInAutomation ? 1000 : 300;
+  // UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS.
+  static DEFERRING_TIMEOUT_MS = Cu.isInAutomation ? 1500 : 300;
 
   /**
    * Initialises the class.
@@ -111,7 +111,10 @@ export class UrlbarEventBufferer {
   }
 
   onQueryResults(queryContext) {
-    this._lastQuery.status = QUERY_STATUS.RUNNING_GOT_RESULTS;
+    if (queryContext.pendingHeuristicProviders.size) {
+      return;
+    }
+    this._lastQuery.status = QUERY_STATUS.RUNNING_GOT_ALL_HEURISTIC_RESULTS;
     // Ensure this runs after other results handling code.
     Services.tm.dispatchToMainThread(() => {
       this.replayDeferredEvents(true);
@@ -311,29 +314,31 @@ export class UrlbarEventBufferer {
    */
   isSafeToPlayDeferredEvent(event) {
     if (
-      this._lastQuery.status != QUERY_STATUS.RUNNING &&
-      this._lastQuery.status != QUERY_STATUS.RUNNING_GOT_RESULTS
+      this._lastQuery.status == QUERY_STATUS.COMPLETE ||
+      this._lastQuery.status == QUERY_STATUS.UKNOWN
     ) {
       // The view can't get any more results, so there's no need to further
       // defer events.
       return true;
     }
-    let waitingFirstResult = this._lastQuery.status == QUERY_STATUS.RUNNING;
+    let waitingHeuristicResults =
+      this._lastQuery.status == QUERY_STATUS.RUNNING;
     if (event.keyCode == KeyEvent.DOM_VK_RETURN) {
       // Check if we're waiting for providers that requested deferring.
       if (this.waitingDeferUserSelectionProviders) {
         return false;
       }
       // Play a deferred Enter if the heuristic result is not selected, or we
-      // are not waiting for the first results yet.
+      // are not waiting for heuristic results yet.
       let selectedResult = this.input.view.selectedResult;
       return (
-        (selectedResult && !selectedResult.heuristic) || !waitingFirstResult
+        (selectedResult && !selectedResult.heuristic) ||
+        !waitingHeuristicResults
       );
     }
 
     if (
-      waitingFirstResult ||
+      waitingHeuristicResults ||
       !this.input.view.isOpen ||
       this.waitingDeferUserSelectionProviders
     ) {

browser/components/urlbar/UrlbarProvidersManager.sys.mjs

@@ -120,10 +120,7 @@ class ProvidersManager {
     // To improve dataflow and reduce UI work, when a result is added we may notify
     // it to the controller after a delay, so that we can chunk results in that
     // timeframe into a single call. See _notifyResultsFromProvider for details.
-    // Note: to avoid handling events too early, the heuristic timer should be
-    // smaller than UrlbarEventBufferer.DEFERRING_TIMEOUT_MS.
-    this.CHUNK_HEURISTIC_RESULTS_DELAY_MS = 200;
-    this.CHUNK_OTHER_RESULTS_DELAY_MS = 16;
+    this.CHUNK_RESULTS_DELAY_MS = 16;
   }
 
   /**
@@ -480,13 +477,8 @@ class Query {
     let queryPromises = [];
     for (let provider of activeProviders) {
       // Track heuristic providers. later we'll use this Set to wait for them
-      // before returning results to the user. We skip the Omnibox provider
-      // because being implemented in an add-on we have no control over its
-      // performance characteristics.
-      if (
-        provider.type == lazy.UrlbarUtils.PROVIDER_TYPE.HEURISTIC &&
-        provider.name != "Omnibox"
-      ) {
+      // before returning results to the user.
+      if (provider.type == lazy.UrlbarUtils.PROVIDER_TYPE.HEURISTIC) {
         this.context.pendingHeuristicProviders.add(provider.name);
         queryPromises.push(
           startQuery(provider).finally(() => {
@@ -626,35 +618,19 @@ class Query {
 
   _notifyResultsFromProvider(provider) {
     // We use a timer to reduce UI flicker, by adding results in chunks.
-    if (!this._chunkTimer && this.context.pendingHeuristicProviders.size) {
-      // This is the first time we see a result, and some heuristic providers
-      // are still pending. We start a longer "heuristic" timeout, because we
-      // don't want to surprise the user with an unexpected default action (e.g.
-      // searching instead of handling a bookmark keyword).
-      // Since heuristic providers return results pretty quickly, this timer
-      // will often be skipped early.
-      // Note that if the heuristic timer elapses, we may still cause an
-      // imperfect default action, but that's still better than looking stale.
-      this._chunkTimer = new lazy.SkippableTimer({
-        name: "heuristic",
-        callback: () => this._notifyResults(),
-        time: UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS,
-        logger: provider.logger,
-      });
-    } else if (!this._chunkTimer || this._chunkTimer.done) {
+    if (!this._chunkTimer || this._chunkTimer.done) {
       // Either there's no heuristic provider pending at all, or the previous
       // timer is done, but we're still getting results. Start a short timer
       // to chunk remaining results.
       this._chunkTimer = new lazy.SkippableTimer({
         name: "chunking",
         callback: () => this._notifyResults(),
-        time: UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS,
+        time: UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS,
         logger: provider.logger,
       });
     } else if (
-      this._chunkTimer.name == "heuristic" &&
-      !this._chunkTimer.done &&
-      !this.context.pendingHeuristicProviders.size
+      !this.context.pendingHeuristicProviders.size &&
+      provider.type == lazy.UrlbarUtils.PROVIDER_TYPE.HEURISTIC
     ) {
       // All the active heuristic providers have returned results, we can skip
       // the heuristic chunk timer and start showing results immediately.
@@ -666,7 +642,6 @@ class Query {
 
   _notifyResults() {
     this.muxer.sort(this.context, this.unsortedResults);
-
     // We don't want to notify consumers if there are no results since they
     // generally expect at least one result when notified, so bail, but only
     // after nulling out the chunk timer above so that it will be restarted

browser/components/urlbar/tests/browser/browser.toml

@@ -550,6 +550,8 @@ support-files = [
 ]
 fail-if = ["a11y_checks"] # Bug 1854660 clicked element may not be focusable and/or labeled
 
+["browser_slow_heuristic.js"]
+
 ["browser_speculative_connect.js"]
 support-files = [
   "searchSuggestionEngine2.xml",

browser/components/urlbar/tests/browser/browser_slow_heuristic.js

@@ -0,0 +1,84 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Test that slow heuristic results are still waited for on selection.
+
+"use strict";
+
+add_task(async function test_slow_heuristic() {
+  // Must be between CHUNK_RESULTS_DELAY_MS and DEFERRING_TIMEOUT_MS
+  let timeout = 150;
+  Assert.greater(timeout, UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS);
+  Assert.greater(UrlbarEventBufferer.DEFERRING_TIMEOUT_MS, timeout);
+
+  // First, add a provider that adds a heuristic result on a delay.
+  let heuristicResult = new UrlbarResult(
+    UrlbarUtils.RESULT_TYPE.URL,
+    UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
+    { url: "https://example.com/" }
+  );
+  heuristicResult.heuristic = true;
+  let heuristicProvider = new UrlbarTestUtils.TestProvider({
+    results: [heuristicResult],
+    name: "heuristicProvider",
+    priority: Infinity,
+    addTimeout: timeout,
+  });
+  UrlbarProvidersManager.registerProvider(heuristicProvider);
+  registerCleanupFunction(() => {
+    UrlbarProvidersManager.unregisterProvider(heuristicProvider);
+  });
+
+  // Do a search without waiting for a result.
+  const win = await BrowserTestUtils.openNewBrowserWindow();
+  let promiseLoaded = BrowserTestUtils.browserLoaded(
+    win.gBrowser.selectedBrowser
+  );
+
+  win.gURLBar.focus();
+  EventUtils.sendString("test", win);
+  EventUtils.synthesizeKey("KEY_Enter", {}, win);
+  await promiseLoaded;
+
+  await UrlbarTestUtils.promisePopupClose(win);
+  await BrowserTestUtils.closeWindow(win);
+});
+
+add_task(async function test_fast_heuristic() {
+  let longTimeoutMs = 1000000;
+  let originalHeuristicTimeout = UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS;
+  UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS = longTimeoutMs;
+  registerCleanupFunction(() => {
+    UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS = originalHeuristicTimeout;
+  });
+
+  // Add a fast heuristic provider.
+  let heuristicResult = new UrlbarResult(
+    UrlbarUtils.RESULT_TYPE.URL,
+    UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL,
+    { url: "https://example.com/" }
+  );
+  heuristicResult.heuristic = true;
+  let heuristicProvider = new UrlbarTestUtils.TestProvider({
+    results: [heuristicResult],
+    name: "heuristicProvider",
+    priority: Infinity,
+  });
+  UrlbarProvidersManager.registerProvider(heuristicProvider);
+  registerCleanupFunction(() => {
+    UrlbarProvidersManager.unregisterProvider(heuristicProvider);
+  });
+
+  // Do a search.
+  const win = await BrowserTestUtils.openNewBrowserWindow();
+
+  let startTime = Cu.now();
+  Assert.greater(
+    longTimeoutMs,
+    Cu.now() - startTime,
+    "Heuristic result is returned faster than CHUNK_RESULTS_DELAY_MS"
+  );
+
+  await UrlbarTestUtils.promisePopupClose(win);
+  await BrowserTestUtils.closeWindow(win);
+});

browser/components/urlbar/tests/browser/head.js

@@ -14,6 +14,7 @@ ChromeUtils.defineESModuleGetters(this, {
   SearchUtils: "resource://gre/modules/SearchUtils.sys.mjs",
   TelemetryTestUtils: "resource://testing-common/TelemetryTestUtils.sys.mjs",
   UrlbarController: "resource:///modules/UrlbarController.sys.mjs",
+  UrlbarEventBufferer: "resource:///modules/UrlbarEventBufferer.sys.mjs",
   UrlbarQueryContext: "resource:///modules/UrlbarUtils.sys.mjs",
   UrlbarResult: "resource:///modules/UrlbarResult.sys.mjs",
   UrlbarSearchUtils: "resource:///modules/UrlbarSearchUtils.sys.mjs",

browser/components/urlbar/tests/engagementTelemetry/browser/browser_glean_telemetry_engagement_edge_cases.js

@@ -86,12 +86,8 @@ add_task(async function engagement_before_showing_results() {
   });
 
   // Increase chunk delays to delay the call to notifyResults.
-  let originalHeuristicTimeout =
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS = 1000000;
-  let originalOtherTimeout =
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = 1000000;
+  let originalChunkTimeout = UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS;
+  UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS = 1000000;
 
   // Add a provider that waits forever in startQuery() to avoid fireing
   // heuristicProviderTimer.
@@ -103,9 +99,7 @@ add_task(async function engagement_before_showing_results() {
   const cleanup = () => {
     UrlbarProvidersManager.unregisterProvider(noResponseProvider);
     UrlbarProvidersManager.unregisterProvider(anotherHeuristicProvider);
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS =
-      originalHeuristicTimeout;
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = originalOtherTimeout;
+    UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS = originalChunkTimeout;
   };
   registerCleanupFunction(cleanup);
 

browser/components/urlbar/tests/quicksuggest/browser/browser_telemetry_impressionEdgeCases.js

@@ -387,16 +387,10 @@ async function doEngagementWithoutAddingResultToView(
   // Set the timeout of the chunk timer to a really high value so that it will
   // not fire. The view updates when the timer fires, which we specifically want
   // to avoid here.
-  let originalHeuristicTimeout =
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS = 30000;
-  let originalOtherTimeout =
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS;
-  UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = 30000;
+  let originalChunkTimeout = UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS;
+  UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS = 30000;
   const cleanup = () => {
-    UrlbarProvidersManager.CHUNK_HEURISTIC_RESULTS_DELAY_MS =
-      originalHeuristicTimeout;
-    UrlbarProvidersManager.CHUNK_OTHER_RESULTS_DELAY_MS = originalOtherTimeout;
+    UrlbarProvidersManager.CHUNK_RESULTS_DELAY_MS = originalChunkTimeout;
   };
   registerCleanupFunction(cleanup);