With D234449 fixed, we can change `SuggestProvider.rustSuggestionTypes` (plural)
to `rustSuggestionType` (singular). We can remove `isRustSuggestionTypeEnabled`
too because it's assumed that if the feature itself is enabled then its Rust
suggestions are too. I don't expect we'll ever need to break that assumption,
but if we do, we can just add back `isRustSuggestionTypeEnabled` at that time.
Depends on D234449
Differential Revision: https://phabricator.services.mozilla.com/D234450
This makes two major changes:
Split `AdmWikipedia` into `AmpSuggestions` and `OfflineWikipediaSuggestions`. I
included "offline" in the name because Merino also serves Wikipedia suggestions,
but they're the so-called "dynamic Wikipedia" suggestions served from
Elasticsearch, and they have never been managed by a
`BaseFeature`/`SuggestFeature` in Firefox.
Send the `quick-suggest` ping only for AMP suggestions, in `AmpSuggestions`.
Stop sending it for offline Wikipedia suggestions. @bholley and I discussed this
on the data-review@mozilla.com list after I emailed it for the senstive data
review in D233752 [1]. We only use the ping for AMP suggestions, and Wikipedia
telemetry should follow the usual data opt-out. @nanj agreed the ping isn't
necessary for Wikipedia.
[1] https://groups.google.com/a/mozilla.com/g/data-review/c/BC2JKaXA7nA
Depends on D234440
Differential Revision: https://phabricator.services.mozilla.com/D234449
This adds `SuggestBackendMerino` and `QuickSuggest.backends`, an array that
contains all backends. So now we have Rust, ML, and Merino backends.
This also makes some changes to relevancy ranking (added in bug 1923187) because
that was done in `_fetchMerinoSuggestions()`, which this patch removes.
Previously, ranking was applied to all Merino suggestions at once. With this
patch, it's applied to each suggestion as it's visited by
`#filterAndSortSuggestions()`, which is now the right place to do it IMO. It
will be easy to expand it to non-Merino suggestions now if we want.
Depends on D232022
Differential Revision: https://phabricator.services.mozilla.com/D231863
This does several things, and I'll update D231863 so it builds on it:
Rename `BaseFeature` to `SuggestFeature` since only Suggest uses it and it's
pretty deeply integrated with `QuickSuggest` at this point.
Factor out all the suggestions/provider-specific methods into a new
`SuggestProvider` subclass. All features that manage suggestions are now
subclasses of `SuggestProvider` instead of `BaseFeature`.
Add a new subclass called `SuggestBackend` that codifies the expected backend
methods.
Rename `Weather` to `WeatherSuggestions` to be more consistent with the other
feature subclasses.
Update a bunch of comments so hopefully they're clearer and more accurate. I
also replaced the term "quick suggest" with "Suggest".
It doesn't make any substantive changes beyond the above.
Differential Revision: https://phabricator.services.mozilla.com/D232022
This is part 2 of 4, the main part. It removes the backend and the code in
`BaseFeature` and feature subclasses that hook into it. As a consequence it also
makes the other following changes:
* Remove `QuickSuggest.backend` (eventually I'd like to add a Merino backend and
then add a new `QuickSuggest.backends` list that has the Rust, Merino, and ML
backends)
* Add `QuickSuggest.config`
* Replace uses of `QuickSuggest.backend.config` and `QuickSuggest.jsBackend.config`
with `QuickSuggest.config`
* Remove the `quickSuggestRustEnabled` Nimbus variable
* Update most remaining tests so that they do not assume the JS backend exists
I left the `browser.urlbar.quicksuggest.rustEnabled` pref. I would like to
preserve the ability to toggle off Rust suggestions while keeping other parts of
Suggest enabled even if we don't need it now. That seems like a wise thing to do
and is similar to how both Merino and the ML backends can be toggled separately.
Depends on D231465
Differential Revision: https://phabricator.services.mozilla.com/D231466
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
