This is pretty much a straight factoring out with no substantive changes. I did
change some identifiers (variables, function names), especially so it's clear
that these utils can be used with any objects, not only suggestions.
The test is so long because it tries to check each conditional in the `best()`
helpers, e.g., when Merino returns bad responses, when locations are missing
values, etc.
Differential Revision: https://phabricator.services.mozilla.com/D229720
The problem in the bug is that the ML backend can return Yelp ML suggestions
even when `suggest.yelp` is false. The Yelp feature also doesn't discard them
when the pref is false.
This patch adds an `isMlIntentEnabled` getter to `BaseFeature`. It also adds
`mlIntent`, which the Yelp feature already defined but I forgot to add and
document in `BaseFeature`. So now `BaseFeature` and `SuggestBackendMl` handle
enabled/disabled suggestion types very similarly to the Rust backend.
Depends on D229050
Differential Revision: https://phabricator.services.mozilla.com/D229083
This adds some simplistic caching for weather suggestions, just like we have
simplistic caching for geolocation. That fixes the bug by avoiding latency when
calling Merino while you're typing a long query that keeps matching the same
suggestion again and again.
Instead of duplicating the caching logic from geolocation, I moved it into
`MerinoClient` so that both geolocation and weather can use it, and potentially
other features in the future.
Since caching interferes with tests that don't expect it, I added a simple way
to disable it during tests. `MerinoTestUtils` sets a static property on
`MerinoClient` called `_test_disableCache`. I added a test that specifically
tests the cache so we can be sure it works right.
Depends on D228755
Differential Revision: https://phabricator.services.mozilla.com/D228775
The error reported in the bug is thrown when `isURLEquivalentToResultURL()`
passes an undefined `originalUrl` to `rawSuggestionUrlMatches()` as the muxer is
comparing a history result to the weather result. Weather result payloads don't
include `originalUrl`, but `isURLEquivalentToResultURL()` assumes that all Rust
suggestions have it.
This patch modifies `isURLEquivalentToResultURL()` so it doesn't make that
assumption. Other places that access `originalUrl` also do not assume it's
defined, so I think this is just a mistake in `isURLEquivalentToResultURL()`.
Alternatively, we could make sure `originalUrl` is always defined on all
payloads, or maybe only Rust payloads, but that would require updating several
tests, and I like the idea that results only include it when necessary.
Differential Revision: https://phabricator.services.mozilla.com/D228742
This adds some logic on top of D226858 that computes the distance between the
client's geolocation coordinates and weather suggestions' coordinates. It works
really well. We still fall back to comparing the region and country if the
coordinates aren't present in the geolocation, but in practice, I'm not sure
Merino will ever return a geolocation without coordinates.
Also, I realized that we don't need to sort the suggestions to find the one with
the best region/country. We only need to do one loop through them.
Depends on D227271
Differential Revision: https://phabricator.services.mozilla.com/D227301
This adds a new Suggest backend for ML-based suggestions called
`SuggestBackendMl`. Before, with the JS and Rust backends, only one backend was
enabled at a time, but both the ML and Rust backends can be enabled at the same
time since we will want to serve suggestions from both for the foreseeable
future. Features can support ML suggestions by implementing the new
`BaseFeature.mlIntent` getter and handling ML suggestions in `makeResult()`.
Each feature can decide whether it supports ML suggestions and whether they
should be preferred over Rust suggestions.
I've updated the Yelp feature to hook into this, since Yelp suggestions are
supported by the ML model that Chidam is working on. If ML is enabled, then the
feature will only serve ML suggestions. I'm not sure if that's what we want long
term, but for now that will make it clear to people which backend is being used
while we develop this feature.
The `quickSuggestMlEnabled` variable/pref determines whether the ML backend is
enabled. The `yelpMlEnabled` variable/pref determines whether Yelp ML
suggestions are enabled. We can create similar variable/prefs for each feature
that supports ML suggestions so that they can be toggled independently of each
other.
Other changes:
Move the `is_sponsored` logic out of the Rust backend and into the provider.
Otherwise it would need to be duplicated in the ML backend too.
Depends on D224523
Differential Revision: https://phabricator.services.mozilla.com/D226736
This integrates Rust exposure suggestions with desktop. Exposure suggestions are
a part of the replacement for the existing potential exposures feature
(bug 1881875). When we want to test potential exposures in the future, we can
add new exposure suggestions to remote settings and tell the Rust component to
ingest them and return them in queries. When the Rust component returns an
exposure suggestion, desktop will record it in exposure telemetry. (The other
part of the replacement is keyword exposure telemetry in bug 1915507 D220501).
More details about the design of exposure suggestions here:
* Bug 1893086
* https://github.com/mozilla/application-services/pull/6343
The way desktop tells the Rust component about different types of exposure
suggestions is through the new "provider constraints" feature. `ingest()` and
`query()` can take a `SuggestionProviderConstraints` object that changes what's
ingested and queried. Therefore, we need to re-ingest exposure suggestions when
the provider constraints change. Right now, exposure suggestions are the only
kind of suggestions that use provider constraints.
Depends on D220359
Differential Revision: https://phabricator.services.mozilla.com/D220359
This keeps track of the enabled suggestion types that have been ingested while
the app is running. I call those types "fresh". In the code, they're stored in
the `#ingestedSuggestionTypes` set. Types that have not been ingested and types
that are disabled are called "stale". When a feature is updated, we update the
staleness bookkeeping in `#ingestedSuggestionTypes` and ingest only enabled
stale types.
This is similar to how each feature has an `#isEnabled` property. It's our
cached value of the last-known state that we can compare to the current state.
Depends on D219943
Differential Revision: https://phabricator.services.mozilla.com/D220352
Please see the bug for context. This changes how ingest works. Right now we
ingest all "default" suggestion types at once. This patch causes a suggestion
type to be ingested only when it's enabled.
Before this patch, there are two times ingest happens, on startup and when the
ingest timer fires.
With this patch, there are now a few times ingest happens:
* On startup (as before). `SuggestBackendRust` will ingest all registered and
enabled suggestion types.
* When the ingest timer fires (as before). Similar to startup, all registered
and enabled types will be ingested.
* When a suggestion type becomes enabled. I added this functionality to
`BaseFeature.update()` so that each feature gets this behavior automatically.
It's worth saying more about the startup case. On startup, all `BaseFeature`s'
initializations race each other. `SuggestBackendRust` is itself a `BaseFeature`,
so there's no guarantee it will be initialized before other `BaseFeature`s that
implement Rust suggestions. If any `BaseFeature`s are initialized before
`SuggestBackendRust`, they'll try to ingest but nothing will happen because
`SuggestBackendRust` won't be initialized yet. Once `SuggestBackendRust` is
initialized, it will ingest suggestions for all registered `BaseFeature`s that
already tried and failed to ingest. And after `SuggestBackendRust` initializes,
any `BaseFeature`s that are initialized later will be able to successfully
ingest.
Differential Revision: https://phabricator.services.mozilla.com/D216488
This makes sure `result.payload.originalUrl` is set correctly depending on Rust
vs. JS, and AMP vs. Wikipedia. The logic is getting a little messy due to all
the various combinations, but there's no way around it until we drop the JS
backend.
I tried setting `raw_url` (or `rawUrl`) on JS suggestions to match Rust
suggestions, and while that made the `AdmWikipedia` logic slightly nicer, it
either made the `UrlbarProviderQuickSuggest._canAddSuggestion()` logic a little
uglier because we need to also check `raw_url` for JS suggestions (snake_case),
or instead we could set `rawUrl` (camelCase) on JS suggestions just like Rust
suggestions, but every other property on JS suggestions uses snake_case.
This is *not* based on bug 1861540 because we need to uplift this, but I don't
want to uplift bug 1861540.
Differential Revision: https://phabricator.services.mozilla.com/D192124
This builds on the recent vendor that was done in bug 1859069 and D191004 in
particular.
The parameter to the Rust `SuggestStore.query()` function now takes an object
with a `providers` property, instead of separate boolean sponsored and
nonsponsored properties. `providers` is an array whose items are integers that
identify Rust providers (defined in [the RustSuggest bindings here](https://searchfox.org/mozilla-central/rev/c3c92500ef8ffab4fa53d4b2d5c5e2b49025e89b/toolkit/components/uniffi-bindgen-gecko-js/components/generated/RustSuggest.sys.mjs#953-958)).
`MockRustSuggest` in `QuickSuggestTestUtils.sys.mjs` is becoming kind of a pain
to keep updated. More tasks in `test_quicksuggest_pocket.js` should use Rust
than the ones I've updated here, but they don't because I would need to
(re)implement the prefix-matching behavior in `MockRustSuggest`. In a follow-up
I'd like to explore using a mock remote settings server instead so that the real
Rust component can be tested.
Differential Revision: https://phabricator.services.mozilla.com/D191351
This builds on the recent vendor that was done in bug 1859069 and D191004 in
particular.
The parameter to the Rust `SuggestStore.query()` function now takes an object
with a `providers` property, instead of separate boolean sponsored and
nonsponsored properties. `providers` is an array whose items are integers that
identify Rust providers (defined in [the RustSuggest bindings here](https://searchfox.org/mozilla-central/rev/c3c92500ef8ffab4fa53d4b2d5c5e2b49025e89b/toolkit/components/uniffi-bindgen-gecko-js/components/generated/RustSuggest.sys.mjs#953-958)).
`MockRustSuggest` in `QuickSuggestTestUtils.sys.mjs` is becoming kind of a pain
to keep updated. More tasks in `test_quicksuggest_pocket.js` should use Rust
than the ones I've updated here, but they don't because I would need to
(re)implement the prefix-matching behavior in `MockRustSuggest`. In a follow-up
I'd like to explore using a mock remote settings server instead so that the real
Rust component can be tested.
Differential Revision: https://phabricator.services.mozilla.com/D191351
This updates the engagement Glean test so it checks Rust results.
It also adds a getter to `SuggestBackendRust` that returns a promise that's
resolved once the backend's initialization is fully complete. Setting
`quicksuggest.rustEnabled` will either trigger the Rust backend to perform
initialization and ingestion, or it will trigger the JS backend to re-sync all
features. Either way, tests need to wait until all of that is done.
Differential Revision: https://phabricator.services.mozilla.com/D189452
This builds on D188681 and adds a new `BaseFeature` called `SuggestBackendRust`.
When `quickSuggestRustEnabled` is true, `UrlbarProviderQuickSuggest` will use
`SuggestBackendRust` to fetch remote settings suggestions; otherwise it will use
`SuggestBackendJs`.
The Rust component is already integrated into desktop Firefox (bug 1851256, bug
1851845), and it's exposed to JS via `RustSuggest.sys.mjs`. Currently it only
supports AMP (sponsored, a.k.a. adM) and Wikipedia (non-sponsored) suggestions.
It's possible to configure the path of the Sqlite file created by the Rust
component. This patch uses `suggest.sqlite` in the user's local profile (cache)
directory.
This is only the initial integration. I can think of a few follow-ups:
* Handle icons. In this patch, results from `SuggestBackendRust` don't have
icons at all. I have a WIP.
* Handle ingestion better. "Ingest" here means Firefox must tell the Rust
component to re-fetch suggestions from remote settings and rebuild its Sqlite
database. Unfortunately the Rust component doesn't keep the data updated by
itself, so we'll need to periodically tell the component to ingest. This patch
performs ingestion every time `SuggestBackendRust` is (re)enabled, which is a
good enough start.
* Maybe handle tests better. For now I modified the main quick suggest unit
test, test_quicksuggest.js, so it tests both backends. Other tests should
maybe be updated too, I'm not sure yet.
Depends on D188681
Differential Revision: https://phabricator.services.mozilla.com/D188684
This converts `QuickSuggestRemoteSettings` into a `BaseFeature` so that it can
be managed by `QuickSuggest` and easily enabled and disabled depending on
whether the new Rust component is enabled.
Summary of major changes:
* Rename `QuickSuggestRemoteSettings` to `SuggestBackendJs` and make it a
`BaseFeature`. In D188684 I'll also add a new `SuggestBackendRust` feature.
* Introduce a `quickSuggestRustEnabled` Nimbus variable. The JS backend will be
disabled if this variable is true. Nothing else uses the variable in this
patch but D188684 does.
* Move `DEFAULT_SUGGESTION_SCORE` to `UrlbarProviderQuickSuggest` and make the
provider ensure all suggestions have scores.
Differential Revision: https://phabricator.services.mozilla.com/D188681
This adds a `quickSuggestScoreMap` Nimbus variable that lets experiments
override suggestion scores. It maps from telemetry types to score values. For
example:
```
"quickSuggestScoreMap": {
"amo": 0.25,
"adm_sponsored": 0.3
}
```
In this example, addon suggestions will always have a score of 0.25, and
sponsored suggestions will always have a score of 0.3. Of course, different
branches within an experiment and different experiments can set different
scores.
While working on this, I saw we have a bug when we try to look up the
`BaseFeature` for a result. To do the lookup, we look up the result's
`telemetryType` in `FEATURE_NAMES_BY_TELEMETRY_TYPE`. That's a problem for `adm`
suggestions because the `telemetryType` will be either `adm_sponsored` or
`adm_nonsponsored`, but neither of those is present in
`FEATURE_NAMES_BY_TELEMETRY_TYPE` -- only `adm` is.
To fix it, I added back the `provider` property to result payloads that I
previously removed, and I added `BaseFeature.merinoProvider` so each feature can
specify its Merino provider. Then, `QuickSuggest` can build a map from Merino
provider names to features, allowing us to look up features without needing to
hardcode something like `FEATURE_NAMES_BY_TELEMETRY_TYPE` or
`FEATURE_NAMES_BY_MERINO_PROVIDER`.
Since I added back the `provider` property, I had to update a lot of tests. (As
a follow up, it would be nice to centralize the creation of expected result
objects in the test helper.)
I also added `BaseFeature.getSuggestionTelemetryType()` to help implement the
score map and to better formalize the idea that telemetry types are an important
property that all quick suggest results should include.
Differential Revision: https://phabricator.services.mozilla.com/D181709
This implements most parts of Pocket suggestions. They don't need any special UI
or a dynamic result type because they're only shown as the usual best match rows
or non-best match rows.
Still to do:
* Implement the "Show less frequently" behavior once we decide what the keywords
will be and how that will work.
* Implement the bottom "Mozilla Pocket" text inside the suggestion row once it's
finalized. We can use the same technique we use to show the "Sponsored" bottom
text for adM suggestions.
Other changes this makes:
* Replace the `type=bestmatch` attribute with an `bestmatch` attribute. That
lets best-match rows have a `type` too, in this case `"pocket"` (actually
either `"rs_pocket"` or `"merino_pocket"`, since I'm using the telemetry
result type).
* Improve how UrlbarProviderQuickSuggest delegates to individual features when
getting result commands, view updates, handling commands, etc., so that we
don't need to add new `case` statements for each new type of suggestion.
Differential Revision: https://phabricator.services.mozilla.com/D180059
This refactors quick suggest remote settings management so it will be easier to
support new types of remote settings suggestions. It builds on D177191.
Currently, RemoteSettingsClient only handles adM and Wikipedia suggestions. It
would be possible for it to support more suggestions types by adding a series of
`if` statements, one per suggestion type (to check if the suggestion type is
enabled), with each statement fetching a specific suggestion type's data.
However, that doesn't scale elegantly.
It would also be possible for RemoteSettingsClient to be agnostic about the
suggestions in remote settings. IOW it doesn't need to care that different types
of suggestions are stored in RS. It could treat them all as one big map from
keywords to suggestions. However, that would require uniformity and stability in
how suggestions are stored in RS, and since the Suggest project is not mature,
that's not the case right now, and it won't be the case any time soon.
Instead, this revision moves suggestion-specific logic from RemoteSettingsClient
to BaseFeature classes specific to each suggestion type. Each BaseFeature
registers itself with remote settings using the new QuickSuggestRemoteSettings
module, fetches its specific RS data, and builds whatever data structure is
appropriate for serving its suggestions. I expect most features to use a simple
map from keywords to suggestions, like adM/Wikipedia does, so I factored out the
map-related code into a new SuggestionsMap class. The weather feature is an
exception because only its keywords are stored in RS, not the suggestion itself.
UrlbarProviderQuickSuggest accesses remote settings suggestions by going through
QuickSuggestRemoteSettings. It only needs to make one call.
This design should work well when the cross-platform Rust component is ready.
We should only need to modify UrlbarProviderQuickSuggest so it fetches
suggestions from the Rust component instead of QuickSuggestRemoteSettings.
Depends on D177191
Differential Revision: https://phabricator.services.mozilla.com/D176779
'fromChrome' really meant "force tabs to open in the foreground", so let's
rename it accordingly.
This removes the attempt to document arguments for openUILinkIn.
I'll add documentation back on the end of this stack, for openLinkIn, when
various bits are reorganized anyway.
Differential Revision: https://phabricator.services.mozilla.com/D170384
'fromChrome' really meant "force tabs to open in the foreground", so let's
rename it accordingly.
This removes the attempt to document arguments for openUILinkIn.
I'll add documentation back on the end of this stack, for openLinkIn, when
various bits are reorganized anyway.
Depends on D170210
Differential Revision: https://phabricator.services.mozilla.com/D170384
This adds a weather feature to quick suggest. It periodically fetches a weather
suggestion from Merino. UrlbarProviderQuickSuggest shows the suggestion when the
search string is empty ("zero prefix").
The implementation of the UrlbarResult returned by UrlbarProviderQuickSuggest is
only temporary. Mandy is working on the final UI in
[SNT-323](https://mozilla-hub.atlassian.net/browse/SNT-323). Landing a temporary
implementation allows Mandy to base her patch on it and trigger real weather
suggestions from Merino to test her implementation against. It also lets people
on the team test weather suggestions without having to wait for the final UI to
land.
I added the following prefs and Nimbus variable to control the feature. It will
initially be rolled out in an experiment, so we need a Nimbus variable. In the
initial experiment, users will be able to dismiss the suggestion but not toggle
a checkbox in about:preferences.
* `weatherFeatureGate` - Nimbus variable that controls the whole feature
* `browser.urlbar.weather.featureGate` - Fallback pref for the Nimbus variable
* `browser.urlbar.suggest.weather` - When the feature gate pref is true, this
determines whether the suggestion should be shown. In a future patch, we'll
flip this to false when users dismiss the suggestion.
I set the fetch interval to 30 minutes. That seems reasonable considering that
the suggestion contains the current temperature and weather. Merino will set
caching headers appropriately so that Firefox won't actually make a new network
request if the previously fetched suggestion is new enough.
Depends on D161368
Differential Revision: https://phabricator.services.mozilla.com/D161410
This adds a weather feature to quick suggest. It periodically fetches a weather
suggestion from Merino. UrlbarProviderQuickSuggest shows the suggestion when the
search string is empty ("zero prefix").
The implementation of the UrlbarResult returned by UrlbarProviderQuickSuggest is
only temporary. Mandy is working on the final UI in
[SNT-323](https://mozilla-hub.atlassian.net/browse/SNT-323). Landing a temporary
implementation allows Mandy to base her patch on it and trigger real weather
suggestions from Merino to test her implementation against. It also lets people
on the team test weather suggestions without having to wait for the final UI to
land.
I added the following prefs and Nimbus variable to control the feature. It will
initially be rolled out in an experiment, so we need a Nimbus variable. In the
initial experiment, users will be able to dismiss the suggestion but not toggle
a checkbox in about:preferences.
* `weatherFeatureGate` - Nimbus variable that controls the whole feature
* `browser.urlbar.weather.featureGate` - Fallback pref for the Nimbus variable
* `browser.urlbar.suggest.weather` - When the feature gate pref is true, this
determines whether the suggestion should be shown. In a future patch, we'll
flip this to false when users dismiss the suggestion.
I set the fetch interval to 30 minutes. That seems reasonable considering that
the suggestion contains the current temperature and weather. Merino will set
caching headers appropriately so that Firefox won't actually make a new network
request if the previously fetched suggestion is new enough.
Depends on D161368
Differential Revision: https://phabricator.services.mozilla.com/D161410
This ports the remote settings client to `BaseFeature`.
The remote settings client is different from the other two features I ported in
this patch stack because it depends on preferences, not Nimbus variables:
`suggest.quicksuggest.sponsored` and `suggest.quicksuggest.nonsponsored`. To
support that, I modified `BaseFeature` so it can list the prefs it depends
on. When QuickSuggest observes a pref change, it checks whether it's a pref for
a feature, and if so, it updates it.
Please see bug 1799264 for details.
Depends on D161367
Differential Revision: https://phabricator.services.mozilla.com/D161368
This adds a new `BaseFeature` class that quick suggest features can extend.
`BaseFeature` encapsulates the logic for deciding whether a feature should be
enabled. Subclasses override a few methods and getters to describe their own
logic and to do initialization and uninitialization.
Any time a Nimbus variable (or a variable fallback pref) changes, QuickSuggest
iterates over each feature and enables or disables it appropriately.
As a first step, I ported impression caps to `BaseFeature`. Later patches in
this stack will port other features.
Please see bug 1799264 for details.
Differential Revision: https://phabricator.services.mozilla.com/D161366
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
Differential Revision: https://phabricator.services.mozilla.com/D160987
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
Differential Revision: https://phabricator.services.mozilla.com/D160986
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
Differential Revision: https://phabricator.services.mozilla.com/D160985
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
Differential Revision: https://phabricator.services.mozilla.com/D160984
