Get rid of message_center::ButtonType enum.

Intuit the button type from the presence or absence of the placeholder
text.

Bug: 797128
Change-Id: I6fe658f644e42b0556ffcaac9bc7322f44f88fea
Reviewed-on: https://chromium-review.googlesource.com/900184Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534704}

chrome/browser/notifications/notification_platform_bridge_android.cc

@@ -67,14 +67,8 @@ ScopedJavaLocalRef<jobject> JNI_NotificationPlatformBridge_ConvertToJavaBitmap(
 
 NotificationActionType GetNotificationActionType(
     message_center::ButtonInfo button) {
-  switch (button.type) {
-    case message_center::ButtonType::BUTTON:
-      return NotificationActionType::BUTTON;
-    case message_center::ButtonType::TEXT:
-      return NotificationActionType::TEXT;
-  }
-  NOTREACHED();
-  return NotificationActionType::TEXT;
+  return button.placeholder ? NotificationActionType::TEXT
+                            : NotificationActionType::BUTTON;
 }
 
 ScopedJavaLocalRef<jobjectArray> ConvertToJavaActionInfos(
@@ -91,8 +85,11 @@ ScopedJavaLocalRef<jobjectArray> ConvertToJavaActionInfos(
     ScopedJavaLocalRef<jstring> title =
         base::android::ConvertUTF16ToJavaString(env, button.title);
     int type = GetNotificationActionType(button);
-    ScopedJavaLocalRef<jstring> placeholder =
-        base::android::ConvertUTF16ToJavaString(env, button.placeholder);
+    ScopedJavaLocalRef<jstring> placeholder;
+    if (button.placeholder) {
+      placeholder =
+          base::android::ConvertUTF16ToJavaString(env, *button.placeholder);
+    }
     ScopedJavaLocalRef<jobject> icon =
         JNI_NotificationPlatformBridge_ConvertToJavaBitmap(env, button.icon);
     ScopedJavaLocalRef<jobject> action_info = Java_ActionInfo_createActionInfo(

chrome/browser/notifications/notification_template_builder.cc

@@ -295,11 +295,11 @@ void NotificationTemplateBuilder::AddActions(
   bool inline_reply = false;
   std::string placeholder;
   for (const auto& button : buttons) {
-    if (button.type != message_center::ButtonType::TEXT)
+    if (!button.placeholder)
       continue;
 
     inline_reply = true;
-    placeholder = base::UTF16ToUTF8(button.placeholder);
+    placeholder = base::UTF16ToUTF8(*button.placeholder);
     break;
   }
 

chrome/browser/notifications/notification_template_builder_unittest.cc

@@ -155,7 +155,6 @@ TEST_F(NotificationTemplateBuilderTest, InlineReplies) {
 
   std::vector<message_center::ButtonInfo> buttons;
   message_center::ButtonInfo button1(base::ASCIIToUTF16("Button1"));
-  button1.type = message_center::ButtonType::TEXT;
   button1.placeholder = base::ASCIIToUTF16("Reply here");
   buttons.emplace_back(button1);
   buttons.emplace_back(base::ASCIIToUTF16("Button2"));
@@ -188,11 +187,9 @@ TEST_F(NotificationTemplateBuilderTest, InlineRepliesDoubleInput) {
 
   std::vector<message_center::ButtonInfo> buttons;
   message_center::ButtonInfo button1(base::ASCIIToUTF16("Button1"));
-  button1.type = message_center::ButtonType::TEXT;
   button1.placeholder = base::ASCIIToUTF16("Reply here");
   buttons.emplace_back(button1);
   message_center::ButtonInfo button2(base::ASCIIToUTF16("Button2"));
-  button2.type = message_center::ButtonType::TEXT;
   button2.placeholder = base::ASCIIToUTF16("Should not appear");
   buttons.emplace_back(button2);
   notification->set_buttons(buttons);
@@ -225,7 +222,6 @@ TEST_F(NotificationTemplateBuilderTest, InlineRepliesTextTypeNotFirst) {
   std::vector<message_center::ButtonInfo> buttons;
   buttons.emplace_back(base::ASCIIToUTF16("Button1"));
   message_center::ButtonInfo button2(base::ASCIIToUTF16("Button2"));
-  button2.type = message_center::ButtonType::TEXT;
   button2.placeholder = base::ASCIIToUTF16("Reply here");
   buttons.emplace_back(button2);
   notification->set_buttons(buttons);
@@ -369,7 +365,6 @@ TEST_F(NotificationTemplateBuilderTest, Images) {
 
   std::vector<message_center::ButtonInfo> buttons;
   message_center::ButtonInfo button(base::ASCIIToUTF16("Button1"));
-  button.type = message_center::ButtonType::TEXT;
   button.placeholder = base::ASCIIToUTF16("Reply here");
   button.icon = gfx::Image::CreateFrom1xBitmap(icon);
   buttons.emplace_back(button);

chrome/browser/notifications/platform_notification_service_impl.cc

@@ -540,17 +540,9 @@ PlatformNotificationServiceImpl::CreateNotificationFromData(
     // the 1x bitmap - crbug.com/585815.
     button.icon =
         gfx::Image::CreateFrom1xBitmap(notification_resources.action_icons[i]);
-    button.placeholder =
-        action.placeholder.is_null()
-            ? l10n_util::GetStringUTF16(IDS_NOTIFICATION_REPLY_PLACEHOLDER)
-            : action.placeholder.string();
-    switch (action.type) {
-      case content::PLATFORM_NOTIFICATION_ACTION_TYPE_BUTTON:
-        button.type = message_center::ButtonType::BUTTON;
-        break;
-      case content::PLATFORM_NOTIFICATION_ACTION_TYPE_TEXT:
-        button.type = message_center::ButtonType::TEXT;
-        break;
+    if (action.type == content::PLATFORM_NOTIFICATION_ACTION_TYPE_TEXT) {
+      button.placeholder = action.placeholder.as_optional_string16().value_or(
+          l10n_util::GetStringUTF16(IDS_NOTIFICATION_REPLY_PLACEHOLDER));
     }
     buttons.push_back(button);
   }

chrome/browser/notifications/platform_notification_service_unittest.cc

@@ -247,9 +247,9 @@ TEST_F(PlatformNotificationServiceTest, DisplayPersistentPropertiesMatch) {
   const auto& buttons = notification.buttons();
   ASSERT_EQ(2u, buttons.size());
   EXPECT_EQ("Button 1", base::UTF16ToUTF8(buttons[0].title));
-  EXPECT_EQ(message_center::ButtonType::BUTTON, buttons[0].type);
+  EXPECT_FALSE(buttons[0].placeholder);
   EXPECT_EQ("Button 2", base::UTF16ToUTF8(buttons[1].title));
-  EXPECT_EQ(message_center::ButtonType::TEXT, buttons[1].type);
+  EXPECT_TRUE(buttons[1].placeholder);
 }
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)

ui/message_center/public/cpp/notification.h

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "base/values.h"
@@ -45,15 +46,6 @@ struct MESSAGE_CENTER_PUBLIC_EXPORT NotificationItem {
   base::string16 message;
 };
 
-enum class ButtonType {
-  // A simple button having an icon and a title that the user can click on.
-  BUTTON,
-
-  // A button having an icon and a title that should also enable the user to
-  // input text, enabling them to quickly respond from the notification.
-  TEXT
-};
-
 enum class SettingsButtonHandler {
   NONE,     // No button. This is the default.
   TRAY,     // Button shown, the tray handles clicks. Only used on Chrome OS.
@@ -77,12 +69,10 @@ struct MESSAGE_CENTER_PUBLIC_EXPORT ButtonInfo {
   // requirements of the notification.
   gfx::Image icon;
 
-  // Type of this button.
-  ButtonType type = ButtonType::BUTTON;
-
-  // The placeholder string that should be displayed in the input field for TEXT
-  // type buttons until the user has entered a response themselves.
-  base::string16 placeholder;
+  // The placeholder string that should be displayed in the input field for
+  // text input type buttons until the user has entered a response themselves.
+  // If the value is null, there is no input field associated with the button.
+  base::Optional<base::string16> placeholder;
 };
 
 // TODO(estade): add an ALWAYS value to mark notifications as additionally

ui/message_center/views/notification_view_md.cc

@@ -329,14 +329,13 @@ const char* LargeImageContainerView::GetClassName() const {
 
 // NotificationButtonMD ////////////////////////////////////////////////////////
 
-NotificationButtonMD::NotificationButtonMD(views::ButtonListener* listener,
-                                           bool is_inline_reply,
-                                           const base::string16& label,
-                                           const base::string16& placeholder)
+NotificationButtonMD::NotificationButtonMD(
+    views::ButtonListener* listener,
+    const base::string16& label,
+    const base::Optional<base::string16>& placeholder)
     : views::LabelButton(listener,
                          base::i18n::ToUpper(label),
                          views::style::CONTEXT_BUTTON_MD),
-      is_inline_reply_(is_inline_reply),
       placeholder_(placeholder) {
   SetHorizontalAlignment(gfx::ALIGN_CENTER);
   SetInkDropMode(views::LabelButton::InkDropMode::ON);
@@ -783,10 +782,10 @@ void NotificationViewMD::ButtonPressed(views::Button* sender,
   for (size_t i = 0; i < action_buttons_.size(); ++i) {
     if (sender != action_buttons_[i])
       continue;
-    if (action_buttons_[i]->is_inline_reply()) {
+    if (action_buttons_[i]->placeholder()) {
       inline_reply_->textfield()->set_index(i);
       inline_reply_->textfield()->set_placeholder(
-          action_buttons_[i]->placeholder());
+          *action_buttons_[i]->placeholder());
       inline_reply_->textfield()->RequestFocus();
       inline_reply_->AnimateBackground(*event.AsLocatedEvent());
       inline_reply_->SetVisible(true);
@@ -1048,7 +1047,7 @@ void NotificationViewMD::CreateOrUpdateImageView(
 
 void NotificationViewMD::CreateOrUpdateActionButtonViews(
     const Notification& notification) {
-  std::vector<ButtonInfo> buttons = notification.buttons();
+  const std::vector<ButtonInfo>& buttons = notification.buttons();
   bool new_buttons = action_buttons_.size() != buttons.size();
 
   if (new_buttons || buttons.size() == 0) {
@@ -1062,9 +1061,8 @@ void NotificationViewMD::CreateOrUpdateActionButtonViews(
   for (size_t i = 0; i < buttons.size(); ++i) {
     ButtonInfo button_info = buttons[i];
     if (new_buttons) {
-      bool is_inline_reply = button_info.type == ButtonType::TEXT;
       NotificationButtonMD* button = new NotificationButtonMD(
-          this, is_inline_reply, button_info.title, button_info.placeholder);
+          this, button_info.title, button_info.placeholder);
       action_buttons_.push_back(button);
       action_buttons_row_->AddChildView(button);
     } else {
@@ -1130,8 +1128,8 @@ void NotificationViewMD::CreateOrUpdateInlineSettingsViews(
   settings_row_->SetVisible(false);
 
   settings_done_button_ = new NotificationButtonMD(
-      this, false, l10n_util::GetStringUTF16(IDS_MESSAGE_CENTER_SETTINGS_DONE),
-      base::EmptyString16());
+      this, l10n_util::GetStringUTF16(IDS_MESSAGE_CENTER_SETTINGS_DONE),
+      base::nullopt);
   auto* settings_button_row = new views::View;
   auto settings_button_layout = std::make_unique<views::BoxLayout>(
       views::BoxLayout::kHorizontal, kSettingsButtonRowPadding, 0);

ui/message_center/views/notification_view_md.h

@@ -9,6 +9,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "ui/message_center/message_center_export.h"
 #include "ui/message_center/views/message_view.h"
 #include "ui/views/controls/button/button.h"
@@ -114,9 +115,8 @@ class NotificationButtonMD : public views::LabelButton {
   // |placeholder| is placeholder text shown on the input field. Only used when
   // |is_inline_reply| is true.
   NotificationButtonMD(views::ButtonListener* listener,
-                       bool is_inline_reply,
                        const base::string16& label,
-                       const base::string16& placeholder);
+                       const base::Optional<base::string16>& placeholder);
   ~NotificationButtonMD() override;
 
   void SetText(const base::string16& text) override;
@@ -127,12 +127,12 @@ class NotificationButtonMD : public views::LabelButton {
 
   SkColor enabled_color_for_testing() { return label()->enabled_color(); }
 
-  bool is_inline_reply() const { return is_inline_reply_; }
-  const base::string16& placeholder() const { return placeholder_; }
+  const base::Optional<base::string16>& placeholder() const {
+    return placeholder_;
+  }
 
  private:
-  const bool is_inline_reply_;
-  const base::string16 placeholder_;
+  const base::Optional<base::string16> placeholder_;
 
   DISALLOW_COPY_AND_ASSIGN(NotificationButtonMD);
 };

ui/message_center/views/notification_view_md_unittest.cc

@@ -472,7 +472,7 @@ TEST_F(NotificationViewMDTest, TestInlineReply) {
   delegate_->set_expecting_reply_submission(true);
 
   std::vector<ButtonInfo> buttons = CreateButtons(2);
-  buttons[1].type = ButtonType::TEXT;
+  buttons[1].placeholder = base::string16();
   notification()->set_buttons(buttons);
   UpdateNotificationViews();
   widget()->Show();