1857236 - invoking onEngagement on every provider is error prone

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P3)

Description

[Marco Bonardo [:mak]]

A UrlbarProvider would expect to receive onEngagement only for its own results, but currently we notify it to ALL providers.
This is very error prone as if one forgets about this detail they may end up handling the wrong results.
IIRC there was a reason related to either telemetry, impressions counting, or the experimental API (that we'd like to remove in bug 1855958 anyway), we must check.
We could then remove the various checks for result.providerName in the onEngagement implementations.

Comment 1

[Marco Bonardo [:mak]]

We discussed this, and we'd like to split onEngegament in: onEngagement, onAbandonment, onImpression, where onImpression won't happen for the "impression" event (we should rename it to "pause" event or such), but will rather stick to the telemetry definition (result visible when engagement or abandonment happen).
Each provider will declare which notifications it wants to handle through something like:

get notifications() { 
  return ["abandonment", "engagement", "impression", ...];
}

Comment 2

[Karandeep Lubana [:klubana]]

Attachment: Bug 1857236 - invoking onEngagement on every provider is error prone. r=mak

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58b04615a138
invoking onEngagement on every provider is error prone. r=mak

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/58b04615a138