- Hooked the RustSharedRemoteSettingsService code to the Region module.
The new system updates the app context as the home region changes.
- Updated `RemoteSettingsService.updateConfig` to be sync rather than
async wrapped. It should execute quickly and I want to have
confidence that the config is actually updated when that function
returns.
- Updated/Simplified the SuggestBackendRust.sys.mjs testing code. Now
we just need to track if we should disable the Rust backend for the
tests or not.
- Updated `QuickSuggest._test_reinit` to wait for any unfinished
ingestion from `SuggestBackendRust`. This is needed now because
changing the remote settings service affects all active clients. If
there are unfinished ingestion tasks, then this creates a race
between those task finishing and the test finishing and uninitializing
the remote settings mock server.
Differential Revision: https://phabricator.services.mozilla.com/D248467
This converts `ExposureSuggestions` to `DynamicSuggestions`. Dynamic Rust
suggestions can be used to deliver hidden-exposure suggestions. I updated the
exposure test to show that, and I also added a new dynamic test.
Differential Revision: https://phabricator.services.mozilla.com/D244466
This hooks up desktop to the dismissal API in the Rust component [1] and removes
`BlockedSuggestions`.
The Rust dismissal API has two ways to dismiss a suggestion: by `Suggestion`
object and by dismissal key. We use the first one for Rust suggestions and the
second one for other suggestions, like Merino. A dismissal key is just an
arbitrary opaque string token stored in the dismissed-suggestions table in the
Rust component. It's up to consumers (like desktop) to use appropriate dismissal
keys for their non-Rust suggestions.
In order to retain the user's blocked digests that desktop has always recorded
in the `quicksuggest.blockedDigests` pref, I took advantage of dismissal keys by
migrating each digest to a dismissal key in the Rust component.
[1] See bug 1961412 and https://github.com/mozilla/application-services/pull/6714
Differential Revision: https://phabricator.services.mozilla.com/D246369
This includes some small fixes to desktop Suggest code and tests due to changes
in the `suggest` app-services component. It also incorporates D243740 by
@daisuke since it's required for vendoring.
This vendor replaces exposure suggestions with dynamic suggestions in the
`suggest` component, which breaks a few desktop tests. I disabled them for now
and will address them in bug 1961040.
Depends on D244233
Differential Revision: https://phabricator.services.mozilla.com/D245826
Please see the bug for the motivation.
This adds a new `SuggestFeature.primaryUserControlledPreference` getter that
returns the feature-specific pref that lets the user toggle on/off the feature.
That way we can add `QuickSuggest.clearDismissedSuggestions()`, which goes
through each feature and clears that pref, and `canClearDismissedSuggestions()`,
which goes through and checks whether there are any prefs that can be cleared.
I also added a couple of notification topics for dismissals that the settings UI
uses to update the disabled state of its Restore button.
All of this will let us more easily move to the Suggest Rust component's
dismissal API too, which we should sooner or later.
Depends on D244865
Differential Revision: https://phabricator.services.mozilla.com/D244866
This replaces `quickSuggestHideSettingsUI` with `quickSuggestSettingsUi`. Like
the old variable, we can show or hide all Suggest settings UI, and now we can
also hide UI that pertains only to Suggest online. This is necessary because as
we start to roll out Suggest to more regions, online (Merino) won't be available
for them initially.
Differential Revision: https://phabricator.services.mozilla.com/D238159
This replaces `quickSuggestHideSettingsUI` with `quickSuggestSettingsUi`. Like
the old variable, we can show or hide all Suggest settings UI, and now we can
also hide UI that pertains only to Suggest online. This is necessary because as
we start to roll out Suggest to more regions, online (Merino) won't be available
for them initially.
Differential Revision: https://phabricator.services.mozilla.com/D238159
This builds on D237170. Like that revision, I kept this one small and focused to
make the changes easier to see. There are opportunities for further cleanup that
I'll work on in follow-ups.
Depends on D237170
Differential Revision: https://phabricator.services.mozilla.com/D237312
This mostly just moves all Suggest-related things from `UrlbarPrefs` to
`QuickSuggest`. To keep the patch small and make review easier, I tried to keep
it a straight move rather than making improvements, renames, and other
unnecessary changes. There's opportunity to improve and simplify some things,
and I'd like to do that in follow-ups.
Depends on D237281
Differential Revision: https://phabricator.services.mozilla.com/D237170
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
