This reworks the fix to bug 1729776.
Currently Nimbus doesn't have a way to force the two suggestions prefs [1] on or
off. That might seem surprising since we've run experiments already. Initially
we defaulted the two prefs to true but we defaulted the separate feature-gate
pref [2] to false, and it was the feature-gate pref we controlled via Nimbus. At
some point we changed the defaults to false and then in Firefox flipped them to
true after showing the onboarding dialog. As a result we've never needed to
override the two suggestion prefs via Nimbus.
The problem now is that we default-enable offline (for US en users), so we set
all three prefs to true. For the online rollout, we need to keep the
feature-gate pref enabled but disable the suggestion prefs, and there's no way
to do that.
My first idea was to add new Nimbus variables to override the two suggestion
prefs. The prefs would keep their default true values but be overridden by
Nimbus. But that doesn't work because there's no way for Firefox to tell whether
the prefs are true because the user has opted in (overriding Nimbus) or because
they still have their default values. Setting the prefs to true on the user
branch doesn't have any effect because they're also true on the default branch.
Or maybe there's a way I don't know about to force them to true on the user
branch, but even if there were, it seems brittle to rely on a value being set on
the user branch to distinguish between the two cases. (This is a potential
problem for any prefs that are controlled by both Nimbus and the user. So far
the prefs we've been using via Nimbus have all been hidden feature-gate-type
things and implementation details.)
We already have a `quickSuggestScenario` variable. We currently use it only to
tell what we should send in the telemetry ping (bug 1729576) and whether some
parts of the prefs UI should be shown. This revision makes it much more
important by treating it as the source of truth for the user's scenario. It now
determines the default values of related prefs, including the two suggestions
prefs.
The logic is:
```
If quickSuggestScenario is non-null:
scenario = quickSuggestScenario
Else (e.g., there's no rollout):
If the user is US en:
scenario = offline
Else:
scenario = history
```
After determining the scenario, it's set it as
`browser.urlbar.quicksuggest.scenario` on the default branch. There's an
existing pref observer in UrlbarPrefs, and I added a case for this pref so that
when it's updated we also update all the other prefs that depend on the
scenario. This way when the pref is set -- either due to Nimbus update or by
changing it on about:config -- all the other prefs stay in sync.
I kept the default value of `browser.urlbar.quicksuggest.scenario` but removed
it as the fallback for `quickSuggestScenario`. If it still both had a default
and remained the fallback, then it would be impossible to tell when Nimbus is
trying to override it, because any fetch of the value from Nimbus would just
return the fallback pref's value if there is no override.
I considered instead removing the default value and keeping it as the fallback.
The drawback of that is that unenrollments would not take effect until restart.
I actually tried this approach first, and in tests, after mock experiments were
unenrolled, the pref values remained what they were when the experiment was
active.
It might also be possible to not have the `browser.urlbar.quicksuggest.scenario`
pref at all. We could call NimbusFeatures directly to get the scenario. However,
currently we cache and access Nimbus variables through UrlbarPrefs, as we do
with prefs, and I don't want to add an inconsistency.
This revision also fixes bug 1730596 since it was easy to do given that I needed
a way to prevent indirect recursive updates to the scenario, and I can use that
for bug 1730596 too (the `_updatingFirefoxSuggestScenario` bool).
[1] `browser.urlbar.suggest.quicksuggest` and `browser.urlbar.suggest.quicksuggest.sponsored`
[2] `browser.urlbar.quicksuggest.enabled`
Differential Revision: https://phabricator.services.mozilla.com/D125511
Summary of changes:
* Remove the Sponsored action text for Firefox Suggest sponsored results
* Instead show Sponsored below their titles
* Remove the Firefox Suggest action text for non-sponsored results
This uses the action text for "Sponsored" as we do now, but it wraps it below
the title.
The Figma spec (link in Jira ticket) shows URLs in Firefox Suggest results, the
same as history results. I asked Natalie about it, and that's not correct, or at
least we shouldn't implement that now. If we do need to show URLs, then this
approach is probably worse than D124519 because wrapping the action would also
force the URL on a new line since the URL comes after it.
Differential Revision: https://phabricator.services.mozilla.com/D124563
The Jira ticket (link in the bug) calls for two separate checkboxes for Firefox
Suggest results: a main checkbox plus a sponsored-suggestions checkbox. The
sponsored-suggestions checkbox is subordinate to the main checkbox, i.e., the
main checkbox has to be checked to turn on sponsored suggestions. This will
allow users to toggle sponsored suggestions separately from non-sponsored
suggestions. It's a change from the current situation where we have only one
pref and checkbox that control both sponsored and non-sponsored suggestions.
So part 1 of fixing this bug is to add a new pref for sponsored suggestions.
This revision keeps the current `suggest.quicksuggest` pref as the main pref and
adds a new `suggest.quicksuggest.sponsored` pref. I confirmed with Natalie that
we want to enable both prefs when the user opts in through the onboarding
dialog.
We currently record a telemetry event when `suggest.quicksuggest` is toggled. We
also want a similar event for the new pref, so this adds one.
The pref situation for Firefox Suggest is confusing but in summary:
* `browser.urlbar.quicksuggest.enabled`: The global toggle for the entire
Firefox Suggest rollout involving sponsored and non-sponsored suggestions, the
related telemetry and preferences UI, etc. This pref can be overridden by the
`quickSuggestEnabled` Nimbus variable. If false, neither sponsored nor
non-sponsored suggestions will be shown. If true, then we look at the
individual `suggest.quicksuggest` and `suggest.quicksuggest.sponsored` prefs.
* `browser.urlbar.suggest.quicksuggest`: Whether any Firefox Suggest results are
shown. This must be true to show both non-sponsored and sponsored results.
* `browser.urlbar.suggest.quicksuggest.sponsored`: Whether sponsored Firefox
Suggest results are shown. Both this pref and `suggest.quicksuggest` must be
true to show sponsored results.
Differential Revision: https://phabricator.services.mozilla.com/D124300
This uses `TelemetryStopwatch` to record the time between the `fetch` start and
its resolve. The new histogram is `FX_URLBAR_MERINO_LATENCY_MS`.
Depends on D124132
Differential Revision: https://phabricator.services.mozilla.com/D123993
This looks for a `score` property in suggestions from both Merino and remote
settings and picks the suggestion with the highest score. The score is intended
to be normalized -- i.e., in the range [0, 1] -- but currently it doesn't
matter. As discussed, I hardcoded a score of 0.2 for remote settings
suggestions.
When Merino returns multiple suggestions, currently we're just picking the first
one, but now we can pick the one with the highest score without any extra work,
so I went ahead and made sure we handle that now.
Also, this is robust against Merino not including a `score` in the response. In
that case we'll prefer the remote settings suggestion if there is one, and we'll
use the Merino suggestion if there's not.
Finally I added some new test tasks that aren't specifically related to scoring:
testing cases where either Merino or remote settings returns suggestions but not
both. I noticed I missed those cases earlier.
Differential Revision: https://phabricator.services.mozilla.com/D124132
This integrates a fetch to Merino in UrlbarProviderQuickSuggest. We continue to
do the remote settings fetch too. Per the Jira ticket, we should prefer the
Merino suggestion when both sources return one.
Each fetch is controlled by a new pref and Nimbus variable, so we can enable
them independently.
At first I started making a UrlbarProviderMerino class, but it's better to
modify UrlbarProviderQuickSuggest because everything besides the fetch source is
the same: We want to collect the same telemetry, have the same results and
payloads, etc.
Depends on D123852
Differential Revision: https://phabricator.services.mozilla.com/D123707
This adds preview localizations for remaining Firefox Suggest strings.
I removed support for `payload.helpTitle` and `sponsoredText`. The `helpTitle`
code comment says it's useful for experiments with hardcoded strings, but we're
not shipping experiments as extensions anymore, and in-tree experimental/
in-development features should use preview localizations AFAICT.
Covered by existing tests:
* browser/components/urlbar/tests/browser/browser_helpUrl.js
* browser/components/urlbar/tests/browser/browser_quickSuggest*
* browser/components/preferences/tests/browser_searchQuickSuggest.js
Depends on D122550
Differential Revision: https://phabricator.services.mozilla.com/D123032
This introduces a new `result.isSuggestedIndexRelativeToGroup` property. When
true, `result.suggestedIndex` is relative to the result's group.
We can get rid of some hardcoded quick-suggest things with this.
I considered a `relativeSuggestedIndex` property that would replace
`suggestedIndex`, but it would have made `muxer._addSuggestedIndexResults` a
little more complex because it would need to know whether it should use
`relativeSuggestedIndex` or `suggestedIndex`. `UrlbarView._rowCanUpdateToResult`
would also need to be updated since it checks for suggestedIndex results. But
there are trade-offs either way and I'm open to using `relativeSuggestedIndex`.
While working on this I noticed that we broke the position of quick suggest
results in bug 1662167. They're supposed to be last in the outer general group,
and currently the muxer is hardcoded to put them last in `GENERAL`, but after
that bug `INPUT_HISTORY` actually comes last. To fix that I added a new
`GENERAL_PARENT` group that's used for the outer general group. Quick suggest
results now use this group, combined with a relative `suggestedIndex`, to come
last. I wanted to rename `GENERAL` to `PLACES` or `BOOKMARKS_HISTORY` and use
`GENERAL` for the outer group, but we use `GENERAL` in a ton of places (tests
especially) and I didn't want to touch blame for it all. I'm open to better
names than `GENERAL_PARENT`. I considered `FIREFOX_SUGGEST` but I don't think
it's a good idea to tie it to specific branding that may change.
Differential Revision: https://phabricator.services.mozilla.com/D122799
This adds two new Nimbus urlbar variables:
* `quickSuggestSponsoredIndex`
* `quickSuggestNonSponsoredIndex`
They work similarly to `suggestedIndex` except they're relative to the general
bucket. We have bug 1710518 for finding a more general way to accomplish this.
I made a new test file instead of adding to browser_quicksuggest.js because I
wanted to test various indexes, and that's not easily doable with the current
test since it sets up a mock experiment right at the beginning, and AFAIK it's
not possible to change a (mock) experiment's variables or override them with
prefs. A new file specifically for this feature doesn't seem like a bad idea
anyway.
The new test copy-pastes some boilerplate from browser_quicksuggest.js. I
considered factoring it out, but I'd like to keep this patch simple since
product may want to uplift it, and in bug 1709741 I have a larger patch that
includes similar refactoring.
Differential Revision: https://phabricator.services.mozilla.com/D115437
This might be a little hacky because it builds special handling of quick suggest
results into the muxer instead of using some general solution. I have ideas for
general solutions, but they would all be larger than this patch, and we want to
uplift this to 89.
Differential Revision: https://phabricator.services.mozilla.com/D114669
With bug 1699227 and bug 1701193 fixed, we can now use -1 as the quick suggest
result's suggested index to make it stick to the bottom of the view without any
flickering.
Differential Revision: https://phabricator.services.mozilla.com/D110636
This adds event telemetry that's recorded when the
`browser.urlbar.suggest.quicksuggest` pref is toggled. This pref corresponds to
the checkbox in about:preferences#search labeled "Show suggested and sponsored
results in the address bar".
I used `contextservices.quicksuggest` as the event telemetry category name to be
similar to the `contextual.services.quicksuggest.*` scalars. Event names are
limited to 30 chars, so it couldn't be exactly the same.
This is based on my earlier revision for scalar telemetry in D106173.
Depends on D106173
Differential Revision: https://phabricator.services.mozilla.com/D106248
