Please see the bug for context and motivation.
I found a problem with the `terminal` calculation I added in bug 1915507
D220501. Basing it on query contexts isn't quite right. Ideally, for visible
results, `terminal` will be true iff the result is in `view.visibleResults` at
the end of the session, i.e., iff the result is recorded in the engagement or
abandonment event. Anything else would be confusing.
During the last query in a session, the view will be full of stale results from
previous queries. As the view updates itself, it will mark these rows as stale
and call `addExposure()` for them, passing in the current (final) query context.
It's not incorrect to add exposures since these rows are in fact visible. And if
the final query is canceled, they'll remain visible and actually be terminal
results. However, if the final query finishes without being canceled, the view
will remove these rows, and they won't be terminal at all, but since the view
called `addExposure()` for them with the final query context, `TelemetryEvent`
will think they are.
For visible exposures, the `terminal` calculation needs to be based on the
actual visible rows at the end of the session, same as how the results in the
engagement/abandonment events are determined. For hidden exposures, we can just
use `queryContext.results`.
Differential Revision: https://phabricator.services.mozilla.com/D221912
This replaces the current potential exposures implementation with "keyword
exposures." The current implementation is based on a keyword list defined in
Nimbus. Keyword exposures improve on the following drawbacks to that approach:
* Potential exposures can't be recorded for existing result types without
duplicating their keywords in the Nimbus list. The ability to record potential
exposures for existing types is an idea from DS (Dave).
* They're unrelated to `exposure` telemetry, so we don't get an `exposure` event
for them in the main ping, making it hard to correlate them with engagements
and abandonments. This is another drawback pointed out by DS (Dave). (By
design, we don't want to correlate individual keywords with
engagements/abandonments for privacy reasons, but we do want to know whether a
potential exposure was triggered during an engagement/abandonment without
knowing the matching keywords.)
* Storing the keywords in Nimbus means the Nimbus recipe is at least as large as
the keyword list. It's probably not a great idea to put thousands of keywords
in the recipe JSON.
* The keyword-matching strategy is simplistic exact matching. It can't do more
sophisticated strategies used for real results, like how we recently started
using FTS in Suggest.
Keyword exposures as implemented by this revision improve on all that. Summary:
* Keyword exposures are implemented on top of existing `exposure` telemetry.
They can be enabled for any result type, and they are always "in addition to"
`exposure` telemetry.
* They're enabled with a new bool Nimbus variable. When true, they're recorded
for all results that trigger `exposure` telemetry.
* They work with the new `Exposure` Rust suggestions (bug 1915317, bug 1893086,
result type "rust_exposure"). In combination, `Exposure` suggestions and
keyword exposures are the new way to do "potential exposures," i.e., exposure
telemetry for hypothetical suggestions that includes matching keywords. Like
every other Rust suggestion type, `Exposure` suggestions take their keywords
from remote settings and can do sophisticated matching.
Other changes in this revision:
* Keyword exposures are recorded on each instance of a matched result. With the
current approach, potential exposures are recorded per unique matched keyword.
That gives us more info. I cleared this idea with Dave.
* Properly detect `terminal` cases by comparing the final query context with the
context at the time of the exposure. The current detection is wrong because
it's only based on search strings, which doesn't work because the final search
string could also have been typed earlier in the session.
* In the feature manifest, change the branch of the related `setPref` variables
from `default` to `user`. When an experiment is uninstalled, the Nimbus client
[does *not* restore previous defaults](https://searchfox.org/mozilla-central/rev/446ff34da077a31d5550359480cb327f729c027b/toolkit/components/normandy/lib/PrefUtils.sys.mjs#119) for prefs set on the default branch. That
breaks some of the tests and also doesn't seem good for users.
Differential Revision: https://phabricator.services.mozilla.com/D220501
This replaces `UrlbarResult.exposureResultType` and `exposureResultHidden` with
a single simple value called `exposureTelemetry`. Please see the bug for the
rationale.
Other changes:
* For convenience, make `UrlbarPrefs.get("exposureResults")` return a `Set` so
consumers don't have to parse the string value.
* Simplify handling of result properties in `check_results()` (xpcshell tests)
* Add more tasks/checks to test_exposure.js
Depends on D219939
Differential Revision: https://phabricator.services.mozilla.com/D219942
If a provider onEngagement method causes the address bar panel to close when
a result is picked, we may record consecutive engagement and abandonment events
for the same search session.
Differential Revision: https://phabricator.services.mozilla.com/D206110
The problem has to do with hidden exposures whose hypothetical rows can't
immediately be made visible either because they would overflow the filled-up
view or because a misplaced result was seen.
The relevant part of `#updateResults()` is [here](https://searchfox.org/mozilla-central/rev/6a2a2a52d7e544a2fd5678d04991a7e78b694f22/browser/components/urlbar/UrlbarView.sys.mjs#1289-1291). When `canBeVisible` is false,
we effectively forget about that hidden-exposure result, which is not right. For
normal results, we also have that `canBeVisible` logic (a few lines down from
the part I linked), but the difference is that we create a hidden row for them
and then unhide the row when the query finishes and stale rows are removed.
We're missing an analog to that logic for hidden-exposure results, so we're
undercounting them.
Fortunately it's not too hard to fix. I added a new kind of exposure for
`TelemetryEvent` to track called "tentative" exposures. Tentative exposures are
entirely analogous to this case where a row would be added but hidden, and then
unhidden when stale rows are removed.
While I was modifying the test, I rephrased the comments for existing tasks so
hopefully they're easier to understand.
Differential Revision: https://phabricator.services.mozilla.com/D205155
#selectedElement may end up pointing to disconnected nodes. And so the public
.selectedElement getter.
This is how it was happening: a first call to onQueryResults adds and selects a
heuristic result. Then a second call to onQueryResults brings a new heuristic
result that requires new content (not compatible with the previous one), so the
old heuristic is emptied out, and new DOM is generated.
Because the code in onQueryResults relies on .selectedElement, at the second
invokation it thinks the selection is still valid, and doesn't select the new
heuristic. In reality .selectedElement at that time is pointing to a removed
DOM node.
The patch introduces a #rawSelectedElement and converts #selectedElement
into a getter.
Plus some minor logging improvements, and removing unused #mainContainer property.
Differential Revision: https://phabricator.services.mozilla.com/D195779
