Make improvements to UrlbarProviderQuickSuggest in preparation for handling many types of suggestions
Categories
(Firefox :: Address Bar, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox114 | --- | verified |
People
(Reporter: adw, Assigned: adw)
References
(Regressed 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
text/x-phabricator-request
|
Details |
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.
Assignee | ||
Comment 1•2 years ago
|
||
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 addscore
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
Assignee | ||
Comment 2•2 years ago
|
||
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'sproviderName
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
ofprovider.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.
Assignee | ||
Comment 3•2 years ago
|
||
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 newUrlbarResult
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.
Updated•2 years ago
|
Updated•2 years ago
|
Comment 5•2 years ago
|
||
bugherder |
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 6•2 years ago
|
||
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•2 years ago
|
||
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.
Description
•