2065803 - pinMultiSelectedTabs hands pinTab a bare metrics context, so pinned_tabs.pin records source "unknown"

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, task, P2)

Description

[Dão Gottwald [:dao]]

Tabbrowser.pinMultiSelectedTabs calls pinTab like this:

this.pinTab(tab, this.TabMetrics.decomposedContext(metricsContext));

but pinTab(aTab, { metricsContext = this.TabMetrics.UNKNOWN_CONTEXT } = {}) takes an options object. It looks for a metricsContext property on the context object it was handed, finds none, and falls back to UNKNOWN_CONTEXT.

That context reaches #notifyPinnedStatus, which puts it on the TabPinned event as detail.metricsContext. BrowserUsageTelemetry._onTabPinned records Glean.pinnedTabs.pin with source: event.detail.metricsContext.telemetrySource, so pinning a multi-selection records source: "unknown" for each tab rather than the surface the user pinned from.

Pinning a single tab is unaffected. So is the aggregate tab.interaction event: recordTabMetrics skips both a decomposed context and a non-user-triggered one, so the per-tab call records nothing either way.

unpinMultiSelectedTabs, directly below it, does it correctly:

this.unpinTab(selectedTabs[i], {
  metricsContext: this.TabMetrics.decomposedContext(metricsContext),
});

Both are in browser/components/tabbrowser/Tabbrowser.sys.mjs, and browser/components/tabbrowser/test/browser/tabs/browser_multiselect_tabs_pin_unpin.js is where a test for the recorded source would go.

Found while documenting the class's option-bag parameters for bug 2065769. TypeScript does not catch it: the argument is a call result rather than a fresh object literal, so no excess-property check applies.

Comment 1

[Dão Gottwald [:dao]]

Correction to the last paragraph above: TypeScript does catch this. With the class under checkJs, tsc reports

Tabbrowser.sys.mjs(8867,24): error TS2559: Type 'TabMetricsContext' has no properties in common with type '{ metricsContext?: TabMetricsContext; }'.

The weak-type check fires whenever the source type shares no properties with the target, whether or not the argument is a fresh object literal, so the excess-property reasoning I gave does not apply. The rest of the description stands.

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 2065803 - Record the surface multi-selected tabs were pinned from. r?mossop!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6a22871ac112
https://hg.mozilla.org/integration/autoland/rev/fa4e4bf7bc9b
Record the surface multi-selected tabs were pinned from. r=mossop

Comment 4

[Serban Stanca [:SerbanS]]

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