Plumbing in notification replies: PlatformNotificationService -> SW

chrome/browser/notifications/persistent_notification_delegate.cc

 // Copyright 2014 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/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"
 
 PersistentNotificationDelegate::PersistentNotificationDelegate(
     content::BrowserContext* browser_context,
     const std::string& notification_id,
     const GURL& origin,
     int notification_settings_index)
     : WebNotificationDelegate(browser_context, notification_id, origin),
       notification_settings_index_(notification_settings_index) {}
 
 PersistentNotificationDelegate::~PersistentNotificationDelegate() {}
 
 void PersistentNotificationDelegate::Display() {}
 
 void PersistentNotificationDelegate::Close(bool by_user) {
   PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClose(
       browser_context(), id(), origin(), 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) {
   DCHECK_GE(button_index, 0);
   if (button_index == notification_settings_index_) {
     NotificationCommon::OpenNotificationSettings(browser_context());
     return;
   }
 
   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 */));
+}