Minimize the delegate dependencies for non persistent notifications.

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 4c60c08daae8000ad58f8de373444e782274bc8e..91cf91dc39d4f1a617913699e6a3befca3ce6a2c 100644
--- a/chrome/browser/notifications/platform_notification_service_impl.cc
+++ b/chrome/browser/notifications/platform_notification_service_impl.cc
@@ -15,6 +15,8 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
+#include "chrome/browser/notifications/native_notification_delegate.h"
+#include "chrome/browser/notifications/notification_common.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
 #include "chrome/browser/notifications/notification_object_proxy.h"
 #include "chrome/browser/notifications/persistent_notification_delegate.h"
@@ -27,6 +29,8 @@
 #include "chrome/browser/safe_browsing/ping_manager.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/features.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
@@ -120,6 +124,7 @@ PlatformNotificationServiceImpl::PlatformNotificationServiceImpl()
 
 PlatformNotificationServiceImpl::~PlatformNotificationServiceImpl() {}
 
+// TODO(miguelg): Move this to PersistentNotificationHandler
 void PlatformNotificationServiceImpl::OnPersistentNotificationClick(
     BrowserContext* browser_context,
     const std::string& notification_id,
@@ -165,6 +170,7 @@ void PlatformNotificationServiceImpl::OnPersistentNotificationClick(
               base::Unretained(this)));
 }
 
+// TODO(miguelg): Move this to PersistentNotificationHandler
 void PlatformNotificationServiceImpl::OnPersistentNotificationClose(
     BrowserContext* browser_context,
     const std::string& notification_id,
@@ -311,15 +317,26 @@ void PlatformNotificationServiceImpl::DisplayNotification(
   DCHECK_EQ(0u, notification_data.actions.size());
   DCHECK_EQ(0u, notification_resources.action_icons.size());
 
-  NotificationObjectProxy* proxy = new NotificationObjectProxy(
+  NotificationDelegate* notification_delegate;
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)

[Peter Beverloo, 2017/05/31 17:52:42]

Please add some justification about why we need this here. This will go away
once the MCDS becomes a platform bridge right?

[Miguel Garcia, 2017/06/01 17:00:53]

Even sooner, see https://codereview.chromium.org/2906883003/ two CLs ahead in
the chain :)
I will add the follow up patches to the CL description

+  if (base::FeatureList::IsEnabled(features::kNativeNotifications) &&
+      g_browser_process->notification_platform_bridge()) {
+    notification_delegate = new NativeNotificationDelegate(notification_id);
+  } else {
+    notification_delegate = new NotificationObjectProxy(
+        browser_context, notification_id, origin, std::move(delegate));
+  }
+#else
+  notification_delegate = new NotificationObjectProxy(
       browser_context, notification_id, origin, std::move(delegate));
+#endif  // BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+
   Notification notification = CreateNotificationFromData(
       profile, GURL() /* service_worker_scope */, origin, notification_data,
-      notification_resources, proxy);
+      notification_resources, notification_delegate);
 
   GetNotificationDisplayService(profile)->Display(
-      NotificationCommon::NON_PERSISTENT, notification.delegate_id(),
-      notification);
+      NotificationCommon::NON_PERSISTENT, notification_id, notification);
   if (cancel_callback) {
 #if defined(OS_WIN)
     std::string profile_id =
@@ -327,9 +344,8 @@ void PlatformNotificationServiceImpl::DisplayNotification(
 #elif defined(OS_POSIX)
     std::string profile_id = profile->GetPath().BaseName().value();
 #endif
-    *cancel_callback =
-        base::Bind(&CancelNotification, notification.delegate_id(), profile_id,
-                   profile->IsOffTheRecord());
+    *cancel_callback = base::Bind(&CancelNotification, notification_id,
+                                  profile_id, profile->IsOffTheRecord());
   }
 }
 
@@ -354,6 +370,9 @@ void PlatformNotificationServiceImpl::DisplayPersistentNotification(
   // The notification settings button will be appended after the developer-
   // supplied buttons, available in |notification_data.actions|.
   int settings_button_index = notification_data.actions.size();
+
+  // TODO(miguelg) the persistent delegate should not be needed for
+  // native notifications, use the NativeNotificationDelegate instead.

[Peter Beverloo, 2017/05/31 17:52:42]

"... use the NND instead."

That should be done implicitly within the NPB for the MC right?

[Miguel Garcia, 2017/06/01 17:00:53]

Now that I know for a fact this is the case I think I am just going to remove
the TODO.

On 2017/05/31 17:52:42, Peter Beverloo wrote:

   PersistentNotificationDelegate* delegate = new PersistentNotificationDelegate(
       browser_context, notification_id, origin, settings_button_index);