commit 33e94c6ee9937ddcafadbf26e3d227510b189eba
parent 3477266a56ee2b93154112a287ecccf12a3d898b
Author: Drew Willcoxon <adw@mozilla.com>
Date: Wed, 3 Dec 2025 00:19:35 +0000
Bug 2003624 - For dismissals of AMP suggestions, don't fall back to full keyword. r=daisuke
D274079 made two changes:
1. Look for `dismissal_key` in Merino suggestions
2. For AMP Merino suggestions, fall back to the full keyword as the dismissal
key if `dismissal_key` isn't defined
The main point of this patch is to revert the second change. This patch keeps
the first change since it might be useful in the future.
The only part of this patch that's concerned with reverting the second change is
the small chunk in `AmpSuggestions` that starts with the comment, "AMP
suggestions are dismissed by their full keyword".
The rest of the patch is some related cleanup and (hopefully) improvement that I
took this opportunity to do. Summary:
* In `UrlbarProviderQuickSuggest.#makeResult()` - Document the code better by
splitting it up into smaller chunks and documenting each chunk.
* Also in `UrlbarProviderQuickSuggest` - For unmanaged Merino suggestions, set
`result.payload.isSponsored` in `#makeUnmanagedResult()` rather than in
`#makeResult()`. That way, all the Merino-specific code is in one place, in
`#makeUnmanagedResult()`.
* In `AmpSuggestions.makeResult()` - Stop modifying the passed-in suggestion
object and make it clearer that we're normalizing the Merino suggestion in
order to make it like Rust suggestions.
* Also in `AmpSuggestions.makeResult()` - Pass the normalized Merino suggestion
to `#replaceSuggestionTemplates()`. That way, `#replaceSuggestionTemplates()`
doesn't need to assume that the passed-in suggestion is from Merino and
therefore uses snake_case. Or IOW all the Merino-specific code is now in one
place, in `makeResult()`.
Differential Revision: https://phabricator.services.mozilla.com/D274846
Diffstat:
4 files changed, 66 insertions(+), 65 deletions(-)
diff --git a/browser/components/urlbar/UrlbarProviderQuickSuggest.sys.mjs b/browser/components/urlbar/UrlbarProviderQuickSuggest.sys.mjs
@@ -353,25 +353,34 @@ export class UrlbarProviderQuickSuggest extends UrlbarProvider {
return null;
}
- // Set important properties that every Suggest result should have. See
- // `QuickSuggest.getFeatureBySource()` for `source` and `provider` values.
- // If the suggestion isn't managed by a feature, then it's from Merino and
- // we assume `is_sponsored` is set appropriately. (Merino uses snake_case.)
+ // Set important properties that every Suggest result should have.
+
+ // `source` indicates the Suggest backend the suggestion came from.
result.payload.source = suggestion.source;
+
+ // `provider` depends on `source` and generally indicates the type of
+ // Suggest suggestion. See `QuickSuggest.getFeatureBySource()`.
result.payload.provider = suggestion.provider;
- if (suggestion.suggestionType) {
- // `suggestionType` is defined only for dynamic Rust suggestions and is
- // the dynamic type. Avoid adding an undefined property to other payloads.
- result.payload.suggestionType = suggestion.suggestionType;
+
+ // Set `isSponsored` unless the feature already did.
+ if (!result.payload.hasOwnProperty("isSponsored")) {
+ result.payload.isSponsored = !!feature?.isSuggestionSponsored(suggestion);
}
+
+ // For most Suggest results, the result type recorded in urlbar telemetry is
+ // `${source}_${telemetryType}` (the payload values).
result.payload.telemetryType = this.#getSuggestionTelemetryType(suggestion);
- result.payload.isSponsored = feature
- ? feature.isSuggestionSponsored(suggestion)
- : !!suggestion.is_sponsored;
+
+ // Handle icons here unless the feature already did.
+ result.payload.icon ||= suggestion.icon;
+ result.payload.iconBlob ||= suggestion.icon_blob;
switch (suggestion.source) {
case "merino":
- // Set `dismissalKey` unless the feature already did it.
+ // Dismissals of Merino suggestions are recorded in the Rust component's
+ // database. Each dismissal is recorded as a string value called a key.
+ // If Merino includes `dismissal_key` in the suggestion, use that as the
+ // key. Otherwise we'll use its URL. See `QuickSuggest.dismissResult()`.
if (
suggestion.dismissal_key &&
!result.payload.hasOwnProperty("dismissalKey")
@@ -381,15 +390,16 @@ export class UrlbarProviderQuickSuggest extends UrlbarProvider {
break;
case "rust":
// `suggestionObject` is passed back to the Rust component on dismissal.
+ // See `QuickSuggest.dismissResult()`.
result.payload.suggestionObject = suggestion;
+ // `suggestionType` is defined only for dynamic Rust suggestions and is
+ // the dynamic type. Don't add an undefined property to other payloads.
+ if (suggestion.suggestionType) {
+ result.payload.suggestionType = suggestion.suggestionType;
+ }
break;
}
- // Handle icons here so each feature doesn't have to do it, but use `||=` to
- // let them do it if they need to.
- result.payload.icon ||= suggestion.icon;
- result.payload.iconBlob ||= suggestion.icon_blob;
-
// Set the appropriate suggested index and related properties unless the
// feature did it already.
if (!result.hasSuggestedIndex) {
@@ -455,6 +465,7 @@ export class UrlbarProviderQuickSuggest extends UrlbarProvider {
let payload = {
url: suggestion.url,
originalUrl: suggestion.original_url,
+ isSponsored: !!suggestion.is_sponsored,
isBlockable: true,
isManageable: true,
};
diff --git a/browser/components/urlbar/private/AmpSuggestions.sys.mjs b/browser/components/urlbar/private/AmpSuggestions.sys.mjs
@@ -92,58 +92,46 @@ export class AmpSuggestions extends SuggestProvider {
}
makeResult(queryContext, suggestion) {
- let originalUrl;
- if (suggestion.source == "rust") {
- // The Rust backend replaces URL timestamp templates for us, and it
- // includes the original URL as `rawUrl`.
- originalUrl = suggestion.rawUrl;
- } else {
- // Replace URL timestamp templates, but first save the original URL.
- originalUrl = suggestion.url;
- this.#replaceSuggestionTemplates(suggestion);
-
- // Normalize the Merino suggestion so it has camelCased properties like
- // Rust suggestions.
- suggestion.fullKeyword = suggestion.full_keyword;
- suggestion.impressionUrl = suggestion.impression_url;
- suggestion.clickUrl = suggestion.click_url;
- suggestion.blockId = suggestion.block_id;
- suggestion.iabCategory = suggestion.iab_category;
- suggestion.requestId = suggestion.request_id;
- suggestion.dismissalKey = suggestion.dismissal_key;
+ let normalized = Object.assign({}, suggestion);
+ if (suggestion.source == "merino") {
+ // Normalize the Merino suggestion so it has the same properties as Rust
+ // AMP suggestions: camelCased properties plus a `rawUrl` property whose
+ // value is `url` without replacing the timestamp template.
+ normalized.rawUrl = suggestion.url;
+ normalized.fullKeyword = suggestion.full_keyword;
+ normalized.impressionUrl = suggestion.impression_url;
+ normalized.clickUrl = suggestion.click_url;
+ normalized.blockId = suggestion.block_id;
+ normalized.iabCategory = suggestion.iab_category;
+ normalized.requestId = suggestion.request_id;
+
+ // Replace URL timestamp templates inline. This isn't necessary for Rust
+ // AMP suggestions because the Rust component handles it.
+ this.#replaceSuggestionTemplates(normalized);
}
let payload = {
- originalUrl,
- url: suggestion.url,
- title: suggestion.title,
- requestId: suggestion.requestId,
- urlTimestampIndex: suggestion.urlTimestampIndex,
- sponsoredImpressionUrl: suggestion.impressionUrl,
- sponsoredClickUrl: suggestion.clickUrl,
- sponsoredBlockId: suggestion.blockId,
- sponsoredAdvertiser: suggestion.advertiser,
- sponsoredIabCategory: suggestion.iabCategory,
+ url: normalized.url,
+ originalUrl: normalized.rawUrl,
+ title: normalized.title,
+ requestId: normalized.requestId,
+ urlTimestampIndex: normalized.urlTimestampIndex,
+ sponsoredImpressionUrl: normalized.impressionUrl,
+ sponsoredClickUrl: normalized.clickUrl,
+ sponsoredBlockId: normalized.blockId,
+ sponsoredAdvertiser: normalized.advertiser,
+ sponsoredIabCategory: normalized.iabCategory,
isBlockable: true,
isManageable: true,
};
- // AMP suggestions are dismissed by their full keyword, not by URL like
- // usual. For Rust suggestions, the Rust component handles that for us, no
- // need to do anything here. For Merino suggestions, Merino should include
- // `dismissal_key`, but fall back to the full keyword in case it doesn't.
- if (suggestion.source == "merino") {
- payload.dismissalKey =
- suggestion.dismissalKey || suggestion.fullKeyword || originalUrl;
- }
-
let isTopPick =
lazy.UrlbarPrefs.get("quickSuggestAmpTopPickCharThreshold") &&
lazy.UrlbarPrefs.get("quickSuggestAmpTopPickCharThreshold") <=
queryContext.trimmedLowerCaseSearchString.length;
payload.qsSuggestion = [
- suggestion.fullKeyword,
+ normalized.fullKeyword,
isTopPick
? lazy.UrlbarUtils.HIGHLIGHT.TYPED
: lazy.UrlbarUtils.HIGHLIGHT.SUGGESTED,
@@ -365,7 +353,7 @@ export class AmpSuggestions extends SuggestProvider {
let timestamp = timestampParts
.map(n => n.toString().padStart(2, "0"))
.join("");
- for (let key of ["url", "click_url"]) {
+ for (let key of ["url", "clickUrl"]) {
let value = suggestion[key];
if (!value) {
continue;
diff --git a/browser/components/urlbar/tests/quicksuggest/QuickSuggestTestUtils.sys.mjs b/browser/components/urlbar/tests/quicksuggest/QuickSuggestTestUtils.sys.mjs
@@ -526,7 +526,9 @@ class _QuickSuggestTestUtils {
});
} else {
result.payload.icon = icon;
- result.payload.dismissalKey = dismissalKey || fullKeyword || originalUrl;
+ if (typeof dismissalKey == "string") {
+ result.payload.dismissalKey = dismissalKey;
+ }
}
return result;
diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_merino.js b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_merino.js
@@ -428,9 +428,9 @@ add_task(async function dismissals_amp() {
full_keyword: "full keyword 2",
},
expected: {
- // dismissal key should be the `full_keyword` value
- dismissalKey: "full keyword 2",
- notDismissalKeys: ["https://example.com/2"],
+ // dismissal key should be the `url` value
+ dismissalKey: "https://example.com/2",
+ notDismissalKeys: ["full keyword 2"],
},
},
{
@@ -439,9 +439,9 @@ add_task(async function dismissals_amp() {
full_keyword: "full keyword 3",
},
expected: {
- // dismissal key should be the `full_keyword` value
- dismissalKey: "full keyword 3",
- notDismissalKeys: [`https://example.com/3-${TIMESTAMP_TEMPLATE}`],
+ // dismissal key should be the `url` value
+ dismissalKey: `https://example.com/3-${TIMESTAMP_TEMPLATE}`,
+ notDismissalKeys: ["full keyword 3"],
},
},
{
@@ -529,7 +529,7 @@ add_task(async function dismissals_amp() {
url: suggestion.url,
originalUrl: suggestion.original_url || suggestion.url,
displayUrl: suggestion.url.replace(/^https:\/\//, ""),
- dismissalKey: expected.dismissalKey,
+ dismissalKey: suggestion.dismissal_key,
requestId: suggestion.request_id,
sponsoredImpressionUrl: suggestion.impression_url,
sponsoredClickUrl: suggestion.click_url,
