1829081 - Make improvements to UrlbarProviderQuickSuggest in preparation for handling many types of suggestions

Status: VERIFIED FIXED

(Firefox :: Address Bar, task, P1)

Description

[Drew Willcoxon :adw]

UrlbarProviderQuickSuggest is too monolithic. It handles both remote settings and Merino suggestions. And as we start to add new types of suggestions to remote settings, it would need to handle them too. In summary, it has these problems:

• It's just getting too big:
  • The conditional that determines whether the provider should be active gets bigger with each new type of suggestion.
  • Other logic is suggestion specific too, so the class gets bigger and bigger with more per-suggestion logic.
  • If many suggestions need to be implemented using dynamic result types, UrlbarProviderQuickSuggest would need to support them all, either by adding the appropriate code inside the file itself or by splitting it into separate smaller helper files, which is similar to having per-suggestion providers but worse.
• RemoteSettingsClient was built with adM and expanded Wikipedia suggestions in mind. As we add new types of remote settings suggestions, we could either expand that class to allow fetching of all remote settings suggestions -- make it monolithic -- or we could move the fetching of specific suggestion types into smaller suggestion-specific classes, e.g., a Pocket provider that fetches Pocket suggestions. The second option seems desirable but it makes less sense with a monolithic UrlbarProviderQuickSuggest. In addition, different suggestion types will likely have different schemas and may even have different lookup/fetch methods, and it's not clear how to nicely handle that with a monolithic RemoteSettingsClient.
• As we plan for more and more new types of suggestions, the "quick suggest" name makes less and less sense. At some point these are just urlbar results like any others.
• It goes against the grain of the urlbar design, where many providers add different types of results and the muxer decides which ones to show. It duplicates that broader logic inside a single provider.

For Merino specifically, there should be a single provider since only one Merino call should be made per query. We don't want multiple Merino providers each making their own calls.

I'd like do the following:

• Factor out Merino results into UrlbarProviderMerino
• Factor out the current remote settings results into UrlbarProviderAdmWikipedia
• For each new suggestion type that uses remote settings, add a new provider that returns results for it from remote settings, e.g., UrlbarProviderPocket. If Merino also returns the suggestion type, add public methods to the provider so UrlbarProviderMerino can create the same types of UrlbarResults, dismiss results in the same way, record the same telemetry on engagement, use the same dynamic result type view updates, etc.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1829081 - Refactor UrlbarProviderQuickSuggest into UrlbarProviderAdmWikipedia and UrlbarProviderMerino.

This refactors UrlbarProviderQuickSuggest into UrlbarProviderAdmWikipedia and
UrlbarProviderMerino. Please see bug 1829081 for details.

Major changes:

• hg cp UrlbarProviderQuickSuggest UrlbarProviderAdmWikipedia
• hg cp UrlbarProviderQuickSuggest UrlbarProviderMerino
• hg rm UrlbarProviderQuickSuggest
• Since multiple providers now return "quick suggest" results, modify the muxer
  to pick the one with the largest score, and add score to payloads
• Remove the RESULT_SUBTYPE concept for quick suggest results. Replace it with
  result.payload.merinoProvider for Merino results. Remote settings results
  don't need a similar property because their parent UrlbarProvider is their
  provider/subtype.
• Use the Merino provider name directly in telemetry. It makes the code simpler
  and consistency is nice.
• If UrlbarProviderMerino doesn't recognize a suggestion type from Merino, it
  will ignore it
• Add a UrlbarResult.isQuickSuggest getter so we can easily tell whether a
  result is considered part of the quick suggest feature
• Update a ton of tests that use UrlbarProviderQuickSuggest

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1829081 - Add BaseQuickSuggestProvider and factor out quick suggest providers.

This refactors UrlbarProviderQuickSuggest into a base class called
BaseQuickSuggestProvider and the following new provider subclasses, one per type
of quick suggest result:

• UrlbarProviderAdmWikipedia: sponsored adM suggestions and non-sponsored
  Wikipedia (sometimes called "expanded Wikipedia") suggestions from Merino and
  remote settings
• UrlbarProviderDynamicWikipedia: dynamic Wikipedia suggestions from Merino
• UrlbarProviderNavigational: navigational suggestions ("top picks") from Merino

BaseQuickSuggestProvider handles things that most quick suggest results have in
common:

• Fetching from Merino
• Fetching from remote settings
• Recording legacy telemetry on engagement
• Dismissing results
• Updating impression stats (for impression caps)
• Filtering out suggestions that can't be shown
• Merino session management

Subclasses need to implement a few small getters and methods, which are called
at appropriate times by BaseQuickSuggestProvider. We can add new provider
subclasses that extend BaseQuickSuggestProvider to more easily handle new types
of quick suggest results in the future.

BaseQuickSuggestProvider uses a singleton MerinoClient instance. Promises
returned by MerinoClient.fetch() are cached based on query context. That way,
the first Merino fetch for a query will start the request, and later fetches
will only await the fetch promise.

Most of the other changes in this revision are consequences of this refactoring.
Summary of the major changes:

• hg cp UrlbarProviderQuickSuggest BaseQuickSuggestProvider
• hg cp UrlbarProviderQuickSuggest UrlbarProviderAdmWikipedia
• hg rm UrlbarProviderQuickSuggest
• Since multiple providers now return "quick suggest" results, modify the muxer
  to pick the one with the largest score, and add score to payloads
• Remove the RESULT_SUBTYPE concept for quick suggest results. It can be
  replaced by checking a result's providerName and also by
  result.payload.telemetryType.
• Properly document the ${source}_${type} result types in metrics.yaml (added
  in D174209)
• For Glean result types, replace "suggest_sponsor" with "merino_adm" and
  "rs_adm", and replace "suggest_non_sponsor" with "merino_wikipedia" and
  "rs_wikipedia"
• Add a UrlbarResult.isQuickSuggest getter so we can easily tell whether a
  result is considered part of the quick suggest feature. Also add the concept
  of provider.isQuickSuggestProvider so providers that are part of the quick
  suggest feature but that don't extend BaseQuickSuggestProvider can be easily
  detected as quick suggest providers. This revision uses that for the weather
  provider, since it's not a typical quick suggest provider.
• Update a ton of tests that use UrlbarProviderQuickSuggest
• Remove some code and a test (browser_quicksuggest_bestMatch.js) that aren't
  being used anymore and likely won't be in the future.
• Don't record the custom contextual services pings for dynamic Wikipedia
  results. These pings are only necessary for adM suggestions.
• Move two tasks for navigational suggestions to a new
  test_quicksuggest_navigational.js file.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1829081 - Make improvements to UrlbarProviderQuickSuggest in preparation for handling many types of suggestions.

This makes some changes to UrlbarProviderQuickSuggest that are much more modest
than my previously proposed refactorings in D176111 and D175998. After thinking
about it more and discussing it with @wstuckey and @daisuke, I'm not sure it's a
good idea to split UrlbarProviderQuickSuggest into multiple providers. In the
future, we will replace a lot of our desktop JS with a single cross-platform
Rust component that serves remote settings suggestions and possibly Merino
suggestions too. We should work with that in mind, and I don't think it makes a
lot of sense to have multiple urlbar providers all fetching from this one
component, even though it would make some things nicer, like being able to
isolate suggestion-specific UI code to each provider.

The main goal of this revision is to better prepare UrlbarProviderQuickSuggest
for many new types of suggestions:

• Add #makeResult(), which returns a new UrlbarResult appropriate for a
  given suggestion
• Add #makeDefaultResult(), which returns a result for suggestions that either
  don't need special handling or are unrecognized
• Remove the RESULT_SUBTYPE consts. The idea is that the client doesn't need
  to care too much about the types of results Merino returns, except when
  special handling is required (special UI, special telemetry, etc.)
• Add telemetryType to result payloads so that the result types recorded in
  Glean are clear and well defined. telemetryType is also used to tell the
  type of a result in general. For results that are served by Merino,
  telemetryType is the Merino provider name
• Streamline legacy telemetry handling a little, although hopefully we won't be
  doing too much legacy telemetry in the future

There are still open questions that this revision does not resolve, especially
the ability isolate suggestion-specific UI code in a nice way (dynamic result
types). I don't think this revision paints us into any corners.

Other changes:

• Properly document the ${source}_${type} result types in metrics.yaml (added
  in D174209)
• For Glean result types, replace "suggest_sponsor" with "merino_adm_sponsored"
  and "rs_adm_sponsored", and replace "suggest_non_sponsor" with
  "merino_adm_nonsponsored" and "rs_adm_nonsponsored". This is more consistent
  with the ${source}_${providerName} convention I'd like to establish.
• Remove code related to Nimbus exposures and a test
  (browser_quicksuggest_bestMatch.js). We aren't using Nimbus exposures anymore.
  We can always add it back if necessary.
• Don't record the custom contextual services pings for dynamic Wikipedia
  results. These pings are only necessary for adM suggestions.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02147a101bd9
Make improvements to UrlbarProviderQuickSuggest in preparation for handling many types of suggestions. r=daisuke

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/02147a101bd9

Comment 6

[Drew Willcoxon :adw]

For QA verification, there are no particular STR since this is a small refactoring of how Firefox Suggest suggestions are handled. We only want to make sure suggestions still generally appear as expected.

Comment 7

[Carmen Fat [:cfat] - Ecosystem QA]

We have verified this issue on the latest Firefox Nightly 115.0a1 (Build ID: 20230508214159) and Beta 114.0b1 (Build ID: 20230508175934), on Windows 10 x64, macOS 12.6.1, and Ubuntu 20.04 x64.

• We have verified that the following types of suggestions are triggered correctly: Weather, Best Bets and Dynamic Wikipedia.
• We have also verified that the telemetry probes are correctly displayed in Glean for each of the above mentioned suggestion type.