Implement native web notifications for mac

chrome/browser/notifications/platform_notification_service_impl.cc

Index: chrome/browser/notifications/platform_notification_service_impl.cc
diff --git a/chrome/browser/notifications/platform_notification_service_impl.cc b/chrome/browser/notifications/platform_notification_service_impl.cc
index 6a28a84f962924ea2b99752549571da507af4a8a..7202fad0a4ac6761bb9acece1c8b5a1a7790dd99 100644
--- a/chrome/browser/notifications/platform_notification_service_impl.cc
+++ b/chrome/browser/notifications/platform_notification_service_impl.cc
@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics_action.h"
 #include "base/prefs/pref_service.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
@@ -54,10 +55,6 @@
 #include "extensions/common/permissions/permissions_data.h"
 #endif
 
-#if defined(OS_ANDROID)
-#include "base/strings/string_number_conversions.h"
-#endif
-
 using content::BrowserContext;
 using content::BrowserThread;
 using content::PlatformNotificationContext;
@@ -99,7 +96,12 @@ PlatformNotificationServiceImpl::GetInstance() {
 }
 
 PlatformNotificationServiceImpl::PlatformNotificationServiceImpl()
-    : notification_ui_manager_for_tests_(nullptr) {}
+    : notification_ui_manager_for_tests_(nullptr) {
+#if defined(OS_MACOSX)
+  native_notification_ui_manager_.reset(
+      NotificationUIManager::CreateNativeNotificationManager());

[Peter Beverloo, 2016/01/08 05:14:26]

Could you revert the changes to notification_ui_manager.h, and instead create a
helper function (in the anonymous namespace above) that does the following?

scoped_ptr<NotificationUIManager> GetNativeNotificationUIManager() {
#if MAC
  if (... AcceptNativeNotifications() conditions ...)
    return new NotificationUIManagerMac();
#else
  return nullptr;
#endif
}

You can then assign in the initialization list of this constructor. That
significantly reduces the impact of this patch on code it doesn't have to touch,
and gives us a much cleaner patch towards supporting a Windows prototype or
converting the Android manager.

[Miguel Garcia, 2016/01/08 11:46:55]

So the problem with doing it the way you suggest is that then this (pure C++)
file needs to include notification_ui_manager_mac.h which contains ObjC code.
This is sort of ugly but on top of that it does not compile since ObjC can only
be included as a seaparate compilation unit that is then merged at link time.

I am happy to discuss alternatives but let's do it offline then, not in the code
review. Perhaps also after this cl lands?

In any case I think that AcceptNativeNotifications() does belong to the
notification_ui_manager.h interface.

 

On 2016/01/08 05:14:26, Peter Beverloo wrote:

[Peter Beverloo, 2016/01/08 19:40:19]

This is a good point. Looking at various other files, we can include C++ files
from ObjC code, but not the other way around. Using an interface class for
NotificationCenterDelegate won't help either because scoped_nsobject.h uses ObjC
code as well.

Instead, let's remove the #ifdefs - define a CreateNativeNotificationManager()
for Android and non-Mac desktop (w/ an ifdef...) that always returns a nullptr,
and always initialise native_notification_ui_manager_ with its value. In the Mac
version (in the .mm file), only return a non-nullptr when the conditions you
currently list in AcceptNativeNotifications() pass.
We won't use it if AcceptNativeNotifications() returns false, so we've allocated
memory for no reason at that point. This is exactly the reason why factory
functions exists.
 

+#endif
+}
 
 PlatformNotificationServiceImpl::~PlatformNotificationServiceImpl() {}
 
@@ -336,12 +338,17 @@ void PlatformNotificationServiceImpl::ClosePersistentNotification(
 #if defined(OS_ANDROID)
   // TODO(peter): Remove this conversion when the notification ids are being
   // generated by the caller of this method.
-  std::string textual_persistent_notification_id =
-      base::Int64ToString(persistent_notification_id);
   GetNotificationUIManager()->CancelById(
-      textual_persistent_notification_id,
+      base::Int64ToString(persistent_notification_id),

[Peter Beverloo, 2016/01/08 05:14:26]

I very badly need to clean this up :-(.

Per the previous comment, you could make this slightly better by doing something
like:

#if ANDROID
  bool cancel_by_persistent_id = true;
#else
  bool cancel_by_persistent_id = !!native_notification_ui_manager_;
#endif

  if (cancel_by_persistent_id) {
    GetNoti..()->..
  }

[Miguel Garcia, 2016/01/08 11:46:55]

Done.

       NotificationUIManager::GetProfileID(profile));
 #else
+  if (native_notification_ui_manager_ &&
+      native_notification_ui_manager_->AcceptNativeNotifications()) {
+    GetNotificationUIManager()->CancelById(
+        base::Int64ToString(persistent_notification_id),
+        NotificationUIManager::GetProfileID(profile));
+  }
+
   auto iter = persistent_notifications_.find(persistent_notification_id);
   if (iter == persistent_notifications_.end())
     return;
@@ -426,6 +433,11 @@ PlatformNotificationServiceImpl::GetNotificationUIManager() const {
   if (notification_ui_manager_for_tests_)
     return notification_ui_manager_for_tests_;
 
+  if (native_notification_ui_manager_ &&
+      native_notification_ui_manager_->AcceptNativeNotifications()) {
+    return native_notification_ui_manager_.get();
+  }
+
   return g_browser_process->notification_ui_manager();
 }