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 patch makes it unnecessary for most features to have to override
`shouldEnable`. Instead, the Suggest framework will check `enablingPreferences`
plus a new property called `additionalEnablingPredicate`. Summary:
* Include Nimbus variables in `enablingPreferences`
* Add a new property to `SuggestFeature` called `additionalEnablingPredicate`
* Make `shouldEnable` call `enablingPreferences` and
`additionalEnablingPredicate`
* Remove `shouldEnable` implementations on most features
Differential Revision: https://phabricator.services.mozilla.com/D235251
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 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 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 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 is hard to test, and since this patch only adds a couple defensive null
checks I don't think it really needs one anyway.
Differential Revision: https://phabricator.services.mozilla.com/D195930
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 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 converts addon suggestions from dynamic to standard results and removes
support for review stars. Other things worth noting:
* I removed `suggestion.is_top_pick` support because addons should always be top
picks. This lets us simplify the code and tests.
* This changes the overall icon box size for addons from 60px to 52px, since it
combines the icon rules for Pocket and addons. I checked with Josh, and he
says that's fine ([Slack](https://mozilla.slack.com/archives/C03LYL79G82/p1696471328852089?thread_ts=1696446640.986769&cid=C03LYL79G82))
* Currently the default `-webkit-line-clamp` is 3 and MDN and Pocket rows
override it by setting it to 2. I think the opposite makes more sense, i.e.
the default should be 2, since there are no row types that need it to be 3.
This lets me avoid adding a rule for addon rows. (`-webkit-line-clamp` was
added in D185247)
Depends on D190138
Differential Revision: https://phabricator.services.mozilla.com/D190145
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 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
It isn't possible to manually test this right now until we set a cap in the
remote settings config (bug 1852353), but the automated tests cover it.
Depends on D187830
Differential Revision: https://phabricator.services.mozilla.com/D187835
The problem is that showing the dismissal acknowledgment tip does not remove the
result from the query context, so when the view later opens and reuses the
cached query context, it still has the dismissed result in it.
At first I thought I should modify `UrlbarView.acknowledgeDismissal()` so it
either invalidates the context cache or removes the result from the cached
context, but I saw that dismissals without the acknowledgment tip do not have
this problem. That's because they go through `UrlbarController.removeResult()`,
which removes the result from the context and then notifies the view.
The real problem is that I implemented dismissal acknowledgments wrong. They
should start by going through `UrlbarController.removeResult()` too.
This patch updates all callers of `acknowledgeDismissal()` so they call
`removeResult()` instead. To signal that an acknowledgment should be shown, they
first set a `result.acknowledgeDismissalL10n` property. When the view is
notified that a result was removed, it shows the tip if this property is
present.
Differential Revision: https://phabricator.services.mozilla.com/D187830
This enables addon suggestions by default (treatment B, no stars) for all
Suggest users. As discussed on Slack, I did not remove the related prefs and
Nimbus variables in case something goes wrong and we need to disable the feature
ASAP. In that case, we can ship a Nimbus rollout to re-disable it. After a
release cycle or two without any problems, we can do a follow-up that removes
them. I also left the treatment A implementation. We can remove that in a
follow-up too.
Since I did leave the prefs and variables, I only had to change their default
values to enable the feature with treatment B.
One problem is that addon suggestions should be best matches, but we don't want
to set `bestMatch.enabled` to true by default. To work around that, I set the
related best-match properties on the `UrlbarResult` inside `AddonSuggestions`
instead of delegating it to the quick suggest provider. Product has requested
that we remove the "Top pick" checkbox from about:preferences, and at that time
we should also remove the `bestMatch.enabled` pref. In the future, results
should be best matches on a case-by-case basis.
This makes a few UI changes that aren't strictly related but I wanted to fix
since more users will now see these suggestions:
* Cache the "Firefox extension" group label to prevent pop-in.
* Align the URL and row title by baseline. Right now the URL is too high above
the title.
* Set `inline-margin-end` of the icon so it's symmetrical with the starting
margin/padding. The current value of 8px is hardcoded and too small.
Differential Revision: https://phabricator.services.mozilla.com/D187640
Each suggestion has "low" and "high confidence" keywords. When a high confidence
keyword is matched, the suggestion should be shown as a top pick, and otherwise
it should be shown as a normal Suggest result. High confidence keywords must be
matched in full, but low confidence keywords can be matched with prefixes
starting at the first word.
The low confidence matching behavior is the same as addon suggestions, so I
factored out that function into a new helper defined on `SuggestionsMap`.
I added a `full_keyword` property to the suggestions. It's not used yet but
we'll use it when we implement the final UI, which needs to show the full
keyword.
Differential Revision: https://phabricator.services.mozilla.com/D182580
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 implements the new required matching behavior, which isn't based on min
keyword length anymore. This is how it works:
* Use the full keywords in remote settings to generate keywords that contain the
first word plus each possible substring after the first word. For example if a
full keyword is "video download", then generate these keywords: "video",
"video ", "video d", "video do", etc. If a full keyword is only one word, then
use it as is. The keywords never change even when the user clicks "Show less
frequently". This is implemented in `onRemoteSettingsSync()`, and I modified
`SuggestionsMap.add()` to make it easy to generate new keywords from the
strings in `suggestion.keywords`.
* Keep track of the number of times the user clicked "Show less frequently" in
`showLessFrequentlyCount`.
* When a suggestion is fetched from the suggestions map, filter it out if the
search string isn't long enough given the `showLessFrequentlyCount`. This is
done in `makeResult()`.
Other changes:
* I made some of the private properties in `AddonSuggestions` public so that the
xpcshell test can easily use them. I think it's OK for them to be public.
* Added `show_less_frequently_cap` to the RS config object so that we can
specify a cap in RS as well as Nimbus.
* mv'ed test_quicksuggest_addResults.js to test_suggestionsMap.js, since I
modified this file. I should have done that back when I replaced `addResults()`
with `SuggestionsMap`.
* Fixed a bug in `SuggestionsMap.add()` where the same suggestion could be added
multiple times to the array stored in the map, if it had duplicate keywords.
Differential Revision: https://phabricator.services.mozilla.com/D179867
This patch was generated as follows:
Run:
`./mach esmify --imports . --prefix=toolkit/mozapps/extensions/AddonManager`
In the output there are linter/prettifier errors due to unused
XPCOMUtils or separate importESModule calls. These have been fixed
manually and verified with `./mach lint --outgoing`.
The `esmify` script also inserts many unwanted newlines around imports
that are broken on two lines due to length. Due to the number of these,
I fixed them programatically.
1. Create patch from the changes so far.
2. From the patch, delete all lines that consist of "+" (i.e. added blank line).
3. Reset the working dir and apply the revised patch.
4. Verify that the diff between step 1 and 3 looks reasonable.
5. Verify that this patch as a whole looks reasonable.
Commands:
```
git diff > rename.diff
:%g/^+$/d
git commit -va -m WIP-rename
git revert HEAD
git apply --recount rename.diff
git diff HEAD^ # and verify that the removed lines are ok.
git commit -va # one last review to verify correctness of whole patch.
git rebase -i HEAD~3 # drop the WIP + reverted commit, pick only the last.
```
`git apply` has the `--recount` option to force it to ignore mismatches
in line counts, which happens because we deleted added lines (^+$)
without fixing up the line counts in the file headers.
Differential Revision: https://phabricator.services.mozilla.com/D179874
