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 error is harmless and originates in the `remote_settings` Rust component,
which the `suggest` Rust component relies on. The exact line that raises the
error is [1]. The equivalent Searchfox link of the vendored code is [2].
The problem is that `url` is `data:,#remote-settings-dummy/v1`, the dummy RS
server URL that's set by the xpcshell and browser test harnesses. `Url::join()`
doesn't work on "cannot-be-a-base" URLs like `data` URIs [3].
That's why this error is logged only in non-Suggest tests: Suggest remains
enabled for those tests, but of course they don't use the mock RS server that's
used for Suggest tests, so while they're running Suggest and `remote_settings`
are trying to use that dummy URL.
This patch allows the RS config to be set to null in `SuggestBackendRust`. In
that case, the backend won't try to create the Suggest store at all, effectively
disabling Rust suggestions. Rust suggestions were already effectively disabled
in non-Suggest tests due to this URL error -- that, plus the fact that there's
no RS server to serve Rust suggestions during non-Suggest tests.
[1] d3a5b91d44/components/remote_settings/src/config.rs (L80)
[2] https://searchfox.org/mozilla-central/rev/7fb746f0be47ce0135af7bca9fffdb5cd1c4b1d5/third_party/rust/remote_settings/src/config.rs#80
[3] https://docs.rs/url/latest/url/struct.Url.html#method.join
Differential Revision: https://phabricator.services.mozilla.com/D231195
This integrates Yelp suggestions with city/region detection from the Rust
component [1]. That includes both Rust Yelp suggestions and ML Yelp suggestions.
The reason for fixing this bug is to improve ML suggestions by reducing false
positives, but there's no reason we can't also do city/region matching for Rust
suggestions too.
With this patch, location detection and matching in Yelp suggestions now works
just like weather suggestions.
There are two benefits to city/region matching: (1) We can ignore queries with
"invalid" cities/regions, (2) we can do prefix matching, so for example if you
type "ramen in ne", we can match that to "ramen in New York, NY". Of course,
"invalid" doesn't always mean truly invalid. It just means we don't have a match
in our city/region database, which does not contain every city in the world.
This doesn't change how we handle queries that don't contain a location. In
those cases we'll still use geolocation.
This also builds on D229720 so that if multiple cities match the query, we'll
choose the one that best matches the user's location.
[1] [Documentation here](https://mozilla.github.io/application-services/book/rust-docs/suggest/struct.SuggestStore.html#method.fetch_geonames), [source here](c348fba48a/components/suggest/src/geoname.rs (L194))
Depends on D229720
Differential Revision: https://phabricator.services.mozilla.com/D229847
This makes two simple changes:
* Include the region in the suggestion title. The Merino team added a
`region_code` that we can use for this. (Right now we only need to worry about
U.S. and Canadian locations.)
* When the suggestion is selected, set the input value to the suggestion URL.
Currently the input becomes empty, which is strange IMO and isn't how other
suggestions behave.
Depends on D229090
Differential Revision: https://phabricator.services.mozilla.com/D229091
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
There were trivial failures in some urlbar tests due to the weather suggestions
changes that are the reason for this vendor, plus some ingest metrics changes in
this revision:
7e71b6a672
Differential Revision: https://phabricator.services.mozilla.com/D227271
This adds support for city-based weather suggestions from Rust, i.e., queries
that contain city and region names.
When Rust detects a weather query with a city, it sets three suggestion
properties: `city`, `region`, and `country`. All three properties will be set
because the Rust component knows which cities are in which regions and
countries, and it will only set the properties if it detects a city name. If a
name matches multiple cities and the query doesn't contain a region, Rust will
return multiple suggestions, one suggestion per city-region, and the suggestions
will be ordered by population size, largest to smallest.
Rust uses data from GeoNames to detect locations. We store GeoNames data in
remote settings and ingest it into the Rust DB, just like we do for suggestions
and keywords. This patch uses some mock GeoNames data in the test.
More info on the Rust part of this in the PR:
https://github.com/mozilla/application-services/pull/6406
For the Merino part, Merino now supports a few extra params for weather
suggestions: city, region, and country. So when the Rust component returns a
weather suggestion with those properties set, we can pass them to Merino to get
a weather suggestion for that location.
This patch adds a new `BaseFeature` method called `filterSuggestions()`. The
important thing about this method is that it's passed all the feature's
suggestions that were matched in the query. The feature can use this info to
determine which suggestion(s) to show. Right now, `Weather` uses it to discard
all weather suggestions except the first one, which means it will show the
suggestion with the largest city population (if the first suggestion has a
city). In the future, I'd like to explore better options like picking the
suggestion whose city name best matches the user's location.
Depends on D226215
Differential Revision: https://phabricator.services.mozilla.com/D226584
This replaces `MerinoTestUtils.WEATHER_RS_DATA` with
`QuickSuggestTestUtils.weatherRecord()`. Weather records now have an attachment
instead of storing their keywords inline. I also removed
`MerinoTestUtils.WEATHER_KEYWORD` because I think it's fine to use "weather"
directly everywhere, and that's simpler.
Some tests also need to be updated for the slight changes in keyword matching,
and some tasks don't make sense anymore.
Depends on D226214
Differential Revision: https://phabricator.services.mozilla.com/D226215
In addition to addressing the bug, this also adds some exposure tests to make
sure that they can be triggered in non-Suggest locales and that non-exposure
suggestions are not triggered.
Depends on D220501
Differential Revision: https://phabricator.services.mozilla.com/D221097
As part of bug 1914535, I'd like to move some Suggest xpcshell test helpers from
head.js to QuickSuggestTestUtils so that other tests outside the quicksuggest
directory can use them and also so that Suggest helpers are more centralized.
This doesn't make any functional changes.
Differential Revision: https://phabricator.services.mozilla.com/D219939
The main thing this does is add a task to the Fakespot engagement telemetry test
that triggers both a Fakespot and history result and then clicks the history
result. I want to make sure the Fakespot engagement event isn't incorrectly
recorded. (I thought it might be but it's not.)
This also has a bunch of changes from http to https. Lint made me change the
URLs in the Fakespot test to https, which was a problem for the AMP result since
`QuickSuggestTestUtils.ampRemoteSettings()` uses http URLs. So I went ahead and
updated that function plus the other ones and all the places that are affected.
Depends on D217780
Differential Revision: https://phabricator.services.mozilla.com/D217887
The Suggest Rust component currently stores the SQLite database in the
"local profile" directory, since it doesn't contain any user data.
Now that the component supports remembering dismissed suggestions,
we'll want to store the database in the "profile" directory instead,
since it'll contain persistent user data.
This commit prepares us for that future by specifying the profile
directory as the data path when we build the store. This commit also
adopts the `SuggestStoreBuilder#remoteSettings{Server, BucketName}()`
APIs.
Depends on D201774
Differential Revision: https://phabricator.services.mozilla.com/D203824
The enrollmentHelper was much more complicated than it needed to be. The
internal asynchrony that required awaiting an additional promise was fixed in
bug 1773583.
The returned cleanup function is no longer async, so unnecessary awaits have
been removed. This also applies to enrollWithFeatureConfig, as it is a wrapper
around enrollmentHelper.
Differential Revision: https://phabricator.services.mozilla.com/D212318
The problem is that `show_less_frequently` is misspelled. That doesn't cause the
first "Show less frequently" click to fail, but it does cause later clicks to
fail as long as the urlbar session remains ongoing (i.e., the view remains
open). That's because `TelemetryEvent` has [a hardcoded check](https://searchfox.org/mozilla-central/rev/d405168c4d3c0fb900a7354ae17bb34e939af996/browser/components/urlbar/UrlbarController.sys.mjs#962) for
`show_less_frequently` when it determines whether the session is still ongoing.
This check fails due to the misspelling, so `TelemetryEvent` thinks the session
has ended, which messes up later events and commands.
This makes a few related improvements:
* Support the `showLessFrequentlyCap` in the remote settings config for Yelp
* Add a "Show less frequently" task to the Yelp test. This uses the existing
`doShowLessFrequentlyTests()` helper, which requires the related getters
and methods to be public.
Differential Revision: https://phabricator.services.mozilla.com/D202356
This turned out to be a huge pain.
Many tests didn't work with the Rust backend. Generally I tried to keep tests
and tasks that truly need the JS backend, and I skip them when Rust is enabled.
I also added checks for `quickSuggestRustEnabled` so that tests don't assume the
Rust backend is enabled. I don't want to remove anything related to the JS
backend until the Rust backend is shipped in Release and we're confident we
don't need the JS backend anymore.
Also, browser tests didn't work properly because by the time
`QuickSuggestTestUtils.ensureQuickSuggestInit()` runs, the Rust backend has
already initialized, and `_test_remoteSettingsConfig` is not defined. To fix
that, I added a new `_test_setRemoteSettingsConfig()` function that recreates
the store with the new RS test config.
I also added some new helpers for generating remote settings data and for making
expected results in xpcshell tests.
Differential Revision: https://phabricator.services.mozilla.com/D201800
This bitrots D200075 but since we want to ship Rust Suggest in desktop in 124,
I'd like to vendor ASAP so we can start landing desktop patches, and we need to
fix this argument order to do that.
Depends on D200892
Differential Revision: https://phabricator.services.mozilla.com/D200893
As of 11amPT December 7, Contextual Services is now looking exclusively at
Glean-sent data for Firefox versions 116+. (See DSRE-1489)
Differential Revision: https://phabricator.services.mozilla.com/D195910
This modifies tests to use the server. There are a few important points to call
out:
This means tests are now using the real Rust component, and we need to make sure
the test RS data is valid and matches what Rust expects. For example, I had to
add `icon` properties to suggestions and set the `advertiser` to "Wikipedia" for
non-sponsored suggestions. Otherwise Rust hits an error on ingest. I also
removed some test cases because they tested behaviors that are impossible with
Rust, for example Pocket keywords that are duplicated in the high- and
low-confidence arrays.
We need to be careful to wait until Suggest is done syncing from remote settings
regardless of whether it's using the JS or Rust backend. I added a way to force
the backends to sync. That way, tests can force a sync, wait for it to finish,
and be sure that all sync activity is done.
A common pattern in tests is to call `ensureQuickSuggestInit()` and then set
Suggest-related prefs (or vice versa). This is a little problematic because both
`ensureQuickSuggestInit()` and setting prefs can cause Suggest to start a sync.
It's more problematic now that we're not mocking remote settings or Rust. So I
combined the two by adding a `prefs` param to `ensureQuickSuggestInit()`. That
way, tests can be sure that all syncing is done once that function returns.
Depends on D192037, D192124
Differential Revision: https://phabricator.services.mozilla.com/D192038
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 modifies tests to use the server. There are a few important points to call
out:
This means tests are now using the real Rust component, and we need to make sure
the test RS data is valid and matches what Rust expects. For example, I had to
add `icon` properties to suggestions and set the `advertiser` to "Wikipedia" for
non-sponsored suggestions. Otherwise Rust hits an error on ingest. I also
removed some test cases because they tested behaviors that are impossible with
Rust, for example Pocket keywords that are duplicated in the high- and
low-confidence arrays.
We need to be careful to wait until Suggest is done syncing from remote settings
regardless of whether it's using the JS or Rust backend. I added a way to force
the backends to sync. That way, tests can force a sync, wait for it to finish,
and be sure that all sync activity is done.
A common pattern in tests is to call `ensureQuickSuggestInit()` and then set
Suggest-related prefs (or vice versa). This is a little problematic because both
`ensureQuickSuggestInit()` and setting prefs can cause Suggest to start a sync.
It's more problematic now that we're not mocking remote settings or Rust. So I
combined the two by adding a `prefs` param to `ensureQuickSuggestInit()`. That
way, tests can be sure that all syncing is done once that function returns.
Depends on D192037
Differential Revision: https://phabricator.services.mozilla.com/D192038
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
As the TODO's say, I'd like to clean this up and add support for AMO and Pocket,
but right now let's just get the status quo working.
Depends on D191003
Differential Revision: https://phabricator.services.mozilla.com/D191004
This removes "best match" as its own separate Firefox Suggest feature. In the
future, whether or not a suggestion is a best match (a.k.a. top pick) will be
determined by relevant product requirements. I've confirmed this with Nive.
Here's a summary of changes:
* Removes prefs and Nimbus variables related to best match
* Removes the "Top pick" checkbox in about:preferences
* Removes support for the `best_match` quick suggest config property. This
property was removed from the config in remote settings a while ago.
* Removes legacy telemetry scalars related to best match. These scalars were
added years ago for the original best match experiment and before we started
using Glean. In the case of non-sponsored suggestions, the scalars have not
been recorded at all for some time. In the case of sponsored suggestions, they
can now be recorded again due to the recent addition of sponsored priority
suggestions, but they are superseded by Glean.
Differential Revision: https://phabricator.services.mozilla.com/D190516
This uses the update timer manager (`nsIUpdateTimerManager`) to periodically
ingest. This is the same mechanism the desktop remote settings client uses.
Please see the bug for more info on the update timer manager. I left some notes
there.
I chose the same 24-hour interval that the remote settings client uses. So
overall, this should give us pretty much the same update behavior we have with
remote settings in the JS backend.
Differential Revision: https://phabricator.services.mozilla.com/D190384
The new Rust implementation of Suggest stores icons for some suggestion types in
its Sqlite database, and it returns these icons to consumers as byte arrays. To
show these icons in the view quickly and without any overhead, we can create
`Blob` URLs from the arrays. Blob URLs need to be revoked when we're done with
them to avoid leaking the backing data.
This patch implements some simple lifetime management of blob URLs in the view.
The first time the view shows a result, it creates a blob URL for the result's
icon. The view caches the URL while it remains open, so as the user continues to
type and possibly match the same result many times, the view will use the same
blob URL each time. When the view closes, it revokes the URL. This seems like a
reasonable, natural lifetime for these URLs, and the implementation is simple.
Depends on D189452
Differential Revision: https://phabricator.services.mozilla.com/D189615
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
