2051435 - [IPC Urlbar] Phase 5: Split TelemetryEvent and carry it over the actor boundary

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P1)

Description

[Dão Gottwald [:dao]]

Phase 5 of the IPC Urlbar work (meta Bug 2049112). Split the urlbar's parent-side TelemetryEvent so it can run for a content-process <moz-urlbar>:

• Decouple the recording helpers from the content side: they currently read this._controller.input/view/browserWindow (searchMode, visibleResults, viewIsOpen, and the browser-window facts behind the SAP). Route those through small accessors so the data can come from the live input/view (chrome direct path) or over the actor (content process).
• Split TelemetryEvent into a child-side collector (start/discard/exposure queues) and a parent-side recorder (Glean, SearchService, Interactions, bounce), and carry engagement / exposures / bounce over the Urlbar actor.
• Engagement recording is parent-driven: on about:newtab, picking a result navigates and unloads the document (tearing down the child actor), so a fire-and-forget RecordEngagement could be lost; the content snapshot rides with the pick action and is recorded atomically parent-side, and teardown flushes rather than drops.

Gated by browser.urlbar.ipc.chromeMessagePassing (off by default); behavior-neutral for chrome. Capstone: a pref-on CI variant runs the chrome urlbar suite over the wire path.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Decouple the engagement-telemetry recording from the input and view. r?daleharvey,mbeier

TelemetryEvent's engagement-recording helpers (#recordSearchEngagementTelemetry,
shared by the engagement, disable, and bounce paths, and #searchSourceToSap) read
the content side directly: input.searchMode, view.visibleResults, view.isOpen,
and the browser-window facts behind the SAP. A content-process <moz-urlbar> has
none of those in-process.

Route those reads through two private getters, #engagementData (searchMode,
visibleResults, viewIsOpen) and #sapMetaData (the SAP window facts), and have the
helpers read them instead of _controller.input/view/browserWindow. The getters
read the live input/view, so values and timing are unchanged on the in-process
chrome path; a later patch overrides them to supply the data over the actor for a
content-process urlbar.

Behavior-neutral; no external call-site changes.

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Decouple exposures and smartbar telemetry from the input and view. r?daleharvey,mbeier

Continuing the TelemetryEvent decouple: #recordExposures read
input.getSearchSource() and view.visibleResults directly, and
#getOptionalSmartbarTelemetry read the SmartbarInput fields. Route both through
getters: add searchSource to #engagementData (exposures reads it alongside
visibleResults), and add a #smartbarData getter for the smartbar-only fields
(chatId/intent/model).

The getters read the live input/view on the chrome direct path, so this is
behavior-neutral; a later patch overrides them to supply the data over the actor
for a content-process urlbar. After this, the only content-side reads left in
TelemetryEvent are the bounce getBrowserState calls.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Own bounce-tracking state in the parent controller instead of the input. r?daleharvey,mbeier

Comment 4

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Assemble an engagement snapshot and record from it. r?daleharvey,mbeier

Comment 5

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Thread the engagement interaction type explicitly instead of mutating the event. r?daleharvey,mbeier

Comment 6

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Extract engagement-snapshot collection into a shared UrlbarTelemetryUtils module. r?daleharvey,mbeier

Comment 7

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Record engagement telemetry over the actor with a content-side collector. r?daleharvey,mbeier

Comment 8

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Decouple the exposure Glean recording from the queue resolution. r?daleharvey,mbeier

Comment 9

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Carry exposure telemetry over the actor with the child collector. r?daleharvey,mbeier

Comment 10

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Resolve the bounce snapshot at tracking time. r?daleharvey,mbeier

Comment 11

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Carry bounce telemetry over the actor with the child collector. r?daleharvey,mbeier

Comment 12

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Fill the controller-proxy gaps so the wire path runs end-to-end. r?daleharvey,mbeier

Comment 13

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Make the engagement telemetry-assert helpers wait for message-path recording. r?daleharvey,mbeier

Comment 14

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Resolve the urlbar's browser window from the actor, not the content child. r?daleharvey,mbeier

On the message path the parent controller had no browser window: it read
browserWindow from the content-side child controller, which the message path
doesn't have. Parent-side providers that read controller.browserWindow
(search suggestions' icons, clipboard, search tips) therefore threw and dropped
their results.

Resolve browserWindow parent-side from the actor's chrome window instead. The
browser window is a parent-process object, unlike the input and view, so it
shouldn't be proxied through the child. With the parent able to reach it on both
paths, the SAP window facts (#sapMetaData) are now computed parent-side rather
than gathered content-side and shipped over the wire.

Also rename the child controller's browserWindow to window (it's the input's
window, which is a content window rather than a browser window in a content
process), and guard the Esc focus-content-document path to chrome, where
gBrowser exists and moving focus into content is meaningful.

Comment 15

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Trim transitional rationale from the IPC Urlbar comments. r?daleharvey,mbeier

Per review feedback, drop the migration/transitional rationale that had
accumulated in docstrings and comments across the controllers ("rather than a
DOM input", "no longer holds", "goes away once", "future content-process
consumers", "Telemetry is wired up in a later patch", etc.). That kind of
before/after "why" belongs in the commit message; once the split has shipped it
only adds noise and goes stale. The durable contract and the non-obvious
invariants (e.g. why the browser window is resolved from the actor, why dispatch
lives on the child) are kept.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e6649578d1bd
https://hg.mozilla.org/integration/autoland/rev/3e45ad465771
[IPC Urlbar] Phase 5: Decouple the engagement-telemetry recording from the input and view. r=daleharvey,urlbar-reviewers
https://github.com/mozilla-firefox/firefox/commit/ea5322df50ad
https://hg.mozilla.org/integration/autoland/rev/b7d4feaaafcc
[IPC Urlbar] Phase 5: Decouple exposures and smartbar telemetry from the input and view. r=daleharvey,urlbar-reviewers

Comment 17

[Phabricator Automation]

Comment on attachment 9603283 [details]
Bug 2051435 - [IPC Urlbar] Phase 5: Fill the controller-proxy gaps so the wire path runs end-to-end. r?daleharvey,mbeier

Revision D309765 was moved to bug 2052787. Setting attachment 9603283 [details] to obsolete.

Comment 18

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Await the in-progress bounce trigger before re-tracking in the child collector. r?daleharvey,mbeier

handleBounceEventTrigger became async (it awaits Interactions), so when a new
engagement starts tracking while a bounce is already in progress,
startTrackingBounceEvent must await the prior trigger before overwriting the
tracking state. Make it async and await accordingly.

Comment 19

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3e45ad465771
https://hg.mozilla.org/mozilla-central/rev/b7d4feaaafcc

Comment 20

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Build the engagement Glean event through shared UrlbarTelemetryUtils statics. r?daleharvey!

A behavior-neutral extraction. The engagement/abandonment Glean event was built
inline in the parent recorder; this moves the building into pure
UrlbarTelemetryUtils statics and routes the recorder through a single
recordFromChild() entry point (which fills the parent-only fields -- sap, default
engine, semantic sources -- and makes the Glean call).

Nothing changes on the direct path: it builds via the same statics and calls
recordFromChild() in-process, producing identical events -- that equivalence is
what to check here. The point of the move is that the statics are pure, so the
next commit's content-side collector can build the event and hand it to the same
recordFromChild() over the actor. The new xpcshell test pins the wire round-trip.

Comment 21

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Carry engagement telemetry over the actor with a content-side collector. r?daleharvey!

Builds on the extracted statics. Adds the content-side UrlbarChildTelemetry
collector that the child controller hands back as engagementEvent on the message
path (where the parent stand-in has no recorder): it builds the event with the
shared statics and ships it over the actor via recordEngagement, and the parent
deserializes and calls the same recordFromChild(). Also lands the
proxy/actor/child-controller wiring and moves browser-window resolution to the
actor.

The direct path is untouched -- the child controller still returns the real
parent recorder, so recording stays in-process; the collector only runs when the
message-passing pref is on. Exposure and bounce are stubbed no-ops here and cross
the actor in later commits.

Comment 22

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Carry exposure telemetry over the actor with the content-side collector. r?daleharvey!

Extends the collector to exposures, mirroring the engagement path; behavior-neutral
on the direct path.

Exposures only record at session end alongside the engagement, so the collector
owns the exposure queue and resolves it to a final list that rides in the
engagement payload. The parent's #recordExposures() splits into
#resolveExposureList() (queue -> list) and #recordExposureList() (record it) so
both paths share the recording: the direct path resolves from its own queue, the
message path records the list the collector shipped. Worth checking: the terminal
flag is now computed content-side -- confirm it matches the previous parent-side
value.

Comment 23

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Resolve the bounce snapshot at tracking time. r?daleharvey!

Resolve the bounce snapshot at startTrackingBounceEvent() time rather than
storing the DOM event and resolving at record time, so the deferred recording
(which runs after the event is gone, and parent-side on the message path) needs
neither the event nor the element. Behavior-neutral.

Comment 24

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Carry bounce telemetry over the actor with the content-side collector. r?daleharvey!

Extends the collector to bounces, the last concern to cross the actor;
behavior-neutral on the direct path.

The collector owns per-browser bounce tracking content-side and, on a trigger,
ships one HandleBounceTrigger with the resolved snapshot's built event, the
tracking start time, and the embedder's browsing-context id; the parent resolves
the browser, asks Interactions for the view time, and records if under the
threshold. recordBounceEvent() splits into a shared #recordBounce() so both paths
record identically. The assert{Engagement,Abandonment,Bounce}Telemetry helpers now
await recording (they resolve immediately in-process) so tests don't race the
actor round-trip -- that's why several tests gained awaits.

Comment 25

[Dão Gottwald [:dao]]

Attachment: Bug 2051435 - [IPC Urlbar] Phase 5: Build the disable-suggest telemetry event content-side. r?daleharvey!

Disable-suggest tracking still built its Glean event parent-side at
pref-toggle time, re-deriving the interaction type and action from the
engagement's DOM event and reading the live input via the engagement data.
Neither is available to a message-path parent controller, which reaches the
input over the actor, so the disable event never recorded there.

Build the disable candidate content-side when a Suggest result is engaged or
abandoned -- the same way bounce is tracked -- and ship the built event. The
parent holds it and, if the user turns Suggest off within the threshold,
records it via #fillAndRecord(), needing neither the DOM event nor the live
input.

Behavior-neutral on the direct path: the recorded Glean events are identical.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/597bd5b37427
https://hg.mozilla.org/integration/autoland/rev/1c472dfa83d1
[IPC Urlbar] Phase 5: Assemble an engagement snapshot and record from it. r=daleharvey,urlbar-reviewers
https://github.com/mozilla-firefox/firefox/commit/22b2b36b5d27
https://hg.mozilla.org/integration/autoland/rev/d992ccb4e931
[IPC Urlbar] Phase 5: Thread the engagement interaction type explicitly instead of mutating the event. r=daleharvey,urlbar-reviewers
https://github.com/mozilla-firefox/firefox/commit/161e7e389cf2
https://hg.mozilla.org/integration/autoland/rev/61fba7ec25e0
[IPC Urlbar] Phase 5: Extract engagement-snapshot collection into a shared UrlbarTelemetryUtils module. r=daleharvey,urlbar-reviewers

Comment 27

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1c472dfa83d1
https://hg.mozilla.org/mozilla-central/rev/d992ccb4e931
https://hg.mozilla.org/mozilla-central/rev/61fba7ec25e0

Comment 28

[Dão Gottwald [:dao]]

Attachment: WIP: [IPC Urlbar] Phase 5 collapsed view: telemetry over the actor (not for landing)

Squashed, read-only view of the Phase 5 telemetry-foundation commits, so a
reviewer can see the whole phase as a single diff. This patch is not for
landing; the official review happens on the separated commits in the stack.

Depends on D309759

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0b0107a89f93
https://hg.mozilla.org/integration/autoland/rev/2c688fda2364
[IPC Urlbar] Phase 5: Build the engagement Glean event through shared UrlbarTelemetryUtils statics. r=daleharvey
https://github.com/mozilla-firefox/firefox/commit/4a413576f97f
https://hg.mozilla.org/integration/autoland/rev/00b2cc915246
[IPC Urlbar] Phase 5: Carry engagement telemetry over the actor with a content-side collector. r=daleharvey
https://github.com/mozilla-firefox/firefox/commit/c43f9ff85e45
https://hg.mozilla.org/integration/autoland/rev/26b5255b2ed2
[IPC Urlbar] Phase 5: Carry exposure telemetry over the actor with the content-side collector. r=daleharvey
https://github.com/mozilla-firefox/firefox/commit/aa56b3500ac1
https://hg.mozilla.org/integration/autoland/rev/4426955f2b4d
[IPC Urlbar] Phase 5: Resolve the bounce snapshot at tracking time. r=daleharvey
https://github.com/mozilla-firefox/firefox/commit/60277f56fd72
https://hg.mozilla.org/integration/autoland/rev/af6e82170e8b
[IPC Urlbar] Phase 5: Carry bounce telemetry over the actor with the content-side collector. r=daleharvey
https://github.com/mozilla-firefox/firefox/commit/fe375cedeb74
https://hg.mozilla.org/integration/autoland/rev/501cf6f89fcf
[IPC Urlbar] Phase 5: Build the disable-suggest telemetry event content-side. r=daleharvey
https://github.com/mozilla-firefox/firefox/commit/4c02810a59dc
https://hg.mozilla.org/integration/autoland/rev/a03c03e8ba39
[IPC Urlbar] Phase 5: Trim transitional rationale from the IPC Urlbar comments. r=daleharvey

Comment 30

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2c688fda2364
https://hg.mozilla.org/mozilla-central/rev/00b2cc915246
https://hg.mozilla.org/mozilla-central/rev/26b5255b2ed2
https://hg.mozilla.org/mozilla-central/rev/4426955f2b4d
https://hg.mozilla.org/mozilla-central/rev/af6e82170e8b
https://hg.mozilla.org/mozilla-central/rev/501cf6f89fcf
https://hg.mozilla.org/mozilla-central/rev/a03c03e8ba39