Closed Bug 1155505 Opened 9 years ago Closed 6 years ago

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

Categories

(Toolkit Graveyard :: Notifications and Alerts, defect, P2)

All
Windows 10
defect

Tracking

(relnote-firefox -, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
relnote-firefox --- -
firefox64 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: feature, parity-chrome)

Attachments

(3 files, 6 obsolete files)

We had this on MetroFox, but no Desktop version.

Also, Toast Notification supports from Windows 8+.
OS: Windows 8.1 → Windows 10
Priority: -- → P4
[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
Attached patch WIP (obsolete) — Splinter Review
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.
There's also bug 852917 for Windows 8
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: nobody → m_kato
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" :)
Attached 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: → 488566
(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
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?
(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.
Attached 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!
Attached 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)
Attached 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)
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?
I will look this review this week.  But this will be also required by UX team 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-
: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 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 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/
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.
Oof, well that's been at least two months. 
Wonder how many months is in the not-badgering category for a re-ping.
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)
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)
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
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 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)
Blocks: 1450036
WriteBitmap will be used for Toast implementation, so it should be moved to WinUtils.
Implemnt notification backend by Windows Toast API that is from Windows 8+.

Original patch is me and add some features by eoger.
For feedback, I whould like to turn on native Windows Toast backend on
Nightly only.
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+
Is this ready to land?
Flags: needinfo?(m_kato)
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)
See Also: → 1473763
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)
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.
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
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
Backout by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10b62fac5269
Backed out 4 changesets for win2012 build bustages CLOSED TREE
Blocks: 1495998
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...
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
Depends on: 1496650
Makoto, Do you think it is worth adding to the release notes?
Thanks
Flags: needinfo?(m_kato)
Keywords: feature
QA Contact: MattN+bmo
Depends on: 1496706
QA Contact: MattN+bmo
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).
(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)
(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)
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
Depends on: 1498623
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.
Depends on: 1502044
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.
Flags: needinfo?(caspy77)
Depends on: 1766095
No longer depends on: 1766095
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.