1805511 - Add `ASRouterTargeting` about non-browser default handlers, including `isDefaultPDFHandler`

Status: VERIFIED FIXED

(Firefox :: Messaging System, enhancement, P1)

Description

[Nick Alexander :nalexander [he/him]]

ASRouterTargeting knows if Firefox is the default browser, but not if Firefox is the default handler for other filetypes, such as isDefaultPDFHandler.

I don't know how general this should be; it would be nice if we exposed the entire list of recognized handlers, a la BrowserContentHandler.sys.mjs, but I see that we're not even recording telemetry for defaults beyond PDF (see BrowserGlue.sys.mjs).

This is probably Windows-only. In addition, we might want to include a special bit about whether the existing default handler is a recognized browser, since we want to treat other browsers differently than general purpose readers such as Acrobat Reader or Nitro PDF.

Comment 1

[Ed Lee :Mardak]

This would most likely be used along with bug 1805509 as part of an upcoming experiment. If this is exposed more broadly as any handler instead of just pdf, we could have the targeting return an object keyed on those file extensions? E.g.,

isDefaultHandler: {
  html: true,
  pdf: true,
}

Then target on isDefaultHandler.pdf or probably negated for this experiment.

Comment 2

[Beth Rennie [:beth] (she/her)]

(In reply to Ed Lee :Mardak from comment #1)

This would most likely be used along with bug 1805509 as part of an upcoming experiment. If this is exposed more broadly as any handler instead of just pdf, we could have the targeting return an object keyed on those file extensions? E.g.,

isDefaultHandler: {
  html: true,
  pdf: true,
}

Then target on isDefaultHandler.pdf or probably negated for this experiment.

:nalexander, what do you think about this? This format would break your implementation of getEnvironmentSnapshot() due to having promises nested inside an object. Should getEnvironmentSnapshot be updated to recursively resolve -- or walk a single level deep if the object it would resolve is not a promise?

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Barret Rennie [:barret] (they/them) from comment #2)

(In reply to Ed Lee :Mardak from comment #1)

This would most likely be used along with bug 1805509 as part of an upcoming experiment. If this is exposed more broadly as any handler instead of just pdf, we could have the targeting return an object keyed on those file extensions? E.g.,

isDefaultHandler: {
  html: true,
  pdf: true,
}

Then target on isDefaultHandler.pdf or probably negated for this experiment.

:nalexander, what do you think about this?

This looks good to me. Please don't forget about the extra bit of "default PDF handler is a known-browser", the experiment wants that.

This format would break your implementation of getEnvironmentSnapshot() due to having promises nested inside an object. Should getEnvironmentSnapshot be updated to recursively resolve -- or walk a single level deep if the object it would resolve is not a promise?

I see no immediate problem with either. Recursively resolving probably makes the most sense?

Comment 4

[Beth Rennie [:beth] (she/her)]

Attachment: Bug 1805511 - Add targeting for default PDF and HTML handlers r?#omc-reviewers!,nalexander!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae16a9890f22
Add targeting for default PDF and HTML handlers r=nalexander,omc-reviewers,emcminn

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing for causing node newtab failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/ba36dea109e7851e4c392dc57e6c7aada3790fbf

Push with failures

Failure log NODE NEWTAB: TEST-UNEXPECTED-FAIL karma | activity-stream:ASRTargeting docs:All targeting params documented in targeting-attributes.md should have docs and table of contents entry for isDefaultHandler: Didn't find the heading:

Failure log BROWSER-CHROME: TEST-UNEXPECTED-FAIL | browser/components/newtab/test/browser/browser_asrouter_targeting.js | Uncaught exception in test bound check_totalBookmarksCount

Failure log XPC : toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js | test_backgroundtask_Nimbus_targeting - test_backgroundtask_Nimbus_targeting

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5bd4ad558f2
Add targeting for default PDF and HTML handlers r=nalexander,omc-reviewers,emcminn

Comment 8

[Sandor Molnar[:smolnar]]

Backed out for causing browser-chrome and xpc failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/8ff160f42b181a2ccd2be11ca7a632c271c1b639

Push with failures

Failure log: browser-chrome -> TEST-UNEXPECTED-FAIL | browser/components/newtab/test/browser/browser_asrouter_targeting.js | Uncaught exception in test bound check_totalBookmarksCount

Failure log: xpc -> TEST-UNEXPECTED-FAIL | toolkit/components/backgroundtasks/tests/xpcshell/test_backgroundtask_experiments.js | test_backgroundtask_Nimbus_targeting - [test_backgroundtask_Nimbus_targeting : 307]

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc29190ca789
Add targeting for default PDF and HTML handlers r=nalexander,omc-reviewers,emcminn

Comment 10

[Noemi Erli[:noemi_erli]]

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

Comment 11

[Marius Coman [:mcoman], Ecosystem QA]

This enhancement was part of the work done on the QA-1801 request. Considering this I am marking it as Verified.