Clean up the NotificationUIManagerAndroid.

chrome/browser/notifications/notification_ui_manager_android.cc

Index: chrome/browser/notifications/notification_ui_manager_android.cc
diff --git a/chrome/browser/notifications/notification_ui_manager_android.cc b/chrome/browser/notifications/notification_ui_manager_android.cc
index aa5984cf9515cb91bc8f980498f6da65da1cd80b..e15dfa894f1c9dc37d5a3c388caefd09854770a2 100644
--- a/chrome/browser/notifications/notification_ui_manager_android.cc
+++ b/chrome/browser/notifications/notification_ui_manager_android.cc
@@ -14,7 +14,6 @@
 #include "chrome/browser/notifications/notification.h"
 #include "chrome/browser/notifications/persistent_notification_delegate.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
-#include "chrome/browser/notifications/profile_notification.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "content/public/common/persistent_notification_status.h"
 #include "content/public/common/platform_notification_data.h"
@@ -27,19 +26,6 @@ using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertUTF16ToJavaString;
 using base::android::ConvertUTF8ToJavaString;
 
-namespace {
-
-// Called when the "notificationclick" event in the Service Worker has finished
-// executing for a notification that was created in a previous instance of the
-// browser.
-void OnEventDispatchComplete(content::PersistentNotificationStatus status) {
-  // TODO(peter): Add UMA statistics based on |status|.
-  // TODO(peter): Decide if we want to forcefully shut down the browser process

[johnme, 2015/04/09 17:00:01]

This TODO seems to have gotten lost?

[Peter Beverloo, 2015/04/09 17:34:42]

It's still captured in NotificationService.java:91. Since we don't have any
concrete plans, simplifying this code for now is the better option.

-  // if we're confident it was created for delivering this event.
-}
-
-}  // namespace
-
 // Called by the Java side when a notification event has been received, but the
 // NotificationUIManager has not been initialized yet. Enforce initialization of
 // the class.
@@ -83,8 +69,7 @@ bool NotificationUIManagerAndroid::OnNotificationClicked(
   PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick(
       ProfileManager::GetLastUsedProfile(),
       persistent_notification_id,
-      origin,
-      base::Bind(&OnEventDispatchComplete));
+      origin);
 
   return true;
 }
@@ -102,22 +87,15 @@ bool NotificationUIManagerAndroid::OnNotificationClosed(
 
 void NotificationUIManagerAndroid::Add(const Notification& notification,
                                        Profile* profile) {
-  // If the given notification is replacing an older one, drop its associated
-  // profile notification object without closing the platform notification.
-  // We'll use the native Android system to perform a smoother replacement.
-  ProfileNotification* notification_to_replace =
-      FindNotificationToReplace(notification, profile);
-  if (notification_to_replace)
-    RemoveProfileNotification(notification_to_replace, false /* close */);
-
-  ProfileNotification* profile_notification =
-      new ProfileNotification(profile, notification);
-
-  // Takes ownership of |profile_notification|.
-  AddProfileNotification(profile_notification);
-
   JNIEnv* env = AttachCurrentThread();
 
+  // The Android notification UI manager only supports Web Notifications, which
+  // have a PersistentNotificationDelegate. The persistent id of the
+  // notification is exposed through it's interface.
+  //
+  // TODO(peter): When content/ passes a message_center::Notification to the
+  // chrome/ layer, the persistent notification id should be captured as a
+  // property on that object instead, making this cast unnecessary.
   PersistentNotificationDelegate* delegate =
       static_cast<PersistentNotificationDelegate*>(notification.delegate());
   DCHECK(delegate);
@@ -159,8 +137,6 @@ void NotificationUIManagerAndroid::Add(const Notification& notification,
 
 bool NotificationUIManagerAndroid::Update(const Notification& notification,
                                           Profile* profile) {
-  // This method is currently only called from extensions and local discovery,
-  // both of which are not supported on Android.
   NOTIMPLEMENTED();
   return false;
 }
@@ -168,115 +144,23 @@ bool NotificationUIManagerAndroid::Update(const Notification& notification,
 const Notification* NotificationUIManagerAndroid::FindById(
     const std::string& delegate_id,
     ProfileID profile_id) const {
-  std::string profile_notification_id =
-      ProfileNotification::GetProfileNotificationId(delegate_id, profile_id);
-  ProfileNotification* profile_notification =
-      FindProfileNotification(profile_notification_id);
-  if (!profile_notification)
-    return 0;
-
-  return &profile_notification->notification();
+  NOTIMPLEMENTED();
+  return nullptr;
 }
 
 bool NotificationUIManagerAndroid::CancelById(const std::string& delegate_id,
                                               ProfileID profile_id) {
-  std::string profile_notification_id =
-      ProfileNotification::GetProfileNotificationId(delegate_id, profile_id);
-  ProfileNotification* profile_notification =
-      FindProfileNotification(profile_notification_id);
-  if (profile_notification) {
-    RemoveProfileNotification(profile_notification, true /* close */);
-    return true;
-  }
-
-  // On Android, it's still possible that the notification can be closed in case
-  // the platform Id is known, even if no delegate exists. This happens when the
-  // browser process is started because of interaction with a Notification.
-  PlatformCloseNotification(delegate_id);
-  return true;
-}
-
-std::set<std::string>
-NotificationUIManagerAndroid::GetAllIdsByProfileAndSourceOrigin(
-    Profile* profile,
-    const GURL& source) {
-  // |profile| may be invalid, so no calls must be made based on the instance.
-  std::set<std::string> delegate_ids;
-
-  for (auto iterator : profile_notifications_) {
-    ProfileNotification* profile_notification = iterator.second;
-    if (profile_notification->notification().origin_url() == source &&
-        profile_notification->profile() == profile)
-      delegate_ids.insert(profile_notification->notification().id());
-  }
-
-  return delegate_ids;
-}
-
-bool NotificationUIManagerAndroid::CancelAllBySourceOrigin(
-    const GURL& source_origin) {
-  bool removed = true;
-
-  for (auto iterator = profile_notifications_.begin();
-       iterator != profile_notifications_.end();) {
-    auto current_iterator = iterator++;
-
-    ProfileNotification* profile_notification = current_iterator->second;
-    if (profile_notification->notification().origin_url() == source_origin) {
-      RemoveProfileNotification(profile_notification, true /* close */);
-      removed = true;
-    }
-  }
-
-  return removed;
-}
-
-bool NotificationUIManagerAndroid::CancelAllByProfile(ProfileID profile_id) {
-  bool removed = true;
-
-  for (auto iterator = profile_notifications_.begin();
-       iterator != profile_notifications_.end();) {
-    auto current_iterator = iterator++;
-
-    ProfileNotification* profile_notification = current_iterator->second;
-    ProfileID current_profile_id =
-        NotificationUIManager::GetProfileID(profile_notification->profile());
-    if (current_profile_id == profile_id) {
-      RemoveProfileNotification(profile_notification, true /* close */);
-      removed = true;
-    }
-  }
-
-  return removed;
-}
-
-void NotificationUIManagerAndroid::CancelAll() {
-  for (auto iterator : profile_notifications_) {
-    ProfileNotification* profile_notification = iterator.second;
-
-    PlatformCloseNotification(profile_notification->notification().id());
-    delete profile_notification;
-  }
-
-  profile_notifications_.clear();
-}
-
-bool NotificationUIManagerAndroid::RegisterNotificationUIManager(JNIEnv* env) {
-  return RegisterNativesImpl(env);
-}
-
-void NotificationUIManagerAndroid::PlatformCloseNotification(
-    const std::string& notification_id) {
   int64_t persistent_notification_id = 0;
-  if (!base::StringToInt64(notification_id, &persistent_notification_id))
-    return;
+  if (!base::StringToInt64(delegate_id, &persistent_notification_id))

[johnme, 2015/04/09 17:00:01]

Nit: please add a TODO about changing delegate_ids in content/.

[Peter Beverloo, 2015/04/09 17:34:42]

Done.

+    return false;
 
   const auto iterator =
       regenerated_notification_infos_.find(persistent_notification_id);
   if (iterator == regenerated_notification_infos_.end())
-    return;
+    return false;
+
+  const RegeneratedNotificationInfo& notification_info = iterator->second;
 
-  RegeneratedNotificationInfo notification_info = iterator->second;
   JNIEnv* env = AttachCurrentThread();
 
   ScopedJavaLocalRef<jstring> origin =
@@ -291,53 +175,32 @@ void NotificationUIManagerAndroid::PlatformCloseNotification(
                                                persistent_notification_id,
                                                origin.obj(),
                                                tag.obj());
+  return true;
 }
 
-void NotificationUIManagerAndroid::AddProfileNotification(
-    ProfileNotification* profile_notification) {
-  std::string id = profile_notification->notification().id();
-
-  // Notification ids should be unique.
-  DCHECK(profile_notifications_.find(id) == profile_notifications_.end());
-
-  profile_notifications_[id] = profile_notification;
+std::set<std::string>
+NotificationUIManagerAndroid::GetAllIdsByProfileAndSourceOrigin(
+    Profile* profile,
+    const GURL& source) {
+  NOTIMPLEMENTED();

[johnme, 2015/04/09 17:00:01]

This is currently used by PushMessagingServiceImpl::RequireUserVisibleUX. One of
us should update that to use the notifications database.

[Peter Beverloo, 2015/04/09 17:34:42]

There's already a row for that on the spreadsheet, because we're aware about it
being unreliable. I'm happy to take it tomorrow unless you're jumping to?

[johnme, 2015/04/10 11:37:47]

There's a difference between unreliable-across-restarts and totally broken ;)
But we're far from a branch point, so I'm happy for that to be a follow-up
patch, that I'll make sure to chase up.

+  return std::set<std::string>();
 }
 
-void NotificationUIManagerAndroid::RemoveProfileNotification(
-    ProfileNotification* profile_notification, bool close) {
-  std::string notification_id = profile_notification->notification().id();
-  if (close)
-    PlatformCloseNotification(notification_id);
-
-  profile_notifications_.erase(notification_id);
-  delete profile_notification;
+bool NotificationUIManagerAndroid::CancelAllBySourceOrigin(
+    const GURL& source_origin) {
+  NOTIMPLEMENTED();
+  return false;
 }
 
-ProfileNotification* NotificationUIManagerAndroid::FindProfileNotification(
-    const std::string& id) const {
-  auto iter = profile_notifications_.find(id);
-  if (iter == profile_notifications_.end())
-    return nullptr;
+bool NotificationUIManagerAndroid::CancelAllByProfile(ProfileID profile_id) {
+  NOTIMPLEMENTED();

[johnme, 2015/04/09 17:00:01]

Doesn't not implementing this mean that NotificationSystemObserver::Observe[1]
will fail to close incognito notifications when the last incognito tab is
closed?

Why don't we make these methods (at least the CancelAll* ones) work by reading
the IDs from the database? It doesn't matter if you try to cancel a notification
the user has already swiped away...

[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/not...

[Peter Beverloo, 2015/04/09 17:34:42]

I'm not sure what our incognito story is right now - it's definitely not well
thought out or tested.

Using the notification database only for persistent notifications was a
trade-off we made a couple of weeks ago. The door's not closed for extensions
and/or other notifications to be part of this, but it'd be inherently incomplete
because we create a bunch of notifications on Android in Java as well, whereas
on desktop this is the only entry point.

[johnme, 2015/04/10 11:37:47]

Ok, we chatted offline and you explained that the notifications database will
get cleared when exiting incognito, which will cause the notifications to be
closed (though that last part isn't yet hooked up - added a task to the
spreadsheet). Sounds reasonable.

+  return false;
+}
 
-  return iter->second;
+void NotificationUIManagerAndroid::CancelAll() {
+  NOTIMPLEMENTED();
 }
 
-ProfileNotification* NotificationUIManagerAndroid::FindNotificationToReplace(
-    const Notification& notification, Profile* profile) const {
-  const std::string& tag = notification.tag();
-  if (tag.empty())
-    return nullptr;
-
-  const GURL origin_url = notification.origin_url();
-  DCHECK(origin_url.is_valid());
-
-  for (const auto& iterator : profile_notifications_) {
-    ProfileNotification* profile_notification = iterator.second;
-    if (profile_notification->notification().tag() == tag ||
-        profile_notification->notification().origin_url() == origin_url ||
-        profile_notification->profile() == profile) {
-      return profile_notification;
-    }
-  }
-  return nullptr;
+bool NotificationUIManagerAndroid::RegisterNotificationUIManager(JNIEnv* env) {
+  return RegisterNativesImpl(env);
 }