Refactor quick suggest
Categories
(Firefox :: Address Bar, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox108 | --- | verified |
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(6 files)
I'd like to refactor the quick suggest code in preparation for weather suggestions and other future work. Right now we have the following problems:
- UrlbarProviderQuickSuggest does too much. It has a lot of code that isn't related to being a urlbar provider. External consumers use it for things that aren't related to being a urlbar provider. It does a lot of the quick suggest feature initialization.
- UrlbarQuickSuggest does too much. It manages remote settings data but it's also the external quick suggest initialization point for BrowserGlue and it manages the onboarding dialog.
I want to split these two modules into the following modules:
- QuickSuggest: The core quick suggest module that is the external initialization point and manages the feature's status (enabled or not), blocked suggestions, impression caps, the onboarding dialog, and general consts and helpers.
- QuickSuggestRemoteSettingsClient: Manages quick suggest's remote settings data including configuration and suggestions. Like MerinoClient but for remote settings.
- UrlbarProviderQuickSuggest: This is only a urlbar provider and it records related engagement telemetry when a result is picked.
Assignee | ||
Comment 1•2 years ago
|
||
This does the following:
- Copies UrlbarProviderQuickSuggest.sys.mjs to a new QuickSuggest.sys.mjs file
- Removes everything related to UrlbarProvider from QuickSuggest
- Removes most everything not related to UrlbarProvider from
UrlbarProviderQuickSuggest - Updates consumers to use the new QuickSuggest module
Please see bug 1798595 for details.
Assignee | ||
Comment 2•2 years ago
|
||
This moves Nimbus exposure event recording out of UrlbarQuickSuggest in
preparation for making UrlbarQuickSuggest strictly related to remote settings.
Please see bug 1798595 for details.
Depends on D160982
Assignee | ||
Comment 3•2 years ago
|
||
This moves code related to the onboarding dialog out of UrlbarQuickSuggest in
preparation for making UrlbarQuickSuggest strictly related to remote settings.
Please see bug 1798595 for details.
Depends on D160983
Assignee | ||
Comment 4•2 years ago
|
||
This moves the help URL const out of UrlbarProviderQuickSuggest in order to make
UrlbarProviderQuickSuggest focused on being a urlbar provider.
It also changes how QuickSuggestTestUtils is initialized to be more similar to
MerinoTestUtils. There's no need for a separate init()
method when the
constructor will do. I'd like to make this change in a different revision but I
did it here because I want to include all changes to about:preferences in one
revision for easier review.
Please see bug 1798595 for details.
Depends on D160984
Assignee | ||
Comment 5•2 years ago
|
||
This does the following:
- Moves quick suggest initialization from UrlbarQuickSuggest to QuickSuggest
- Renames UrlbarQuickSuggest.sys.mjs to QuickSuggestRemoteSettingsClient.sys.mjs, so now this file is focused only on remote settings
- Makes QuickSuggest create an instance of QuickSuggestRemoteSettingsClient and keep it as
QuickSuggest.remoteSettings
- Moves latency telemetry from UrlbarProviderQuickSuggest into QuickSuggestRemoteSettingsClient
- Changes the ad hoc logger used in QuickSuggestRemoteSettingsClient to a proper urlbar-style logger
- Updates consumers to use
QuickSuggest.remoteSettings
instead of UrlbarQuickSuggest
Please see bug 1798595 for details.
Depends on D160985
Assignee | ||
Comment 6•2 years ago
|
||
This does the following:
- Get rid of
QUICK_SUGGEST_SOURCE
since it's only used in a couple of
places. Its simpler to use the string literals directly. - Set
source: "merino"
on Merino suggesions in the Merino client instead of
doing it in UrlbarProviderQuickSuggest, similar to how the remote settings
client setssource: "remote-settings"
- Export
ONBOARDING_CHOICE
andONBOARDING_URI
on the QuickSuggest object for
consistency with other consts - Remove unnecessary consts from QuickSuggestTestUtils that are already defined
on QuickSuggest
Please see bug 1798595 for details.
Depends on D160986
Comment 8•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/30fdaa2d3732
https://hg.mozilla.org/mozilla-central/rev/a5416bd918d9
https://hg.mozilla.org/mozilla-central/rev/ebf254bd4987
https://hg.mozilla.org/mozilla-central/rev/6c96c0f8d176
https://hg.mozilla.org/mozilla-central/rev/f7baffb8d5c4
https://hg.mozilla.org/mozilla-central/rev/66512226f1f1
Assignee | ||
Comment 9•2 years ago
|
||
For QA verification: This is a large refactor of the Firefox Suggest code, so as time allows please do some smoke testing to make sure it didn't cause any obvious regressions. The code is still covered by the same automated tests of course.
Comment 10•2 years ago
|
||
Thanks Drew for the tag.
Verified with a brief smoke testing that everything is in order: coverage here across win10, Ubuntu 22, Mac 12.
Description
•