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)
Tracking
(relnote-firefox -, firefox64 fixed)
RESOLVED
FIXED
mozilla64
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+.
Assignee | ||
Updated•9 years ago
|
OS: Windows 8.1 → Windows 10
Updated•9 years ago
|
Priority: -- → P4
Comment 1•9 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•9 years ago
|
||
Assignee | ||
Comment 3•9 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•9 years ago
|
||
There's also bug 852917 for Windows 8
Assignee | ||
Comment 6•9 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•9 years ago
|
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" :)
Comment 8•7 years ago
|
||
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?
Comment 9•7 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)
Comment 10•7 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? 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)
Comment 11•7 years ago
|
||
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•7 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.
Comment 13•7 years ago
|
||
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•7 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.
Comment 15•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
Feel free to change the f? flag!
Comment 17•7 years ago
|
||
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)
Comment 20•7 years ago
|
||
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) |
Updated•7 years ago
|
Attachment #8911427 -
Attachment is obsolete: true
Comment 22•7 years ago
|
||
+ 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•7 years ago
|
||
I will look this review this week. But this will be also required by UX team review.
Assignee | ||
Comment 24•7 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•7 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)
Comment 26•7 years ago
|
||
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.
Comment 27•7 years ago
|
||
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)
Comment 28•7 years ago
|
||
> 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
Comment 29•7 years ago
|
||
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•7 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•7 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
Comment 32•7 years ago
|
||
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•6 years 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).
Comment 34•6 years ago
|
||
We have reached out to Microsoft last December regarding the issue I described in comment 32 and are awaiting an answer.
Comment 35•6 years 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 36•6 years ago
|
||
Chrome added this lately https://bugs.chromium.org/p/chromium/issues/detail?id=516147#c91
Comment hidden (mozreview-request) |
Comment 38•6 years ago
|
||
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•6 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/#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•6 years 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•6 years ago
|
Component: XUL Widgets → Notifications and Alerts
Flags: needinfo?(enndeakin)
Comment 41•6 years ago
|
||
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) |
Comment 44•6 years ago
|
||
> 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)
Done
Assignee | ||
Comment 45•6 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/#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)
Updated•6 years ago
|
Keywords: parity-chrome
Assignee | ||
Comment 46•6 years ago
|
||
WriteBitmap will be used for Toast implementation, so it should be moved to WinUtils.
Assignee | ||
Comment 47•6 years 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•6 years ago
|
||
For feedback, I whould like to turn on native Windows Toast backend on Nightly only.
Assignee | ||
Comment 49•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42cd1aa89fc498237c7177adb4803f81320afd26
Assignee: nobody → m_kato
Assignee | ||
Updated•6 years ago
|
Attachment #8911505 -
Attachment is obsolete: true
Comment 50•6 years ago
|
||
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+
Comment 52•6 years ago
|
||
Oh, it looks like it's waiting on review from Jim still. Review ping?
Flags: needinfo?(m_kato) → needinfo?(jmathies)
Comment 53•6 years ago
|
||
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)
Comment 54•6 years ago
|
||
Gentle review ping, jimm, if you have cycles. :-)
Flags: needinfo?(jmathies)
Comment 55•6 years ago
|
||
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 56•6 years ago
|
||
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+
Comment 58•6 years 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)
Comment 59•6 years ago
|
||
Seen here: https://pasteboard.co/HGpiwA2.png
Comment 60•6 years ago
|
||
This bug isn't marked as FIXED yet so it's expected that it doesn't work yet.
Comment 61•6 years 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 years 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 years 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
Comment 64•6 years ago
|
||
Backed out 4 changesets (bug 1155505) for win2012 build bustages push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=203024615&revision=f5e706d5a1433a95a8d3015c684cfef10c13e850 failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=203026624&revision=f5e706d5a1433a95a8d3015c684cfef10c13e850&failure_classification_id=2&searchStr=windows%2C2012%2Copt%2Cstatic-analysis-win32-st-an%2Fopt%2C%28s%29 backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/10b62fac52698573bba71b4622e3a35ba2edb181
Assignee | ||
Comment 65•6 years 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...
Assignee | ||
Comment 66•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d36d89bf2dc567544b312c0dd6efa6e2742f4a
Flags: needinfo?(m_kato)
Comment 67•6 years 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 years 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
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 69•6 years ago
|
||
Makoto, Do you think it is worth adding to the release notes? Thanks
Updated•6 years ago
|
QA Contact: MattN+bmo
Comment 70•6 years 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•6 years 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•6 years 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)
Comment 73•6 years ago
|
||
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:
--- → ?
Comment 74•6 years ago
|
||
Added to Nightly 64 release notes with this wording: Websites notifications you subscribed to are now displayed and managed in the Windows Action Center
Comment 75•6 years ago
|
||
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.
Comment 76•5 years ago
|
||
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.
Updated•9 months ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•