2003624 - For dismissals of AMP suggestions, don't fall back to full keyword

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P1)

Description

[Drew Willcoxon :adw]

Bug 2002383 added a hardcoded fallback to full keyword for Merino AMP suggestion dismissals. We decided that wasn't a good idea after all, so let's revert that part of the bug. See that bug for details.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 2003624 - For dismissals of AMP suggestions, don't fall back to full keyword.

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 first change. This patch keeps the
second change since it might be useful in the future.

The only part of this patch that's concerned with reverting the first 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().

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/33e94c6ee993
https://hg.mozilla.org/integration/autoland/rev/a3c18c37a0cf
For dismissals of AMP suggestions, don't fall back to full keyword. r=daisuke

Comment 3

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/a3c18c37a0cf

Comment 4

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #1)

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 first change. This patch keeps the
second change since it might be useful in the future.

I got this backwards: The main point is to revert the second change, not the first. I updated the summary in phab but wanted to note it here too.