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 moves l10n strings related to AMP and Wikipedia suggestions out of
`enUS-searchFeatures.ftl` into the appropriate l10n files in preparation for AMP
and Wikipedia in non-U.S. regions. These strings include:
* The result menu item strings, Dismiss and Manage
* Relevant settings UI strings
`urlbar-result-menu-learn-more-about-firefox-suggest` isn't actually used by AMP
and Wikipedia right now, but it was in the past, and there have been recent
discussions about maybe including it again as Suggest expands outside the U.S.
So I moved it too in case we need it with short notice.
There are other Suggest strings that this patch does not move, in particular:
* `-firefox-suggest-brand-name` is already exposed to localizers
* The "Sponsored" label at the bottom of AMP urlbar rows is already exposed to
localizers as `urlbar-result-action-sponsored`
* Strings for the online toggle switch in the settings UI ("Improve the Firefox
Suggest experience") aren't needed right now because online Suggest (Merino)
won't be available outside the U.S. in the near future.
I changed the ID of the Dismiss string so it doesn't include "firefox-suggest".
Several non-Suggest urlbar results use this string too, and it doesn't actually
include the phrase "Firefox Suggest" anyway.
I also made the view default to this string so that dismissable urlbar results
don't need to specify it, similar to how it defaults to strings for "Learn more"
and Manage.
Depends on D238847
Differential Revision: https://phabricator.services.mozilla.com/D239213
This makes a few changes:
Make sure we always set `result.payload.isSponsored` and base it on
`SuggestProvider.isSuggestionSponsored()` so that it's really clear. With that
change, we don't need to set `suggestion.is_sponsored` anymore. (I'm trying to
stop modifying suggestion objects so much because it's hard to follow.)
Don't call `feature.makeResult()` if the feature is disabled. I'm kind of
surprised we don't do this already, but it's always worked out in the end due to
a few reasons: (1) Some `feature.makeResult()` implementations return null if
their prefs are disabled (effectively duplicating their `shouldEnable` logic),
(2) we don't query the Rust component for disabled suggestions, and (3)
`#canAddSuggestion()` returns null if sponsored/nonsponsored suggestions are
disabled.
Replace `#canAddSuggestion()` with `#canAddResult()`. The logic can be
simplified and is easier to follow if we always deal with results instead of
suggestions. Examples: (1) We can check `result.payload.isSponsored` instead of
having to also set and check `suggestion.is_sponsored`. (2) When checking for
blocked suggestions, we can check `result.payload.originalUrl` instead of
leaking `suggestion.rawUrl` from the Rust component.
Differential Revision: https://phabricator.services.mozilla.com/D235389
I don't think we have any tests specifically for Merino suggestions that don't
correspond to Suggest features, i.e., suggestions with a Merino provider that is
technically unrecognized by Firefox. Dynamic Wikipedia suggestions and
navigational suggestions (a.k.a. top sites) are two examples. So I added tasks
for both sponsored and nonsponsored suggestions to make sure they're disabled
when the relevant pref is disabled, in addition to testing this bug itself.
Differential Revision: https://phabricator.services.mozilla.com/D234787
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 makes a number of changes. The main point is to fix the bug, and I did some
refactoring to accomplish that. Summary of changes:
Move submission of the `quick-suggest` ping from `UrlbarProviderQuickSuggest` to
`AdmWikipedia`.
Add `SuggestProvider.onImpression()` and `onEngagement()`.
`UrlbarProviderQuickSuggest` forwards its own impression and engagement method
calls to the methods on the appropriate Suggest features. This is how
`AdmWikipedia` knows when to submit the ping.
Add a `details` param to `UrlbarProvider.onImpression()`, just like
`onEngagement()` already has. This is necessary because
`AdmWikipedia.onImpression()` needs to know whether the result was clicked since
that info is part of the impression ping.
Replace all `SuggestFeature.handleCommand()` implementations with
`onEngagement()`.
Remove `UrlbarProviderQuickSuggest.#dismissResult()`. The only suggestion types
that relied on it were AMP and Wikipedia. I replaced it with dismissal handling
in `AdmWikipedia.onEngagement()`.
Improve the related tests in a few ways: (1) Make the ping check more rigorous
(in `assertQuickSuggestPing()` in `head.js`), (2) simplify expected ping objects
(no more `type` and `payload` properties, just the properties that are expected
in the ping), (3) make `browser_telemetry_gleanEmptyStrings.js` more rigorous,
(4) factor out a helper function to trigger commands in xpcshell tests.
Differential Revision: https://phabricator.services.mozilla.com/D233980
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 3 of 4. It removes the `"remote-settings"` source used internally
and its `rs_` counterpart in telemetry result types.
Depends on D231466
Differential Revision: https://phabricator.services.mozilla.com/D231467
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 also downgrades and removes some logging that I myself added that hasn't
proven to be useful. I reworded messages I wrote in a few places too.
I didn't touch `SuggestBackendJs.sys.mjs` because it's unused and will likely be
removed soon (bug 1932502).
Depends on D229847
Differential Revision: https://phabricator.services.mozilla.com/D229868
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 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 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
Please see the bug for details.
This makes some changes to search strings and URLs in the tests that might look
unnecessary. They're due to switching to the QuickSuggestTestUtils helpers in
the head files instead of hardcoding the suggestions data. It's possible to
force the helpers to use the existing search strings and URLs that are used in
these tests, but it's good to be consistent with other tests that use these
helpers because it makes everything easier to understand.
Depends on D220352
Differential Revision: https://phabricator.services.mozilla.com/D219943
Please see the bug for details.
This makes some changes to search strings and URLs in the tests that might look
unnecessary. They're due to switching to the QuickSuggestTestUtils helpers in
the head files instead of hardcoding the suggestions data. It's possible to
force the helpers to use the existing search strings and URLs that are used in
these tests, but it's good to be consistent with other tests that use these
helpers because it makes everything easier to understand.
Depends on D219942
Differential Revision: https://phabricator.services.mozilla.com/D219943
