Minimize the delegate dependencies for native extension notifications.

chrome/browser/extensions/api/notifications/notifications_api.cc

Index: chrome/browser/extensions/api/notifications/notifications_api.cc
diff --git a/chrome/browser/extensions/api/notifications/notifications_api.cc b/chrome/browser/extensions/api/notifications/notifications_api.cc
index 577aaf7c764e93da31592fb64bae04dce3274880..9aa3c2e7942117b99af2dd31aac88fa90724d937 100644
--- a/chrome/browser/extensions/api/notifications/notifications_api.cc
+++ b/chrome/browser/extensions/api/notifications/notifications_api.cc
@@ -25,7 +25,9 @@
 #include "chrome/browser/notifications/notifier_state_tracker.h"
 #include "chrome/browser/notifications/notifier_state_tracker_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/extensions/api/notifications/notification_style.h"
+#include "chrome/common/features.h"
 #include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
 #include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
 #include "content/public/browser/render_process_host.h"
@@ -204,12 +206,33 @@ class ShutdownNotifierFactory
   DISALLOW_COPY_AND_ASSIGN(ShutdownNotifierFactory);
 };
 
-class NotificationsApiDelegate : public NotificationDelegate {
+// Temporary native notification api delagate, it is only used
+// to extract the delegate id.
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+class NativeNotificationApiDelegate : public NotificationDelegate {
  public:
-  NotificationsApiDelegate(ChromeAsyncExtensionFunction* api_function,
-                           Profile* profile,
-                           const std::string& extension_id,
-                           const std::string& id)
+  NativeNotificationApiDelegate(const std::string& extension_id,
+                                const std::string& notification_id)
+      : scoped_notification_id_(
+            CreateScopedIdentifier(extension_id, notification_id)) {}
+
+  std::string id() const override { return scoped_notification_id_; }
+
+ private:
+  ~NativeNotificationApiDelegate() override = default;
+  const std::string scoped_notification_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeNotificationApiDelegate);
+};
+#endif  // BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+
+// Message center based notification delegate with all the functionality.
+class NotificationApiDelegate : public NotificationDelegate {
+ public:
+  NotificationApiDelegate(ChromeAsyncExtensionFunction* api_function,
+                          Profile* profile,
+                          const std::string& extension_id,
+                          const std::string& id)
       : api_function_(api_function),
         event_router_(EventRouter::Get(profile)),
         display_helper_(
@@ -222,7 +245,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
 
     shutdown_notifier_subscription_ =
         ShutdownNotifierFactory::GetInstance()->Get(profile)->Subscribe(
-            base::Bind(&NotificationsApiDelegate::Shutdown,
+            base::Bind(&NotificationApiDelegate::Shutdown,
                        base::Unretained(this)));
   }
 
@@ -293,7 +316,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
   }
 
  private:
-  ~NotificationsApiDelegate() override {}
+  ~NotificationApiDelegate() override {}
 
   void SendEvent(events::HistogramValue histogram_value,
                  const std::string& name,
@@ -335,7 +358,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
   std::unique_ptr<KeyedServiceShutdownNotifier::Subscription>
       shutdown_notifier_subscription_;
 
-  DISALLOW_COPY_AND_ASSIGN(NotificationsApiDelegate);
+  DISALLOW_COPY_AND_ASSIGN(NotificationApiDelegate);
 };
 
 }  // namespace
@@ -491,9 +514,19 @@ bool NotificationsApiFunction::CreateNotification(
   if (options->is_clickable.get())
     optional_fields.clickable = *options->is_clickable;
 
-  NotificationsApiDelegate* api_delegate(new NotificationsApiDelegate(
-      this, GetProfile(), extension_->id(), id));  // ownership is passed to
-                                                   // Notification
+  // Create the notification api delegate. Ownership passed to the notification.
+  NotificationDelegate* api_delegate;
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)

[Peter Beverloo, 2017/05/15 15:56:52]

Here and above: These #if-defs are ugly. Please be more complete about why they
are necessary, for how long (you estimate that) they will be necessary and link
to a bug about getting rid of them.

[Miguel Garcia, 2017/05/16 11:01:34]

Good point I provided a high level explanation in
https://bugs.chromium.org/p/chromium/issues/detail?id=720345 and linked to it
from the code (not here but in #210 where the delegates are defined)

+  if (base::FeatureList::IsEnabled(features::kNativeNotifications))
+    api_delegate = new NativeNotificationApiDelegate(extension_->id(), id);
+  else
+    api_delegate =
+        new NotificationApiDelegate(this, GetProfile(), extension_->id(), id);

[Peter Beverloo, 2017/05/15 15:56:52]

if () {
} else {
}

[Miguel Garcia, 2017/05/16 11:01:34]

Done.

+#else
+  api_delegate =
+      new NotificationApiDelegate(this, GetProfile(), extension_->id(), id);
+#endif  // BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+
   Notification notification(
       type, title, message, icon,
       message_center::NotifierId(message_center::NotifierId::APPLICATION,