1621696 - Windows Default Browser Agent - Phase 2 - Windows 10 toast notification API integration

Status: RESOLVED FIXED

(Toolkit :: Default Browser Agent, enhancement)

Description

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

This ticket tracks implementing the mechanics of notification display for the Windows Default Browser Agent (phase 2): creating and displaying Windows 10 toast notifications, registering and reacting to actions, etc.

There's a nice single-file wrapper library around this functionality that we might choose to incorporate: https://github.com/mohabouje/WinToast.

Comment 1

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

See Bug 1607833 for the notification designs.

Comment 2

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

One technically new thing that this will require is a registered COM server to react to actions when the agent is not running. This requires a unique COM CLSID, which will need to be carefully chosen to allow multiple Firefox installations to coincide.

Comment 3

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Just capturing some details: there's a Win32/legacy issue where falling back to the App icon looks horrific: https://github.com/MicrosoftDocs/windows-uwp/issues/1409. It's been closed without being fixed. See also https://stackoverflow.com/q/34965325 which shows the "default" horror, but be aware that the blue backdrop is user specified and could be anything.

It's possible to specify our own image, and https://docs.microsoft.com/en-us/windows/uwp/design/shell/tiles-and-notifications/adaptive-interactive-toasts#app-logo-override suggests that these should be 48x48. We in fact have that size in the tree already (used for branding on Linux).

Unfortunately our own image changes the visual layout to include an App Identifier string, screenshot to follow.

Comment 4

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Attachment: Screen Shot 2020-03-12 at 11.32.13 AM.png

Note the "Console WinToast example" string -- that could be whatever we wanted (say, &brandShortName) but would be fixed across all notifications. (I believe it's a property of the App/executable, in this case default-browser-agent.exe.)

Comment 5

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Nick Alexander :nalexander [he/him] from comment #2)

One technically new thing that this will require is a registered COM server to react to actions when the agent is not running. This requires a unique COM CLSID, which will need to be carefully chosen to allow multiple Firefox installations to coincide.

The approach I am currently taking for this in the BITS service (bug 1515450) is to randomly generate a CLSID, and then point to it with a ProgID based on the path hash (app user model id), something like MozillaBITS6F193CCC56814779.BCM.1. The CLSID can then be retrieved with CLSIDFromProgID by the client or server as long as they know the path hash. We could do this with one of our own registry entries instead, but it seems reasonable to use this builtin indirection mechanism.

Comment 6

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Vendor WinToast library. r=froydnj,mhoye

The WinToast library is licensed MIT.

This version was vendored via:
wget https://raw.githubusercontent.com/mohabouje/WinToast/09227c72f16ccefc36e9d430dea3b435346dbcbc/LICENSE.txt
wget https://raw.githubusercontent.com/mohabouje/WinToast/09227c72f16ccefc36e9d430dea3b435346dbcbc/src/wintoastlib.cpp
wget https://raw.githubusercontent.com/mohabouje/WinToast/09227c72f16ccefc36e9d430dea3b435346dbcbc/src/wintoastlib.h

Comment 7

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Disable building WinToast on mingw r?#firefox-build-system-reviewers

In order to be compatible without our build system, WinToast should use lowercase names for system libraries and should not attempt to build at all on mingw, with which it is not entirely compatible.

Depends on D73951

Comment 8

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Generalize generate_updater_ini.py for more use cases r=Pike,froydnj

The Default Browser Agent needs localization, but is external from Firefox. The best option is to do localization the same way the updater does, since it is also external from Firefox. This requires making the tools surrounding updater.ini a bit more generic so they can be reused for the Default Browser Agent.

Depends on D73953

Comment 9

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Add localization for the default browser agent r=Pike,froydnj

Depends on D73954

Comment 10

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Make sure the default browser agent rebuilds when the resources or manifest change r=mhowell

Depends on D73955

Comment 11

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Record the current and previous default browsers, even if telemetry is disabled r=mhowell

Depends on D73956

Comment 12

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Change the default browser agent to use enums, where possible r=mhowell

The notifications will need to distinguish among the default browser values detected. I'd rather check against enums than check against string values so the compiler can catch typos.

Depends on D73958

Comment 13

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Expose LaunchModernSettingsDialogDefaultApps such that it can be called without libxul r=Mossop

Depends on D73960

Comment 14

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Show toast notification when default browser changes from Firefox to Edge with the Blink engine r=mhowell,agashlin,nalexander

Depends on D73962

Comment 15

[Robin Steuber (she/her) [:bytesized]]

Attachment: notification.png

Comment 16

[Robin Steuber (she/her) [:bytesized]]

Attachment: triple_text_notification.png

The same notification with three times as much text in each field.

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #16)

Created attachment 9146241 [details]
triple_text_notification.png

The same notification with three times as much text in each field.

Unfortunately this looks quite bad. It's basically safe to assume that most of our localizations will get truncated labels for the button.

Comment 18

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

It seems possible to add a newline to a button by using CR or LF (
 or 
) in the content string, at least in my playing around with Notifications Visualizer. This would be awkward for localization but should at least make it possible to use longer strings. If there's a way to wrap automatically I haven't found it yet.

Comment 19

[Robin Steuber (she/her) [:bytesized]]

Attachment: notification_on_RTL.PNG

This is the notification displayed with Left-to-Right text on a Right-to-Left operating system (Arabic)

Comment 20

[Francesco Lodolo [:flod]]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #18)

It seems possible to add a newline to a button by using CR or LF (
 or 
) in the content string, at least in my playing around with Notifications Visualizer. This would be awkward for localization but should at least make it possible to use longer strings.

Unfortunately I don't think awkward describes the situation accurately. This would be incredibly error prone:

• Someone needs to test it, and realize that they need a new line. Otherwise they'll ship a button that doesn't make any sense for the user.
• Add a new line with a format that is not used anywhere else in the codebase (\n is normally used in these case, and that's still confusing).

One the first point, a couple of examples (guessing what German and French could try to use):

• Italian: Imposta Firefox come browser predefinito… would likely show up as the equivalent of Make Firefox as b
• German: (guessing) Firefox als Standardbrowser festlegen -> Firefox als Standardbro (Firefox as standardwhat??, no verb)
• French: (guessin) Faire de Firefox mon navigateur par défaut -> Faire de Firefox mon (Make Firefox my)

That doesn't scale for 96 languages, or even for the ones we care more about, given there's no testing automation (e.g. automated screenshots).

Comment 21

[Axel Hecht [:Pike]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #19)

Created attachment 9146362 [details]
notification_on_RTL.PNG

This is the notification displayed with Left-to-Right text on a Right-to-Left operating system (Arabic)

Thanks for putting this up here. Confirms my suspicion that we'll need to find a way to set the directionality of the UI. I don't know enough about the underlying windows APIs to suggest how.

Comment 22

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

dveditz: it was suggested that the newly imported third-party WinToast library might want sec review and/or fuzzing. (It's the first commit in the attached Phab series, if you'd like to skim yourself.) Looking at the sec review policy at https://wiki.mozilla.org/Security/Reviews/Secure_Development_Lifecycle, it looks much more focused on feature review rather than on the implementation details of third-party code, but can you provide a first pass and redirect us? Thanks!

Comment 23

[Robin Steuber (she/her) [:bytesized]]

Attachment: notification_updated.png

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:bytesized, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 25

[Robin Steuber (she/her) [:bytesized]]

Yeah, these patches are waiting for some other work to be ready before they an merge.

Comment 26

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1621696 - Suppress the followup notification if the default browser has been changed r=mhowell,agashlin,nalexander

Comment 27

[Tom Ritter [:tjr]]

Can we file a followup bug blocking mingw-clang to address the WinToast thing? I'm not sure what level of confidentiality is needed/desired here so I don't want to file it wrong.

Comment 28

[Robin Steuber (she/her) [:bytesized]]

I'm not entirely sure what you want to be addressed. Do you want this feature to work on mingw?

Comment 29

[Tom Ritter [:tjr]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #28)

I'm not entirely sure what you want to be addressed. Do you want this feature to work on mingw?

I'm guessing that the feature itself is controlled by a pref, right? In that case, we'd like to be compiling all the relevant code and Tor Browser will probably just flip the pref off, but might customize the functionality and use it in the future.

Comment 30

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I'm guessing that the feature itself is controlled by a pref, right?

There is a pref, and a policy, but this comes with a separate process running as a scheduled task on Windows, set up by the installer / post-update. With the pref off, the task still runs, to check the pref. I'm just guessing, but it seems more likely that Tor Browser would want to configure it disabled at build time.

It's still probably a good idea to have a bug on file for the mingw build failure, though.

Comment 31

[Razvan Maries]

Merged to central:
https://hg.mozilla.org/mozilla-central/rev/15e6e5554039f45271222226c58215eaff725257
https://hg.mozilla.org/mozilla-central/rev/570afd3a9fe9af6d9f84a9df272cfc688caf6bf9
https://hg.mozilla.org/mozilla-central/rev/ad51b2d2eedd21924b4bb65d993948cfebe622d7
https://hg.mozilla.org/mozilla-central/rev/01b470cc5910c308fc708641572f5900d9f7b067
https://hg.mozilla.org/mozilla-central/rev/e60871c6c19241feebc75f3c5b4090d2fd944ce9
https://hg.mozilla.org/mozilla-central/rev/578b28bc04ea6ebbad2f42dd815cb81ca4189c86
https://hg.mozilla.org/mozilla-central/rev/66711d6a17b2c68943280c01e6a4d54525fd60d4
https://hg.mozilla.org/mozilla-central/rev/e8e6884a16b295971e48d8d9c922eeb366cc3e68
https://hg.mozilla.org/mozilla-central/rev/d573712ab88e95aad88e605a3095c6ede40db9fb
https://hg.mozilla.org/mozilla-central/rev/392bc8ad6fea1953416105d7b594236ed5b87527

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/15e6e5554039f45271222226c58215eaff725257
https://hg.mozilla.org/integration/autoland/rev/570afd3a9fe9af6d9f84a9df272cfc688caf6bf9
https://hg.mozilla.org/integration/autoland/rev/ad51b2d2eedd21924b4bb65d993948cfebe622d7
https://hg.mozilla.org/integration/autoland/rev/01b470cc5910c308fc708641572f5900d9f7b067
https://hg.mozilla.org/integration/autoland/rev/e60871c6c19241feebc75f3c5b4090d2fd944ce9
https://hg.mozilla.org/integration/autoland/rev/578b28bc04ea6ebbad2f42dd815cb81ca4189c86
https://hg.mozilla.org/integration/autoland/rev/66711d6a17b2c68943280c01e6a4d54525fd60d4
https://hg.mozilla.org/integration/autoland/rev/e8e6884a16b295971e48d8d9c922eeb366cc3e68
https://hg.mozilla.org/integration/autoland/rev/d573712ab88e95aad88e605a3095c6ede40db9fb
https://hg.mozilla.org/integration/autoland/rev/392bc8ad6fea1953416105d7b594236ed5b87527