Closed Bug 1857236 Opened 8 months ago Closed 29 days ago

invoking onEngagement on every provider is error prone

Categories

(Firefox :: Address Bar, task, P3)

task

Tracking

()

RESOLVED FIXED
128 Branch
Tracking Status
firefox128 --- fixed

People

(Reporter: mak, Assigned: klubana)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [sng][search-tech-debt])

Attachments

(1 file)

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.

See Also: → 1855958
See Also: → 1869307
Depends on: 1855958
See Also: 1855958

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", ...];
}
Assignee: nobody → klubana
Status: NEW → ASSIGNED
Pushed by klubana@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58b04615a138
invoking onEngagement on every provider is error prone. r=mak
Status: ASSIGNED → RESOLVED
Closed: 29 days ago
Resolution: --- → FIXED
Target Milestone: --- → 128 Branch
Blocks: 1897244
Blocks: 1897245
Regressions: 1898437
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: