Implement ToastNotification for Action Center (Notification Center) on Windows 10

RESOLVED FIXED in Firefox 64

Status

()

P2
normal
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: m_kato, Assigned: m_kato, NeedInfo)

Tracking

(Depends on: 5 bugs, Blocks: 5 bugs, {feature, parity-chrome})

Trunk
mozilla64
All
Windows 10
feature, parity-chrome
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox -, firefox64 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

4 years ago
We had this on MetroFox, but no Desktop version.

Also, Toast Notification supports from Windows 8+.
(Assignee)

Updated

4 years ago
OS: Windows 8.1 → Windows 10
Priority: -- → P4

Comment 1

4 years ago
[15:21:56]	MattN	https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=783232&attachment=786916
[15:24:04]	Gijs	MattN: that looks usable...
[15:25:01]	MattN	Gijs: FWIW, that ideally should have implemented the system-alerts-service instead of hacking around it
(Assignee)

Comment 2

4 years ago
Posted patch WIP (obsolete) — Splinter Review
(Assignee)

Comment 3

4 years ago
Toast API for Windows 8 / 8.1 only supports ms-appx schema image url.  But I don't support it on Windows 10, so I need investigate that image url supports data schema.

Comment 4

4 years ago
There's also bug 852917 for Windows 8
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1214125
(Assignee)

Comment 6

4 years ago
On Desktop mode, image url only support local and ms-appx schema even if Windows 10...

https://msdn.microsoft.com/en-us/library/windows/apps/hh779727.aspx

So to use image url, I have to store image to local and use it.
(Assignee)

Updated

4 years ago
Assignee: nobody → m_kato

Comment 7

3 years ago
Any update or new details on this?

Apparently Microsoft has announced that Edge will support web notifications in Action Center.

According to 
https://www.netmarketshare.com/operating-system-market-share.aspx?qprid=10&qpcustomd=0
Windows 10 is currently at 14.15% market share (and Windows 8 at 9.56%) though I expect that number to rise a good bit before the "free" period ends in July.

We might consider this bug "Edge Parity" :)
Posted patch bug-1155505.patch (obsolete) — Splinter Review
Here's the patch rebased to today's codebase.
I'm unable to show a remote image, but it seems like if I use a file:/// URL for the image it works.
Could someone mentor me on this?
See Also: → bug 488566

Comment 9

2 years ago
(In reply to Edouard Oger [:eoger] from comment #8)
> Created attachment 8903819 [details] [diff] [review]
> bug-1155505.patch
> 
> Here's the patch rebased to today's codebase.
> I'm unable to show a remote image, but it seems like if I use a file:/// URL
> for the image it works.
> Could someone mentor me on this?

Jim, do you know of someone who could help mentor Edouard?
Flags: needinfo?(jmathies)
(In reply to Edouard Oger [:eoger] from comment #8)
> Created attachment 8903819 [details] [diff] [review]
> bug-1155505.patch
> 
> Here's the patch rebased to today's codebase.
> I'm unable to show a remote image, but it seems like if I use a file:/// URL
> for the image it works.
> Could someone mentor me on this?

Why would you want to display a remote image in a native toast? Can you give some background on ow this works?
Flags: needinfo?(jmathies)
The interface nsIAlertsService allows callers to specify an image [0] to show next to the notification text.

[0] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/toolkit/components/alerts/nsIAlertsService.idl#192
(Assignee)

Comment 12

2 years ago
My understand is that image url of Windows Toast Notification API doesn't support data url. (Of courses, chrome:// too)  If using http, it uses WinInet API, not our HTTP stack.  So we should get image by our API like GTK's implementation, then copy to temp directory to show notification image.
However that notification stays in the notification center, that means we can't delete the image right away after showing the notification right?
(Assignee)

Comment 14

2 years ago
(In reply to Edouard Oger [:eoger] from comment #13)
> However that notification stays in the notification center, that means we
> can't delete the image right away after showing the notification right?

If toast only, we might be able to remove it on dismiss event.  Also windows has MOVEFILE_DELAY_UNTIL_REBOOT flag to remove file for action center on badge.
Posted patch bug-1155505.patch (obsolete) — Splinter Review
I have images working. I used a technique similar to what the Gnome implementation does.
Attachment #8903819 - Attachment is obsolete: true
Attachment #8906874 - Flags: feedback?(m_kato)
Feel free to change the f? flag!
Posted patch bug-1155505.patch (obsolete) — Splinter Review
I managed to pull WriteBitmap in WinUtils.
Attachment #8906874 - Attachment is obsolete: true
Attachment #8906874 - Flags: feedback?(m_kato)
Attachment #8907365 - Flags: feedback?(m_kato)
Duplicate of this bug: 488566
Duplicate of this bug: 852917
Posted patch bug-1155505.patch (obsolete) — Splinter Review
Now without main-thread IO!
However, it looks a bit awkward because I realized too late that imgIContainer->GetFrame() can only be called from the main-thread.
I think the proper way to do this is to add a WriteBitmapAsync method in WinUtils that takes a callback interface (say WriteBitmapAsyncCb) and an nsIThread as arguments.
Assignee: m_kato → eoger
Attachment #8632672 - Attachment is obsolete: true
Attachment #8907365 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8907365 - Flags: feedback?(m_kato)
Comment hidden (mozreview-request)
Attachment #8911427 - Attachment is obsolete: true
+ Minor adjustments.
This is ready for a round of review.

On a local build, "taskbar.grouping.useprofile" is needed otherwise GetAppUserModelID will fail, I wonder if we should use a default value sometimes?
(Assignee)

Comment 23

2 years ago
I will look this review this week.  But this will be also required by UX team review.
(Assignee)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8911505 [details]
Bug 1155505 - Use native Window Toasts as a notification backend

https://reviewboard.mozilla.org/r/175578/#review190856

We shouldn't native toast as default.  Becasue I think that native toast is lack of features such as options.  So I think that we should add prefs for this to enable/disable it.

::: browser/components/shell/nsWindowsShellService.cpp:47
(Diff revision 1)
>  #define INITGUID
>  #undef NTDDI_VERSION
>  #define NTDDI_VERSION NTDDI_WIN8
>  // Needed for access to IApplicationActivationManager
>  #include <shlobj.h>
> +#include "WinUtils.h"

Why this #include is this postion?.  It should add it to previous #include block.

::: widget/windows/ToastNotification.cpp:31
(Diff revision 1)
> +ToastNotification::~ToastNotification()
> += default;
> +
> +nsresult
> +ToastNotification::Init()
> +{

Our XUL alert has some features such as opening option doorhanger.  But Windows toast implementation cannot implement it better even if using Windows 10 new API.

So I think we should add prefs to enable nativ toast and default is off.

::: widget/windows/ToastNotification.cpp:56
(Diff revision 1)
> +  return mBackgroundThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
> +}
> +
> +NS_IMETHODIMP
> +ToastNotification::Observe(nsISupports *aSubject, const char *aTopic,
> +                                  const char16_t *aData) {

nit: adjust space of parameter

::: widget/windows/ToastNotification.cpp:80
(Diff revision 1)
> +                                         bool aInPrivateBrowsing,
> +                                         bool aRequireInteraction)
> +{
> +  nsCOMPtr<nsIAlertNotification> alert =
> +    do_CreateInstance(ALERT_NOTIFICATION_CONTRACTID);
> +  NS_ENSURE_TRUE(alert, NS_ERROR_FAILURE);

Use if (NS_WARN_IF(!alert)) instead.

::: widget/windows/ToastNotification.cpp:86
(Diff revision 1)
> +  nsresult rv = alert->Init(aAlertName, aImageUrl, aAlertTitle,
> +                            aAlertText, aAlertTextClickable,
> +                            aAlertCookie, aBidi, aLang, aData,
> +                            aPrincipal, aInPrivateBrowsing,
> +                            aRequireInteraction);
> +  NS_ENSURE_SUCCESS(rv, rv);

Use if (NS_WARN_IF(NS_FAILED(rv)) instead.

::: widget/windows/ToastNotification.cpp:168
(Diff revision 1)
> +  // The alert may have been replaced; only remove it from the active
> +  // handlers map if it's the same.
> +  if (IsActiveHandler(aAlertName, aHandler)) {
> +    // Terrible things happen if the destructor of a handler is called inside
> +    // the hashtable .Remove() method. Wait until we have returned from there.
> +    NS_ADDREF(aHandler);

Use RefPtr<ToastNotiificationHandler> kungFuDeathGrip(aHandler) instead of NS_ADD_REF/NS_RELEASE.

::: widget/windows/ToastNotificationHandler.cpp:48
(Diff revision 1)
> +static nsresult
> +SetNodeValueString(const nsAString& aString,
> +                   IXmlNode* node,
> +                   IXmlDocument* xml)
> +{
> +  ComPtr<IXmlText> inputText;

Why do you use ComPtr?  I guess thst we can use mozilla::RefPtr

::: widget/windows/ToastNotificationHandler.cpp:56
(Diff revision 1)
> +
> +  hr = xml->CreateTextNode(value, &inputText);
> +  if (FAILED(hr)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  ComPtr<IXmlNode> inputTextNode;

Same

::: widget/windows/ToastNotificationHandler.cpp:61
(Diff revision 1)
> +  ComPtr<IXmlNode> inputTextNode;
> +  hr = inputText.As(&inputTextNode);
> +  if (FAILED(hr)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  ComPtr<IXmlNode> appendedChild;

Same.

::: widget/windows/ToastNotificationHandler.cpp:70
(Diff revision 1)
> +  }
> +
> +  return NS_OK;
> +}
> +
> +static ComPtr<IToastNotificationManagerStatics>

Same

::: widget/windows/ToastNotificationHandler.cpp:73
(Diff revision 1)
> +}
> +
> +static ComPtr<IToastNotificationManagerStatics>
> +GetToastNotificationManagerStatics()
> +{
> +  ComPtr<IToastNotificationManagerStatics> toastNotificationManagerStatics;

Same

::: widget/windows/ToastNotificationHandler.cpp:193
(Diff revision 1)
> +  SetNodeValueString(mMsg, msgTextNodeRoot.Get(), toastXml.Get());
> +
> +  return CreateWindowsNotificationFromXml(toastXml.Get(), mClickable);
> +}
> +
> +nsresult

Use bool since this method uses NS_OK and NS_ERROR_FAILURE only

::: widget/windows/ToastNotificationHandler.cpp:329
(Diff revision 1)
> +
> +nsresult
> +ToastNotificationHandler::AsyncSaveImage(imgIRequest* aRequest)
> +{
> +  nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR,
> +                                       getter_AddRefs(mImageFile));

If toast is called twice at same timeing, is mImageFile override?

::: widget/windows/ToastNotificationHandler.cpp:330
(Diff revision 1)
> +nsresult
> +ToastNotificationHandler::AsyncSaveImage(imgIRequest* aRequest)
> +{
> +  nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR,
> +                                       getter_AddRefs(mImageFile));
> +  NS_ENSURE_SUCCESS(rv, rv);

Use NS_WARN_IF(NS_FAILED(rv))

::: widget/windows/ToastNotificationHandler.cpp:353
(Diff revision 1)
> +  char uuidChars[NSID_LENGTH];
> +  uuid.ToProvidedString(uuidChars);
> +  // Remove the brackets at the beginning and ending of the generated UUID.
> +  nsAutoCString uuidStr(Substring(uuidChars + 1, uuidChars + NSID_LENGTH - 2));
> +  uuidStr.AppendLiteral(".bmp");
> +  mImageFile->AppendNative(uuidStr);

When is this file (mImageFile) removed?

::: widget/windows/ToastNotificationHandler.cpp:381
(Diff revision 1)
> +      }
> +
> +      nsCOMPtr<nsIRunnable> cbRunnable = NS_NewRunnableFunction(
> +        "ToastNotificationHandler::AsyncWriteBitmapCb",
> +        [self, rv]() -> void {
> +          auto s = const_cast<ToastNotificationHandler*>(self.get());

s isn't better name.  You change name to handler.
Attachment #8911505 - Flags: review?(m_kato) → review-
(Assignee)

Comment 25

2 years ago
:jaws, does this require UX team review?  Windows 8+ has native toast notification, but it is no way to implement options such as macOS notification center and XUL alert.  Also, Windows 10 can add additional button like [Yes] or [No], but no good UI for it.
Flags: needinfo?(jaws)
Thanks a lot for the review Makoto, I'll send an amended patch soon.

> We shouldn't native toast as default.  Because I think that native toast is lack of features such as options.  So I think that we should add prefs for this to enable/disable it.

In my opinion we should use what the OS provides us.
I'd like not to rush this and let UX make that call if possible.
The Windows native OS notifications are missing UI features that would be necessary to support our do-not-disturb option menu as well as a link to the notification preferences.

We could land this disabled behind a pref until we can figure out how to get those missing features enabled. At that point we could put a checkbox in the preferences to allow the user to choose between the two implementations. Both implementation provides different benefits (platform integration with the native alerts, cross-platform uniform looks with the Firefox alerts).
Flags: needinfo?(jaws)
> The Windows native OS notifications are missing UI features that would be necessary to support our do-not-disturb option menu as well as a link to the notification preferences.

I wonder if we could add these two buttons, see https://docs.microsoft.com/en-us/windows/uwp/controls-and-patterns/tiles-and-notifications-adaptive-interactive-toasts
Yeah that would be great! The UI features would be the buttons that are hidden behind the "gear" icon. We would also want to hid these buttons behind some type of "options" button as well, if possible.

Comment 30

2 years ago
mozreview-review-reply
Comment on attachment 8911505 [details]
Bug 1155505 - Use native Window Toasts as a notification backend

https://reviewboard.mozilla.org/r/175578/#review190856

> Why do you use ComPtr?  I guess thst we can use mozilla::RefPtr

I don't know how to reproduce ComPtr::As with RefPtr.

> If toast is called twice at same timeing, is mImageFile override?

I dont't see why it should since these are two different instances of ToastNotificationHandler.

> When is this file (mImageFile) removed?

This file is removed in ~ToastNotificationHandler

Comment 31

2 years ago
mozreview-review-reply
Comment on attachment 8911505 [details]
Bug 1155505 - Use native Window Toasts as a notification backend

https://reviewboard.mozilla.org/r/175578/#review190856

> Why this #include is this postion?.  It should add it to previous #include block.

I tried moving it up, but it also needs IApplicationActivationManager
After talking with Shorlander, he found that Windows 10 has custom context menu items since the Anniversary Update [0], so we're going to use that.
The patch I just submitted does that, with 2 example items (Snooze/Settings).

However I'm having a problem handling these new actions: it seems that the Activated handler is not called when click on the custom context menu items.
This handler is called correctly with regular buttons (just remove the SetNodeAttr(placement, context menu) in AddActionNode to test this), so I'm thinking this might be a bug. Can we ping someone from Microsoft to get some info here?

[0] https://blogs.msdn.microsoft.com/tiles_and_toasts/2016/06/10/whats-new-for-toast-notifications-and-action-center-in-windows-10-anniversary-update/

Comment 33

a year ago
Any update on this? Would be nice to be able to use given the upcoming changes to the notification center in windows (Focus Assist).
We have reached out to Microsoft last December regarding the issue I described in comment 32 and are awaiting an answer.

Comment 35

a year ago
Oof, well that's been at least two months. 
Wonder how many months is in the not-badgering category for a re-ping.
Comment hidden (mozreview-request)
Posting the last iteration of my patch (you still need "taskbar.grouping.useprofile":true).

After having a quick look at the Chrome integration, I can't help but think that my integration with the Windows API is somewhat brittle (e.g. we don't handle activation events when the application is not running). I also wouldn't be surprised that this is the cause of the bug I encountered in comment 32.

Unfortunately, I don't have more time to work on this (I'm in the Sync team) and this was more like a "20% project" that I started from frustration.
I'm un-assigning myself so someone with more time and experience with the Windows APIs can take this to the finish line.

Neil, would it be possible to re-triage this and consider promoting this from P4? For a seasoned Windows developer the work wouldn't be too extensive I think.
Assignee: eoger → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(enndeakin)
(Assignee)

Comment 39

a year ago
mozreview-review
Comment on attachment 8911505 [details]
Bug 1155505 - Use native Window Toasts as a notification backend

https://reviewboard.mozilla.org/r/175578/#review233034

Total, OK.  But could you fix Mingw32 build error? (https://treeherder.mozilla.org/#/jobs?repo=try&author=m_kato@ga2.so-net.ne.jp&selectedJob=167560767)

Mingw32 doesn't support WinRT API, so you can use __MINGW32__ to disable all.

::: widget/windows/ToastNotificationHandler.cpp:53
(Diff revision 2)
> +  ComPtr<IXmlText> inputText;
> +  HRESULT hr;
> +  HSTRING value = HStringReference(reinterpret_cast<const wchar_t*>(aString.BeginReading())).Get();
> +
> +  hr = xml->CreateTextNode(value, &inputText);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:58
(Diff revision 2)
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +  ComPtr<IXmlNode> inputTextNode;
> +  hr = inputText.As(&inputTextNode);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:63
(Diff revision 2)
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +  ComPtr<IXmlNode> appendedChild;
> +  hr = node->AppendChild(inputTextNode.Get(), &appendedChild);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:74
(Diff revision 2)
> +
> +static bool
> +SetAttribute(IXmlElement* element, const nsAString& name, const nsAString& value) {
> +  HSTRING nameStr = HStringReference(reinterpret_cast<const wchar_t*>(name.BeginReading())).Get();
> +  HSTRING valueStr = HStringReference(reinterpret_cast<const wchar_t*>(value.BeginReading())).Get();
> +  if (FAILED(element->SetAttribute(nameStr, valueStr))) {

if (NS_WARN_IF(FAILED(...))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:86
(Diff revision 2)
> +AddActionNode(IXmlDocument* toastXml, IXmlNode* actionsNode, const nsAString& actionTitle, const nsAString& actionArgs) {
> +  ComPtr<IXmlElement> action;
> +  toastXml->CreateElement(HStringReference(L"action").Get(), &action);
> +
> +  HRESULT hr = SetAttribute(action.Get(), NS_LITERAL_STRING("content"), actionTitle);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:90
(Diff revision 2)
> +  HRESULT hr = SetAttribute(action.Get(), NS_LITERAL_STRING("content"), actionTitle);
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +  hr = SetAttribute(action.Get(), NS_LITERAL_STRING("arguments"), actionArgs);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:94
(Diff revision 2)
> +  hr = SetAttribute(action.Get(), NS_LITERAL_STRING("arguments"), actionArgs);
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +  hr = SetAttribute(action.Get(), NS_LITERAL_STRING("placement"), NS_LITERAL_STRING("contextmenu"));
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:101
(Diff revision 2)
> +  }
> +
> +  // Add <action> to <actions>
> +  ComPtr<IXmlNode> actionNode;
> +  hr = action.As(&actionNode);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:107
(Diff revision 2)
> +    return false;
> +  }
> +
> +  ComPtr<IXmlNode> appendedChild;
> +  hr = actionsNode->AppendChild(actionNode.Get(), &appendedChild);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:122
(Diff revision 2)
> +  ComPtr<IToastNotificationManagerStatics> toastNotificationManagerStatics;
> +  HRESULT hr;
> +  hr = GetActivationFactory(
> +         HStringReference(RuntimeClass_Windows_UI_Notifications_ToastNotificationManager).Get(),
> +         toastNotificationManagerStatics.GetAddressOf());
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:191
(Diff revision 2)
> +
> +  if (mHasImage) {
> +    ComPtr<IXmlNodeList> toastImageElements;
> +    hr = toastXml->GetElementsByTagName(HStringReference(L"image").Get(),
> +                                        toastImageElements.GetAddressOf());
> +    if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:196
(Diff revision 2)
> +    if (FAILED(hr)) {
> +      return false;
> +    }
> +    ComPtr<IXmlNode> imageNode;
> +    hr = toastImageElements->Item(0, &imageNode);
> +    if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:201
(Diff revision 2)
> +    if (FAILED(hr)) {
> +      return false;
> +    }
> +    ComPtr<IXmlElement> image;
> +    hr = imageNode.As(&image);
> +    if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:205
(Diff revision 2)
> +    hr = imageNode.As(&image);
> +    if (FAILED(hr)) {
> +      return false;
> +    }
> +    hr = SetAttribute(image.Get(), NS_LITERAL_STRING("src"), mImageUri);
> +    if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:213
(Diff revision 2)
> +  }
> +
> +  ComPtr<IXmlNodeList> toastTextElements;
> +  hr = toastXml->GetElementsByTagName(HStringReference(L"text").Get(),
> +                                      toastTextElements.GetAddressOf());
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:219
(Diff revision 2)
> +    return false;
> +  }
> +
> +  ComPtr<IXmlNode> titleTextNodeRoot;
> +  hr = toastTextElements->Item(0, &titleTextNodeRoot);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:224
(Diff revision 2)
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +  ComPtr<IXmlNode> msgTextNodeRoot;
> +  hr = toastTextElements->Item(1, &msgTextNodeRoot);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:234
(Diff revision 2)
> +  SetNodeValueString(mMsg, msgTextNodeRoot.Get(), toastXml.Get());
> +
> +  ComPtr<IXmlNodeList> toastElements;
> +  hr = toastXml->GetElementsByTagName(HStringReference(L"toast").Get(),
> +                                      toastElements.GetAddressOf());
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:240
(Diff revision 2)
> +    return false;
> +  }
> +
> +  ComPtr<IXmlNode> toastNodeRoot;
> +  hr = toastElements->Item(0, &toastNodeRoot);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:249
(Diff revision 2)
> +  ComPtr<IXmlElement> actions;
> +  toastXml->CreateElement(HStringReference(L"actions").Get(), &actions);
> +
> +  ComPtr<IXmlNode> actionsNode;
> +  hr = actions.As(&actionsNode);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:253
(Diff revision 2)
> +  hr = actions.As(&actionsNode);
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +
> +  AddActionNode(toastXml.Get(), actionsNode.Get(), NS_LITERAL_STRING("Snooze"), NS_LITERAL_STRING("snooze=1"));

Does this support localization?

::: widget/windows/ToastNotificationHandler.cpp:254
(Diff revision 2)
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +
> +  AddActionNode(toastXml.Get(), actionsNode.Get(), NS_LITERAL_STRING("Snooze"), NS_LITERAL_STRING("snooze=1"));
> +  AddActionNode(toastXml.Get(), actionsNode.Get(), NS_LITERAL_STRING("Settings"), NS_LITERAL_STRING("settings=1"));

same

::: widget/windows/ToastNotificationHandler.cpp:258
(Diff revision 2)
> +  AddActionNode(toastXml.Get(), actionsNode.Get(), NS_LITERAL_STRING("Snooze"), NS_LITERAL_STRING("snooze=1"));
> +  AddActionNode(toastXml.Get(), actionsNode.Get(), NS_LITERAL_STRING("Settings"), NS_LITERAL_STRING("settings=1"));
> +
> +  ComPtr<IXmlNode> appendedChild;
> +  hr = toastNodeRoot->AppendChild(actionsNode.Get(), &appendedChild);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:274
(Diff revision 2)
> +  ComPtr<IToastNotificationFactory> factory;
> +  HRESULT hr;
> +  hr = GetActivationFactory(
> +         HStringReference(RuntimeClass_Windows_UI_Notifications_ToastNotification).Get(),
> +         factory.GetAddressOf());
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:279
(Diff revision 2)
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +
> +  hr = factory->CreateToastNotification(aXml, mNotification.GetAddressOf());
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:287
(Diff revision 2)
> +
> +  hr = mNotification->add_Activated(
> +         Callback<ToastActivationHandler>(
> +           this, &ToastNotificationHandler::OnActivate).Get(),
> +         &mActivatedToken);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:295
(Diff revision 2)
> +
> +  hr = mNotification->add_Dismissed(
> +         Callback<ToastDismissedHandler>(
> +           this, &ToastNotificationHandler::OnDismiss).Get(),
> +         &mDismissedToken);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:303
(Diff revision 2)
> +
> +  hr = mNotification->add_Failed(
> +         Callback<ToastFailedHandler>(
> +           this, &ToastNotificationHandler::OnFail).Get(),
> +         &mFailedToken);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:320
(Diff revision 2)
> +    return false;
> +  }
> +
> +  HSTRING uidStr = HStringReference(reinterpret_cast<const wchar_t*>(uid.BeginReading())).Get();
> +  hr = toastNotificationManagerStatics->CreateToastNotifierWithId(uidStr, &mNotifier);
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:325
(Diff revision 2)
> +  if (FAILED(hr)) {
> +    return false;
> +  }
> +
> +  hr = mNotifier->Show(mNotification.Get());
> +  if (FAILED(hr)) {

if (NS_WARN_IF(FAILED(hr))) {
 ...

::: widget/windows/ToastNotificationHandler.cpp:409
(Diff revision 2)
> +                                       getter_AddRefs(mImageFile));
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  rv = mImageFile->AppendNative(NS_LITERAL_CSTRING("notificationimages"));

Use Append instead of AppendNative to avoid conversion cost

::: widget/windows/WinUtils.h:38
(Diff revision 2)
>  #include "nsIWidget.h"
>  #include "nsIThread.h"
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/EventForwards.h"
> +#include "mozilla/gfx/2D.h"

Don't include 2D.h in header

::: widget/windows/WinUtils.h:114
(Diff revision 2)
>  #if defined(ACCESSIBILITY)
>  namespace a11y {
>  class Accessible;
>  } // namespace a11y
>  #endif // defined(ACCESSIBILITY)
>  

```
namespace gfx {
  class SourceSurface;
} // namespace gfx
```

::: widget/windows/WinUtils.cpp
(Diff revision 2)
>  #include "nsWindow.h"
>  #include "nsWindowDefs.h"
>  #include "KeyboardLayout.h"
>  #include "nsIDOMMouseEvent.h"
>  #include "mozilla/ArrayUtils.h"
> -#include "mozilla/gfx/2D.h"

Don't remove this line

::: widget/windows/nsWidgetFactory.cpp:255
(Diff revision 2)
>    { "@mozilla.org/windows-jumplistlink;1", &kNS_WIN_JUMPLISTLINK_CID },
>    { "@mozilla.org/windows-jumplistshortcut;1", &kNS_WIN_JUMPLISTSHORTCUT_CID },
>    { "@mozilla.org/windows-ui-utils;1", &kNS_WINDOWS_UIUTILS_CID },
>    { "@mozilla.org/widget/dragservice;1", &kNS_DRAGSERVICE_CID, Module::MAIN_PROCESS_ONLY },
>    { "@mozilla.org/widget/bidikeyboard;1", &kNS_BIDIKEYBOARD_CID, Module::MAIN_PROCESS_ONLY },
> +  { NS_SYSTEMALERTSERVICE_CONTRACTID, &kNS_SYSTEMALERTSSERVICE_CID },

Add Module::MAIN_PROCESS_ONLY
Attachment #8911505 - Flags: review?(m_kato)
(Assignee)

Comment 40

a year ago
Even if MSVC build, the following build error occcurs.

01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(515): error C2220: warning treated as error - no 'object' file generated
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(514): note: while compiling class template member function 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::SemaphoreTraits>::SyncLockWithStatusT(Microsoft::WRL::Wrappers::HandleTraits::HANDLENullTraits::Type,DWORD) throw()'
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(671): note: see reference to function template instantiation 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::SemaphoreTraits>::SyncLockWithStatusT(Microsoft::WRL::Wrappers::HandleTraits::HANDLENullTraits::Type,DWORD) throw()' being compiled
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(664): note: see reference to class template instantiation 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::SemaphoreTraits>' being compiled
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(515): warning C5038: data member 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::SemaphoreTraits>::sync_' will be initialized after data member 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::SemaphoreTraits>::status_'
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(515): note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(515): warning C5038: data member 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::MutexTraits>::sync_' will be initialized after data member 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::MutexTraits>::status_'
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(515): note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(514): note: while compiling class template member function 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::MutexTraits>::SyncLockWithStatusT(Microsoft::WRL::Wrappers::HandleTraits::HANDLENullTraits::Type,DWORD) throw()'
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(633): note: see reference to function template instantiation 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::MutexTraits>::SyncLockWithStatusT(Microsoft::WRL::Wrappers::HandleTraits::HANDLENullTraits::Type,DWORD) throw()' being compiled
01:05:02     INFO -  z:\build\build\src\vs2017_15.4.2\SDK\Include\10.0.15063.0\winrt\wrl\wrappers\corewrappers.h(626): note: see reference to class template instantiation 'Microsoft::WRL::Wrappers::Details::SyncLockWithStatusT<Microsoft::WRL::Wrappers::HandleTraits::MutexTraits>' being compiled
01:05:02     INFO -  z:/build/build/src/config/rules.mk:1047: recipe for target 'ToastNotification.obj' failed

Updated

a year ago
Component: XUL Widgets → Notifications and Alerts
Flags: needinfo?(enndeakin)
I think this is something that should be worked on next when there are resources to work on Notifications. If the Windows platform integration team wants to work on this it would be greatly appreciated.
Priority: P4 → P2
Hardware: x86 → All
Comment hidden (advocacy)
Comment hidden (mozreview-request)
(Assignee)

Comment 45

a year ago
mozreview-review
Comment on attachment 8911505 [details]
Bug 1155505 - Use native Window Toasts as a notification backend

https://reviewboard.mozilla.org/r/175578/#review237462

::: widget/windows/ToastNotificationHandler.cpp:253
(Diff revision 3)
> +  hr = actions.As(&actionsNode);
> +  if (NS_WARN_IF(FAILED(hr))) {
> +    return false;
> +  }
> +
> +  AddActionNode(toastXml.Get(), actionsNode.Get(), NS_LITERAL_STRING("Snooze"), NS_LITERAL_STRING("snooze=1"));

Although I add previous review too, I think that we should add localization code like the following.  And add localization resource to /toolkit/locales/en-US/chrome/alerts/alert.properties.

```
nsCOMPtr<nsIStringBundleService> sbs = do_GetService(NS_STRINGBUNDLE_CONTRACTID);
nsCOMPtr<nsIStringBundle> bundle;
sbs->CreateBundle("chrome://alerts/locale/alert.properties", getter_AddRefs(bundle));
// XXX add error check

// [Snooze] localization
nsAutoString snoozeTitle;
bundle->GetStringFromName("webActions.snooze.label", snoozeButtonTitle);

AddActionNode(..., snoozeButtonTitle.get(), ...);

// [Settings] localization 
nsAutoString settingsButtonTitle;
bundle->GetStringFromName("webActions.settings.label", settingsButtonTitle);

AddActionNode(..., seetingsButtonTitle.get(), ...);
```
Attachment #8911505 - Flags: review?(m_kato)
Keywords: parity-chrome
(Assignee)

Comment 46

7 months ago
WriteBitmap will be used for Toast implementation, so it should be moved to WinUtils.
(Assignee)

Comment 47

7 months ago
Implemnt notification backend by Windows Toast API that is from Windows 8+.

Original patch is me and add some features by eoger.
(Assignee)

Comment 48

7 months ago
For feedback, I whould like to turn on native Windows Toast backend on
Nightly only.
(Assignee)

Updated

7 months ago
Attachment #8911505 - Attachment is obsolete: true
Comment on attachment 8998807 [details]
Bug 1155505 - Part 3. Use native Windows Toast backend on Nightly only

(away 8/20-8/27) Jared Wein [:jaws] (please needinfo? me) has approved the revision.
Attachment #8998807 - Flags: review+
Oh, it looks like it's waiting on review from Jim still. Review ping?
Flags: needinfo?(m_kato) → needinfo?(jmathies)
pfft, I've missed a bunch of reviews thanks to crappy phabricator integration with bugzilla. I'll get to this asap although it will probably be early next week.
Flags: needinfo?(jmathies)
Gentle review ping, jimm, if you have cycles. :-)
Flags: needinfo?(jmathies)
Comment on attachment 8998805 [details]
Bug 1155505 - Part 1. Move WriteBitmap to WinUtils. r?

Jim Mathies [:jimm] has approved the revision.
Attachment #8998805 - Flags: review+
Comment on attachment 8998806 [details]
Bug 1155505 - Part 2. Implement native Window Toasts as a notification backend. r?

Jim Mathies [:jimm] has approved the revision.
Attachment #8998806 - Flags: review+
Thanks!
Flags: needinfo?(jmathies)

Comment 58

6 months ago
Hi there I have now installed Nightly build 64.0a1 (2018-09-30), but haven't seen Windows native notification enabled. Do I have to check something in about:config? Thanks!
Flags: needinfo?(m_kato)
This bug isn't marked as FIXED yet so it's expected that it doesn't work yet.

Comment 61

6 months ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82e129f7545e
Part 1. Move WriteBitmap to WinUtils. r=jmathies
https://hg.mozilla.org/integration/mozilla-inbound/rev/5400ec20792c
Part 2. Implement native Window Toasts as a notification backend. r=jmathies
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5e706d5a143
Part 3. Use native Windows Toast backend on Nightly only. r=jaws

Comment 62

6 months ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af0781ab02a6
Part 4. Fix bustage for mingw32. r=me CLOSED TREE

Comment 63

6 months ago
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10b62fac5269
Backed out 4 changesets for win2012 build bustages CLOSED TREE
(Assignee)

Updated

6 months ago
Blocks: 1495998
(Assignee)

Comment 65

6 months ago
Ah, a Callback function of WRL's header doesn't add reference counter, so static analysis detects error.   I will use another Callback function that isn't use lamda...

Comment 67

6 months ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4caf1f44450a
Part 1. Move WriteBitmap to WinUtils. r=jmathies
https://hg.mozilla.org/integration/mozilla-inbound/rev/baf52bca95ae
Part 2. Implement native Window Toasts as a notification backend. r=jmathies
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0026439db7
Part 3. Use native Windows Toast backend on Nightly only. r=jaws

Comment 68

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4caf1f44450a
https://hg.mozilla.org/mozilla-central/rev/baf52bca95ae
https://hg.mozilla.org/mozilla-central/rev/4c0026439db7
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(Assignee)

Updated

6 months ago
Depends on: 1496650
Makoto, Do you think it is worth adding to the release notes?
Thanks
status-firefox40: affected → ---
Flags: needinfo?(m_kato)
Keywords: feature
QA Contact: MattN+bmo
Depends on: 1496706

Comment 70

6 months ago
I haven't read every comment so sorry if this is known...

I use Wire chat in a pinned tab. Whenever a message comes a very small, subtle sound will play (almost a click) and if I'm in another window or tab a notification will show. I can click the notification to take me to the Wire tab.

After updating to the build with this patch in it a new message arrived and it played the normal sound plus a loud and unwanted system sound. Some sort of chimes. I clicked on the notification and it took me to the tab as desired. Later I moved to another window and when a second message came in there was no notification. Only the small click sound from the tab. All subsequent messages were the same (no notifications).

Comment 71

5 months ago
(In reply to Caspy7 from comment #70)
> plus a loud and unwanted system sound.

Seems whether the system sound is appropriate will vary by user and usecase/site, and it's something that the user can hopefully/presumably control via the OS (and probably isn't controllable from the app requesting the notification). If I'm missing something, file a separate bug for this. We could potentially use a custom Firefox-y sound ( https://docs.microsoft.com/en-us/windows/uwp/design/shell/tiles-and-notifications/custom-audio-on-toasts ), though I dunno if that's supported in the API we use given we're not a UWP app. Anyway, material for a separate bug.

> Later I moved to another window and when a second message came in
> there was no notification. Only the small click sound from the tab. All
> subsequent messages were the same (no notifications).

Please file a follow-up bug for this, ideally with more detailed steps to reproduce (What is "Wire chat", can you reproduce with other sites that use these notifications, on a clean profile, etc.) and some details about your environment (what nightly version, 64-bit or 32-bit, what Win8/Win10 version, about:support details for any add-ons / config options that might be related).
Flags: needinfo?(caspy77)
(Assignee)

Comment 72

5 months ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #69)
> Makoto, Do you think it is worth adding to the release notes?
> Thanks

Yes, but this isn't turned on Firefox 64.  After some bugs are fixed, I would like to turn on this (for Firefox 65?).
Flags: needinfo?(m_kato)
(Assignee)

Updated

5 months ago
Blocks: 1497425
Release Note Request (optional, but appreciated)
[Why is this notable]: User facing change
[Affects Firefox for Android]: No
[Suggested wording]: On Windows, notifications will be sent using the native system (Toast).
(better wording is welcome)
relnote-firefox: --- → ?
Added to Nightly 64 release notes with this wording:
Websites notifications you subscribed to are now displayed and managed in the Windows Action Center
It seems that these changes have introduced an issue on the Firefox Screenshots web notification. The icon displayed on the web notification has a black background and it so no longer visible. Also, a "A3710B8EBB50CD3" string is displayed under the text from the web notification.

I have logged this issue on bug 1500054.
I'm going to move the relnote-firefox request over to bug 1497425 so it's easier to keep track of when this feature ships to release.
relnote-firefox: ? → -
You need to log in before you can comment on or make changes to this bug.