1827762 - Replace UrlbarProvider.pickResult() and blockResult() with onEngagement()

Status: VERIFIED FIXED

(Firefox :: Address Bar, task, P1)

Description

[Drew Willcoxon :adw]

See the revision summary.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1827762 - Replace UrlbarProvider.pickResult() and blockResult() with onEngagement()

This removes UrlbarProvider.pickResult() and blockResult() in favor of
handling picks and dismissals through onEngagement(). A number of providers
use those two methods, so this revision touches a lot of files.

Handling dismissals through onEngagement() means UrlbarInput.pickResult()
can no longer tell whether a result is successfully dismissed, so it can't
remove the result anymore. (Maybe onEngagement() could return some value
indicating it dismissed the result, but I don't want to go down that road.)
Instead, I split UrlbarController.handleDeleteEntry() into two methods: a
public one that removes the result and notifies listeners, and a private one
that handles dismissing the selected result internally in
UrlbarController. Providers that have dismissable results should now implement
onEngagement() and call controller.removeResult().

I made some other improvements to engagement handling. There's still room for
more but this patch is big enough already.

Other notable changes:

Include the engaged result in engagement notifications so providers have easy
access to it and can respond to clicks and dismissals more easily. That also
lets us stop passing selIndex and provider to engagementEvent.record()
since now it can compute those from the passed-in result.

Add the concept of isSessionOngoing to engagement notifications so providers
can tell whether an engagement ended the search session. Right now, providers
like quick suggest that record a bunch of provider-specific legacy telemetry
assume that onEngagement() ends the session, but that's no longer true.

Unify result buttons and result menu commands by setting
element.dataset.command on buttons (hopefully we can remove buttons soon, at
least the ones that aren't tip buttons)

Make sure we always notify providers on engagement even on dismissals or
when skipping legacy telemetry

Move dismissal of restyled search suggestions and history results from
UrlbarController.handleDeleteEntry() to the Places provider

Move dismissal of form history results from
UrlbarController.handleDeleteEntry() to the search suggestions provider

In the Places provider, remove the unused _addSearchEngineMatch() method. Also
remove the code in the "searchengine" case that creates a non-search-history
result. This code is unreached because the only time the provider creates a
"searchengine" match it also sets isSearchHistory to true.

In UrlbarTestUtils.promiseAutocompleteResultPopup(), change the default value
of the fireInputEvent param from false to true. This is necessary because
without a starting input event, the start event info in engagementEvent will
be null, so when engagementEvent.record() is called at the end of the
engagement, it will bail, and providers will not be notified of the engagement.
IMO true is a better default value anyway because input events will typically be
fired when the user performs a search.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9922ca4dcbe
Replace UrlbarProvider.pickResult() and blockResult() with onEngagement() r=mak

Comment 3

[Drew Willcoxon :adw]

Changing this to qe-verify+ -- this is a refactor so there should not be any changes visible to the user, but it's a big refactor with a large potential for regressions, so it's prudent to flag this for QA. And if any regressions are found in the near future related to clicks on urlbar results, they may be due to this bug.

There are no specific STR, we just want to make sure clicks on all the different types of urlbar results are still handled properly.

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 5

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

We verified this while performing the 114 Nightly Search Regression run across all OSs, regression bug 1831529 was submitted.