From 7e31642215d3114566d6e25c69c5ac57228ea057 Mon Sep 17 00:00:00 2001 From: Harry Twyford Date: Thu, 9 Jul 2020 05:02:49 +0000 Subject: [PATCH] Bug 1645521 - Part 2 - Allow for multiple heuristic providers and enable ProviderHeuristicFallback. r=adw Differential Revision: https://phabricator.services.mozilla.com/D80293 --- .../test/xpcshell/test_ext_urlbar.js | 4 +- .../urlbar/UrlbarMuxerUnifiedComplete.jsm | 84 +++++++++++++++---- .../UrlbarProviderHeuristicFallback.jsm | 2 +- .../urlbar/UrlbarProviderUnifiedComplete.jsm | 13 ++- .../urlbar/UrlbarProvidersManager.jsm | 73 ++++++++++++++-- browser/components/urlbar/UrlbarUtils.jsm | 4 +- .../urlbar/tests/UrlbarTestUtils.jsm | 4 +- .../tests/browser/browser_updateRows.js | 48 +++++------ .../unit/test_providersManager_filtering.js | 18 +--- .../browser_UsageTelemetry_urlbar_tip.js | 2 +- 10 files changed, 181 insertions(+), 71 deletions(-) diff --git a/browser/components/extensions/test/xpcshell/test_ext_urlbar.js b/browser/components/extensions/test/xpcshell/test_ext_urlbar.js index ce2e319f4b0e..3cb0511be79d 100644 --- a/browser/components/extensions/test/xpcshell/test_ext_urlbar.js +++ b/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, diff --git a/browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm b/browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm index 53af4a7da776..88d862e421aa 100644 --- a/browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm +++ b/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; } /** diff --git a/browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm b/browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm index 42e70f649cc9..78b4caab9d4f 100644 --- a/browser/components/urlbar/UrlbarProviderHeuristicFallback.jsm +++ b/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; } /** diff --git a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm index b980a164ce32..d24791f46dd2 100644 --- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm +++ b/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); diff --git a/browser/components/urlbar/UrlbarProvidersManager.jsm b/browser/components/urlbar/UrlbarProvidersManager.jsm index aab79a1a9702..4c607647f1b3 100644 --- a/browser/components/urlbar/UrlbarProvidersManager.jsm +++ b/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 diff --git a/browser/components/urlbar/UrlbarUtils.jsm b/browser/components/urlbar/UrlbarUtils.jsm index 36767a4b0ba6..409408369644 100644 --- a/browser/components/urlbar/UrlbarUtils.jsm +++ b/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; diff --git a/browser/components/urlbar/tests/UrlbarTestUtils.jsm b/browser/components/urlbar/tests/UrlbarTestUtils.jsm index 617dc4e76271..016b6cca4dd9 100644 --- a/browser/components/urlbar/tests/UrlbarTestUtils.jsm +++ b/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; diff --git a/browser/components/urlbar/tests/browser/browser_updateRows.js b/browser/components/urlbar/tests/browser/browser_updateRows.js index b1d6e983c7af..b764c166bf6d 100644 --- a/browser/components/urlbar/tests/browser/browser_updateRows.js +++ b/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); diff --git a/browser/components/urlbar/tests/unit/test_providersManager_filtering.js b/browser/components/urlbar/tests/unit/test_providersManager_filtering.js index cfe4a9cb674e..cd4ccf72e16c 100644 --- a/browser/components/urlbar/tests/unit/test_providersManager_filtering.js +++ b/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 diff --git a/browser/modules/test/browser/browser_UsageTelemetry_urlbar_tip.js b/browser/modules/test/browser/browser_UsageTelemetry_urlbar_tip.js index 331bb540449b..4aef2c892abb 100644 --- a/browser/modules/test/browser/browser_UsageTelemetry_urlbar_tip.js +++ b/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;