Skip to content

Commit 7ea276b

Browse files
committed
Bug 1886175 - Properly count urlbar hidden exposures whose hypothetical rows can't immediately be shown. r=mak,wstuckey
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
1 parent 691384c commit 7ea276b

File tree

3 files changed

+653
-66
lines changed

3 files changed

+653
-66
lines changed

browser/components/urlbar/UrlbarController.sys.mjs

Lines changed: 53 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -708,7 +708,6 @@ class TelemetryEvent {
708708
constructor(controller, category) {
709709
this._controller = controller;
710710
this._category = category;
711-
this.#exposureResultTypes = new Set();
712711
this.#beginObservingPingPrefs();
713712
}
714713

@@ -1093,18 +1092,20 @@ class TelemetryEvent {
10931092
}
10941093

10951094
// First check to see if we can record an exposure event
1096-
if (
1097-
(method === "abandonment" || method === "engagement") &&
1098-
this.#exposureResultTypes.size
1099-
) {
1100-
const exposureResults = Array.from(this.#exposureResultTypes).join(",");
1101-
this._controller.logger.debug(
1102-
`exposure event: ${JSON.stringify({ results: exposureResults })}`
1103-
);
1104-
Glean.urlbar.exposure.record({ results: exposureResults });
1095+
if (method === "abandonment" || method === "engagement") {
1096+
if (this.#exposureResultTypes.size) {
1097+
let exposure = {
1098+
results: [...this.#exposureResultTypes].sort().join(","),
1099+
};
1100+
this._controller.logger.debug(
1101+
`exposure event: ${JSON.stringify(exposure)}`
1102+
);
1103+
Glean.urlbar.exposure.record(exposure);
1104+
}
11051105

11061106
// reset the provider list on the controller
11071107
this.#exposureResultTypes.clear();
1108+
this.#tentativeExposureResultTypes.clear();
11081109
}
11091110

11101111
this._controller.logger.info(
@@ -1115,16 +1116,54 @@ class TelemetryEvent {
11151116
}
11161117

11171118
/**
1118-
* Add result type to engagementEvent instance exposureResultTypes Set.
1119+
* Registers an exposure for a result in the current urlbar session. All
1120+
* exposures that are added during a session are recorded in an exposure event
1121+
* at the end of the session. Exposures are cleared at the end of each session
1122+
* and do not carry over to the next session.
11191123
*
1120-
* @param {UrlbarResult} result UrlbarResult to have exposure recorded.
1124+
* @param {UrlbarResult} result An exposure will be added for this result if
1125+
* exposures are enabled for its result type.
11211126
*/
11221127
addExposure(result) {
11231128
if (result.exposureResultType) {
11241129
this.#exposureResultTypes.add(result.exposureResultType);
11251130
}
11261131
}
11271132

1133+
/**
1134+
* Registers a tentative exposure for a result in the current urlbar session.
1135+
* Exposures that remain tentative at the end of the session are discarded and
1136+
* are not recorded in the exposure event.
1137+
*
1138+
* @param {UrlbarResult} result A tentative exposure will be added for this
1139+
* result if exposures are enabled for its result type.
1140+
*/
1141+
addTentativeExposure(result) {
1142+
if (result.exposureResultType) {
1143+
this.#tentativeExposureResultTypes.add(result.exposureResultType);
1144+
}
1145+
}
1146+
1147+
/**
1148+
* Converts all tentative exposures that were added and not yet discarded
1149+
* during the current urlbar session into actual exposures that will be
1150+
* recorded at the end of the session.
1151+
*/
1152+
acceptTentativeExposures() {
1153+
for (let type of this.#tentativeExposureResultTypes) {
1154+
this.#exposureResultTypes.add(type);
1155+
}
1156+
this.#tentativeExposureResultTypes.clear();
1157+
}
1158+
1159+
/**
1160+
* Discards all tentative exposures that were added and not yet accepted
1161+
* during the current urlbar session.
1162+
*/
1163+
discardTentativeExposures() {
1164+
this.#tentativeExposureResultTypes.clear();
1165+
}
1166+
11281167
#getInteractionType(
11291168
method,
11301169
startEventInfo,
@@ -1313,5 +1352,6 @@ class TelemetryEvent {
13131352

13141353
#previousSearchWordsSet = null;
13151354

1316-
#exposureResultTypes;
1355+
#exposureResultTypes = new Set();
1356+
#tentativeExposureResultTypes = new Set();
13171357
}

browser/components/urlbar/UrlbarView.sys.mjs

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1181,6 +1181,10 @@ export class UrlbarView {
11811181
// future we should make it support any type of result. Or, even better,
11821182
// results should be grouped, thus we can directly update groups.
11831183

1184+
// Discard tentative exposures. This is analogous to marking the
1185+
// hypothetical hidden rows of hidden-exposure results as stale.
1186+
this.controller.engagementEvent.discardTentativeExposures();
1187+
11841188
// Walk rows and find an insertion index for results. To avoid flicker, we
11851189
// skip rows until we find one compatible with the result we want to apply.
11861190
// If we couldn't find a compatible range, we'll just update.
@@ -1217,7 +1221,7 @@ export class UrlbarView {
12171221
) {
12181222
// We can replace the row's current result with the new one.
12191223
if (result.exposureResultHidden) {
1220-
this.#addExposure(result);
1224+
this.controller.engagementEvent.addExposure(result);
12211225
} else {
12221226
this.#updateRow(row, result);
12231227
}
@@ -1287,7 +1291,13 @@ export class UrlbarView {
12871291
newSpanCount <= this.#queryContext.maxResults && !seenMisplacedResult;
12881292
if (result.exposureResultHidden) {
12891293
if (canBeVisible) {
1290-
this.#addExposure(result);
1294+
this.controller.engagementEvent.addExposure(result);
1295+
} else {
1296+
// Add a tentative exposure: The hypothetical row for this
1297+
// hidden-exposure result can't be visible now, but as long as it were
1298+
// not marked stale in a later update, it would be shown when stale
1299+
// rows are removed.
1300+
this.controller.engagementEvent.addTentativeExposure(result);
12911301
}
12921302
continue;
12931303
}
@@ -2126,7 +2136,7 @@ export class UrlbarView {
21262136
let visible = this.#isElementVisible(item);
21272137
if (visible) {
21282138
if (item.result.exposureResultType) {
2129-
this.#addExposure(item.result);
2139+
this.controller.engagementEvent.addExposure(item.result);
21302140
}
21312141
this.visibleResults.push(item.result);
21322142
}
@@ -2363,6 +2373,10 @@ export class UrlbarView {
23632373
row = next;
23642374
}
23652375
this.#updateIndices();
2376+
2377+
// Accept tentative exposures. This is analogous to unhiding the
2378+
// hypothetical non-stale hidden rows of hidden-exposure results.
2379+
this.controller.engagementEvent.acceptTentativeExposures();
23662380
}
23672381

23682382
#startRemoveStaleRowsTimer() {
@@ -3460,15 +3474,6 @@ export class UrlbarView {
34603474
this.#populateResultMenu();
34613475
}
34623476
}
3463-
3464-
/**
3465-
* Add result to exposure set on the controller.
3466-
*
3467-
* @param {UrlbarResult} result UrlbarResult for which to record an exposure.
3468-
*/
3469-
#addExposure(result) {
3470-
this.controller.engagementEvent.addExposure(result);
3471-
}
34723477
}
34733478

34743479
/**

0 commit comments

Comments
 (0)