Closed Bug 1798595 Opened 27 days ago Closed 26 days ago

Refactor quick suggest

Categories

(Firefox :: Address Bar, task, P1)

task

Tracking

()

VERIFIED FIXED
108 Branch
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.

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.

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

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

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

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

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 sets source: "remote-settings"
  • Export ONBOARDING_CHOICE and ONBOARDING_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

Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30fdaa2d3732
Refactor quick suggest [Part 1]: Move core code out of UrlbarProviderQuickSuggest into a new QuickSuggest module. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/a5416bd918d9
Refactor quick suggest [Part 2]: Move Nimbus exposure event recording from UrlbarQuickSuggest to QuickSuggest. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/ebf254bd4987
Refactor quick suggest [Part 3]: Move onboarding dialog code from UrlbarQuickSuggest to QuickSuggest. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/6c96c0f8d176
Refactor quick suggest [Part 4]: Move help URL from UrlbarProviderQuickSuggest to QuickSuggest and change how the test utils are initialized. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/f7baffb8d5c4
Refactor quick suggest [Part 5]: Move remote settings code into QuickSuggestRemoteSettingsClient and initialization into QuickSuggest. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/66512226f1f1
Refactor quick suggest [Part 6]: Minor improvements. r=daisuke

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.

Flags: qe-verify+
Flags: in-testsuite-
Blocks: 1799264

Thanks Drew for the tag.
Verified with a brief smoke testing that everything is in order: coverage here across win10, Ubuntu 22, Mac 12.

Status: RESOLVED → VERIFIED
Flags: qe-verify+ → in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.