Minimize the delegate dependencies for native extension notifications.

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

 // Copyright (c) 2012 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/extensions/api/notifications/notifications_api.h"
 
 #include <stddef.h>
 
 #include <utility>
 
 #include "base/callback.h"
 #include "base/feature_list.h"
 #include "base/guid.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/api/notifications/extension_notification_display_helper.h"
 #include "chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h"
 #include "chrome/browser/notifications/notification.h"
 #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"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_system_provider.h"
@@ skipping 158 matching lines @@
   ShutdownNotifierFactory()
       : BrowserContextKeyedServiceShutdownNotifierFactory(
             "NotificationsApiDelegate") {
     DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
   }
   ~ShutdownNotifierFactory() override {}
 
   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_(
             ExtensionNotificationDisplayHelperFactory::GetForProfile(profile)),
         extension_id_(extension_id),
         id_(id),
         scoped_id_(CreateScopedIdentifier(extension_id, id)) {
     DCHECK(api_function_);
     DCHECK(display_helper_);
 
     shutdown_notifier_subscription_ =
         ShutdownNotifierFactory::GetInstance()->Get(profile)->Subscribe(
-            base::Bind(&NotificationsApiDelegate::Shutdown,
+            base::Bind(&NotificationApiDelegate::Shutdown,
                        base::Unretained(this)));
   }
 
   void Close(bool by_user) override {
     EventRouter::UserGestureState gesture =
         by_user ? EventRouter::USER_GESTURE_ENABLED
                 : EventRouter::USER_GESTURE_NOT_ENABLED;
     std::unique_ptr<base::ListValue> args(CreateBaseEventArgs());
     args->AppendBoolean(by_user);
     SendEvent(events::NOTIFICATIONS_ON_CLOSED,
@@ skipping 50 matching lines @@
 
         }
         return enabled;
       }
     }
 
     return false;
   }
 
  private:
-  ~NotificationsApiDelegate() override {}
+  ~NotificationApiDelegate() override {}
 
   void SendEvent(events::HistogramValue histogram_value,
                  const std::string& name,
                  EventRouter::UserGestureState user_gesture,
                  std::unique_ptr<base::ListValue> args) {
     if (!event_router_)
       return;
 
     std::unique_ptr<Event> event(
         new Event(histogram_value, name, std::move(args)));
@@ skipping 21 matching lines @@
   EventRouter* event_router_;
   ExtensionNotificationDisplayHelper* display_helper_;
 
   const std::string extension_id_;
   const std::string id_;
   const std::string scoped_id_;
 
   std::unique_ptr<KeyedServiceShutdownNotifier::Subscription>
       shutdown_notifier_subscription_;
 
-  DISALLOW_COPY_AND_ASSIGN(NotificationsApiDelegate);
+  DISALLOW_COPY_AND_ASSIGN(NotificationApiDelegate);
 };
 
 }  // namespace
 
 bool NotificationsApiFunction::IsNotificationsApiAvailable() {
   // We need to check this explicitly rather than letting
   // _permission_features.json enforce it, because we're sharing the
   // chrome.notifications permissions namespace with WebKit notifications.
   return extension()->is_platform_app() || extension()->is_extension();
 }
@@ skipping 135 matching lines @@
     for (const NotificationItem& api_item : *options->items) {
       optional_fields.items.push_back(message_center::NotificationItem(
           base::UTF8ToUTF16(api_item.title),
           base::UTF8ToUTF16(api_item.message)));
     }
   }
 
   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,
                                  extension_->id()),
       base::UTF8ToUTF16(extension_->name()), extension_->url(),
       api_delegate->id(), optional_fields, api_delegate);
 
   // Apply the "requireInteraction" flag. The value defaults to false.
   notification.set_never_timeout(options->require_interaction &&
                                  *options->require_interaction);
@@ skipping 323 matching lines @@
           : api::notifications::PERMISSION_LEVEL_DENIED;
 
   SetResult(
       base::MakeUnique<base::Value>(api::notifications::ToString(result)));
   SendResponse(true);
 
   return true;
 }
 
 }  // namespace extensions