From ddee77fae4ed1bce6e35dd2889bcdbd945015ef6 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 15 Apr 2016 16:01:21 +0900 Subject: [PATCH 1/3] Do not use get_Setting to determine whether notification is enabled On Windows 10 get_Setting always returns DISABLED when the program has a AppUserModelID. --- brightray/browser/win/windows_toast_notification.cc | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/brightray/browser/win/windows_toast_notification.cc b/brightray/browser/win/windows_toast_notification.cc index 401473b9c3e0..24e131971561 100644 --- a/brightray/browser/win/windows_toast_notification.cc +++ b/brightray/browser/win/windows_toast_notification.cc @@ -90,16 +90,7 @@ void WindowsToastNotification::Show( const bool silent) { auto presenter_win = static_cast(presenter()); std::wstring icon_path = presenter_win->SaveIconToFilesystem(icon, icon_url); - - // Ask Windows for the current notification settings - // If not allowed, we return here (0 = enabled) - ABI::Windows::UI::Notifications::NotificationSetting setting; - toast_notifier_->get_Setting(&setting); - if (setting != 0) { - NotificationFailed(); - return; - } - + ComPtr toast_xml; if(FAILED(GetToastXml(toast_manager_.Get(), title, msg, icon_path, silent, &toast_xml))) { NotificationFailed(); From f4c27c6d29b43784dd6579f815a14c7bd036972a Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 15 Apr 2016 16:14:13 +0900 Subject: [PATCH 2/3] Store weak ref to Notification in ToastEventHandler --- .../browser/linux/libnotify_notification.cc | 11 ++-------- .../browser/linux/libnotify_notification.h | 2 -- brightray/browser/mac/cocoa_notification.h | 3 +-- brightray/browser/mac/cocoa_notification.mm | 10 ++------- .../mac/notification_center_delegate.mm | 4 ++-- brightray/browser/notification.cc | 15 +++++++++++++ brightray/browser/notification.h | 5 +++++ .../browser/win/windows_toast_notification.cc | 21 +++---------------- .../browser/win/windows_toast_notification.h | 8 ++----- 9 files changed, 32 insertions(+), 47 deletions(-) diff --git a/brightray/browser/linux/libnotify_notification.cc b/brightray/browser/linux/libnotify_notification.cc index 399416a5721f..e94c81619fe7 100644 --- a/brightray/browser/linux/libnotify_notification.cc +++ b/brightray/browser/linux/libnotify_notification.cc @@ -136,19 +136,12 @@ void LibnotifyNotification::Dismiss() { void LibnotifyNotification::OnNotificationClosed( NotifyNotification* notification) { - delegate()->NotificationClosed(); - Destroy(); + NotificationDismissed(); } void LibnotifyNotification::OnNotificationView( NotifyNotification* notification, char* action) { - delegate()->NotificationClick(); - Destroy(); -} - -void LibnotifyNotification::NotificationFailed() { - delegate()->NotificationFailed(); - Destroy(); + NotificationClicked(); } } // namespace brightray diff --git a/brightray/browser/linux/libnotify_notification.h b/brightray/browser/linux/libnotify_notification.h index 40d52cac419b..cb9384cb0e0c 100644 --- a/brightray/browser/linux/libnotify_notification.h +++ b/brightray/browser/linux/libnotify_notification.h @@ -34,8 +34,6 @@ class LibnotifyNotification : public Notification { CHROMEG_CALLBACK_1(LibnotifyNotification, void, OnNotificationView, NotifyNotification*, char*); - void NotificationFailed(); - NotifyNotification* notification_; DISALLOW_COPY_AND_ASSIGN(LibnotifyNotification); diff --git a/brightray/browser/mac/cocoa_notification.h b/brightray/browser/mac/cocoa_notification.h index dd7d3cae4b05..7287f00f2456 100644 --- a/brightray/browser/mac/cocoa_notification.h +++ b/brightray/browser/mac/cocoa_notification.h @@ -28,8 +28,7 @@ class CocoaNotification : public Notification { const bool silent) override; void Dismiss() override; - void NotifyDisplayed(); - void NotifyClick(); + void NotificationDisplayed(); NSUserNotification* notification() const { return notification_; } diff --git a/brightray/browser/mac/cocoa_notification.mm b/brightray/browser/mac/cocoa_notification.mm index febd2af4c94c..e7ed61dbf369 100644 --- a/brightray/browser/mac/cocoa_notification.mm +++ b/brightray/browser/mac/cocoa_notification.mm @@ -58,17 +58,11 @@ void CocoaNotification::Show(const base::string16& title, void CocoaNotification::Dismiss() { [NSUserNotificationCenter.defaultUserNotificationCenter removeDeliveredNotification:notification_]; - delegate()->NotificationClosed(); - Destroy(); + NotificationDismissed(); } -void CocoaNotification::NotifyDisplayed() { +void CocoaNotification::NotificationDisplayed() { delegate()->NotificationDisplayed(); } -void CocoaNotification::NotifyClick() { - delegate()->NotificationClick(); - Destroy(); -} - } // namespace brightray diff --git a/brightray/browser/mac/notification_center_delegate.mm b/brightray/browser/mac/notification_center_delegate.mm index 53537bdc311a..fd77e0685e4b 100644 --- a/brightray/browser/mac/notification_center_delegate.mm +++ b/brightray/browser/mac/notification_center_delegate.mm @@ -22,14 +22,14 @@ didDeliverNotification:(NSUserNotification*)notif { auto notification = presenter_->GetNotification(notif); if (notification) - notification->NotifyDisplayed(); + notification->NotificationDisplayed(); } - (void)userNotificationCenter:(NSUserNotificationCenter*)center didActivateNotification:(NSUserNotification *)notif { auto notification = presenter_->GetNotification(notif); if (notification) - notification->NotifyClick(); + notification->NotificationClicked(); } - (BOOL)userNotificationCenter:(NSUserNotificationCenter*)center diff --git a/brightray/browser/notification.cc b/brightray/browser/notification.cc index 6384737c21bf..ba9df5446f30 100644 --- a/brightray/browser/notification.cc +++ b/brightray/browser/notification.cc @@ -20,6 +20,21 @@ Notification::~Notification() { delegate()->NotificationDestroyed(); } +void Notification::NotificationClicked() { + delegate()->NotificationClick(); + Destroy(); +} + +void Notification::NotificationDismissed() { + delegate()->NotificationClosed(); + Destroy(); +} + +void Notification::NotificationFailed() { + delegate()->NotificationFailed(); + Destroy(); +} + void Notification::Destroy() { presenter()->RemoveNotification(this); } diff --git a/brightray/browser/notification.h b/brightray/browser/notification.h index 30ac680d25f8..afacd50c1931 100644 --- a/brightray/browser/notification.h +++ b/brightray/browser/notification.h @@ -29,6 +29,11 @@ class Notification { // notification gets closed. virtual void Dismiss() = 0; + // Should be called by derived classes. + void NotificationClicked(); + void NotificationDismissed(); + void NotificationFailed(); + base::WeakPtr GetWeakPtr() { return weak_factory_.GetWeakPtr(); } diff --git a/brightray/browser/win/windows_toast_notification.cc b/brightray/browser/win/windows_toast_notification.cc index 24e131971561..efa30f2fe11d 100644 --- a/brightray/browser/win/windows_toast_notification.cc +++ b/brightray/browser/win/windows_toast_notification.cc @@ -117,7 +117,7 @@ void WindowsToastNotification::Show( return; } - if (FAILED(SetupCallbacks(toast_notification_.Get()))) { + if (!SetupCallbacks(toast_notification_.Get())) { NotificationFailed(); return; } @@ -134,21 +134,6 @@ void WindowsToastNotification::Dismiss() { toast_notifier_->Hide(toast_notification_.Get()); } -void WindowsToastNotification::NotificationClicked() { - delegate()->NotificationClick(); - Destroy(); -} - -void WindowsToastNotification::NotificationDismissed() { - delegate()->NotificationClosed(); - Destroy(); -} - -void WindowsToastNotification::NotificationFailed() { - delegate()->NotificationFailed(); - Destroy(); -} - bool WindowsToastNotification::GetToastXml( ABI::Windows::UI::Notifications::IToastNotificationManagerStatics* toastManager, const std::wstring& title, @@ -390,8 +375,8 @@ bool WindowsToastNotification::RemoveCallbacks( /* / Toast Event Handler */ -ToastEventHandler::ToastEventHandler(WindowsToastNotification* notification) - : notification_(notification) { +ToastEventHandler::ToastEventHandler(Notification* notification) + : notification_(notification->GetWeakPtr()) { } ToastEventHandler::~ToastEventHandler() { diff --git a/brightray/browser/win/windows_toast_notification.h b/brightray/browser/win/windows_toast_notification.h index c603ce3d7960..fcbe78faa4d0 100644 --- a/brightray/browser/win/windows_toast_notification.h +++ b/brightray/browser/win/windows_toast_notification.h @@ -50,10 +50,6 @@ class WindowsToastNotification : public Notification { private: friend class ToastEventHandler; - void NotificationClicked(); - void NotificationDismissed(); - void NotificationFailed(); - bool GetToastXml(ABI::Windows::UI::Notifications::IToastNotificationManagerStatics* toastManager, const std::wstring& title, const std::wstring& msg, @@ -97,7 +93,7 @@ class ToastEventHandler : public RuntimeClass, DesktopToastDismissedEventHandler, DesktopToastFailedEventHandler> { public: - ToastEventHandler(WindowsToastNotification* notification); + ToastEventHandler(Notification* notification); ~ToastEventHandler(); IFACEMETHODIMP Invoke(ABI::Windows::UI::Notifications::IToastNotification* sender, IInspectable* args); @@ -105,7 +101,7 @@ class ToastEventHandler : public RuntimeClass, IFACEMETHODIMP Invoke(ABI::Windows::UI::Notifications::IToastNotification* sender, ABI::Windows::UI::Notifications::IToastFailedEventArgs* e); private: - WindowsToastNotification* notification_; // weak ref. + base::WeakPtr notification_; // weak ref. DISALLOW_COPY_AND_ASSIGN(ToastEventHandler); }; From 593fb8cdf06e1bb45a857a3fbf3d827d943f0d36 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 15 Apr 2016 16:20:36 +0900 Subject: [PATCH 3/3] Delay notification events to next tick It is possible that the events get emitted when calling Show(), which would then delete the class before Show() ends, results in using members of a deleted class. By delaying the events to next tick we can effectively avoid this. --- brightray/browser/win/windows_toast_notification.cc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/brightray/browser/win/windows_toast_notification.cc b/brightray/browser/win/windows_toast_notification.cc index efa30f2fe11d..4d4b5bfc3412 100644 --- a/brightray/browser/win/windows_toast_notification.cc +++ b/brightray/browser/win/windows_toast_notification.cc @@ -12,6 +12,7 @@ #include "browser/win/scoped_hstring.h" #include "browser/win/notification_presenter_win.h" #include "common/application_info.h" +#include "content/public/browser/browser_thread.h" using namespace ABI::Windows::Data::Xml::Dom; @@ -384,21 +385,27 @@ ToastEventHandler::~ToastEventHandler() { IFACEMETHODIMP ToastEventHandler::Invoke( ABI::Windows::UI::Notifications::IToastNotification* sender, IInspectable* args) { - notification_->NotificationClicked(); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::Bind(&Notification::NotificationClicked, notification_)); return S_OK; } IFACEMETHODIMP ToastEventHandler::Invoke( ABI::Windows::UI::Notifications::IToastNotification* sender, ABI::Windows::UI::Notifications::IToastDismissedEventArgs* e) { - notification_->NotificationDismissed(); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::Bind(&Notification::NotificationDismissed, notification_)); return S_OK; } IFACEMETHODIMP ToastEventHandler::Invoke( ABI::Windows::UI::Notifications::IToastNotification* sender, ABI::Windows::UI::Notifications::IToastFailedEventArgs* e) { - notification_->NotificationFailed(); + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::Bind(&Notification::NotificationFailed, notification_)); return S_OK; }