Implement native web notifications for mac

chrome/browser/notifications/platform_notification_service_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #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"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/notifications/desktop_notification_profile_util.h"
 #include "chrome/browser/notifications/notification_object_proxy.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/notifications/persistent_notification_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
@@ skipping 24 matching lines @@
 #if defined(ENABLE_EXTENSIONS)
 #include "chrome/browser/notifications/notifier_state_tracker.h"
 #include "chrome/browser/notifications/notifier_state_tracker_factory.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/permissions/api_permission.h"
 #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;
 using message_center::NotifierId;
 
 namespace {
 
 // Invalid id for a renderer process. Used in cases where we need to check for
 // permission without having an associated renderer process yet.
 const int kInvalidRenderProcessId = -1;
@@ skipping 21 matching lines @@
 
 }  // namespace
 
 // static
 PlatformNotificationServiceImpl*
 PlatformNotificationServiceImpl::GetInstance() {
   return base::Singleton<PlatformNotificationServiceImpl>::get();
 }
 
 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() {}
 
 void PlatformNotificationServiceImpl::OnPersistentNotificationClick(
     BrowserContext* browser_context,
     int64_t persistent_notification_id,
     const GURL& origin,
     int action_index) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   blink::WebNotificationPermission permission =
@@ skipping 216 matching lines @@
     BrowserContext* browser_context,
     int64_t persistent_notification_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   Profile* profile = Profile::FromBrowserContext(browser_context);
   DCHECK(profile);
 
 #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;
 
   GetNotificationUIManager()->CancelById(
       iter->second, NotificationUIManager::GetProfileID(profile));
 
   persistent_notifications_.erase(iter);
 #endif
 }
@@ skipping 64 matching lines @@
     notification.set_never_timeout(true);
 
   return notification;
 }
 
 NotificationUIManager*
 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();
 }
 
 void PlatformNotificationServiceImpl::OpenNotificationSettings(
     BrowserContext* browser_context) {
 #if defined(OS_ANDROID)
   NOTIMPLEMENTED();
 #else
 
   Profile* profile = Profile::FromBrowserContext(browser_context);
@@ skipping 27 matching lines @@
         extensions::ExtensionRegistry::Get(profile)->GetExtensionById(
             origin.host(), extensions::ExtensionRegistry::EVERYTHING);
     DCHECK(extension);
 
     return base::UTF8ToUTF16(extension->name());
   }
 #endif
 
   return base::string16();
 }