Plumbing in notification replies: PlatformNotificationService -> SW

chrome/browser/notifications/persistent_notification_delegate.cc

Index: chrome/browser/notifications/persistent_notification_delegate.cc
diff --git a/chrome/browser/notifications/persistent_notification_delegate.cc b/chrome/browser/notifications/persistent_notification_delegate.cc
index 1e05cbf76f76557777370a98758c20a37c70e71d..91c26b7a5495639ca8461d4cf687338353b763e9 100644
--- a/chrome/browser/notifications/persistent_notification_delegate.cc
+++ b/chrome/browser/notifications/persistent_notification_delegate.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/notifications/persistent_notification_delegate.h"
 
+#include "base/strings/nullable_string16.h"
+#include "base/strings/string16.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "url/gurl.h"
 
@@ -26,7 +28,8 @@ void PersistentNotificationDelegate::Close(bool by_user) {
 
 void PersistentNotificationDelegate::Click() {
   PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick(
-      browser_context(), id(), origin(), -1 /* action_index */);
+      browser_context(), id(), origin(), -1 /* action_index */,
+      base::NullableString16() /* reply */);
 }
 
 void PersistentNotificationDelegate::ButtonClick(int button_index) {
@@ -37,5 +40,20 @@ void PersistentNotificationDelegate::ButtonClick(int button_index) {
   }
 
   PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick(
-      browser_context(), id(), origin(), button_index);
+      browser_context(), id(), origin(), button_index,
+      base::NullableString16() /* reply */);
+}
+
+void PersistentNotificationDelegate::ButtonClickWithReply(
+    int button_index,
+    const base::string16& reply) {
+  DCHECK_GE(button_index, 0);
+  if (button_index == notification_settings_index_) {

[Peter Beverloo, 2016/10/11 14:22:56]

DCHECK_NE? Why would the site settings button take a reply?

[awdf, 2016/10/11 15:51:27]

oops - this came from copy pasting the method above without really thinking.
You're right it doesn't make sense to do this check if we know this came from a
notification reply.

But if we unify ButtonClick and ButtonClickWithReply, it's irrelevant.

(I did start trying to merge ButtonClick and ButtonClickWithReply last week, but
it got a little bit gnarly so I abandoned it. I still have the local branch
though so I'll give it another go..)

[awdf, 2016/10/12 10:44:58]

I remember why it's gnarly now - it's because there's 39 overrides of
NotificationDelegate.ButtonClick, most of which won't ever care about the reply
parameter (a lot don't even care about the button index)
https://cs.chromium.org/chromium/src/ui/message_center/notification_delegate....
. But perhaps someday some of them might I suppose, and it's probably less
confusing to have a single method for button clicks.. I'll press on (shout now
if you change your mind though!)

[awdf, 2016/10/12 13:26:42]

Done (removed the unnecessary settings check, but didn't unify the methods, as
discussed).

+    NotificationCommon::OpenNotificationSettings(browser_context());
+    return;
+  }
+
+  PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick(
+      browser_context(), id(), origin(), button_index,
+      base::NullableString16(reply, false /* is_null */));
 }