Replace EditableComboboxListener with a callback.

Bug: 1037992
Change-Id: I19a134d72f72dfbcdc698af907fadaac4cce3be0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2328093
Commit-Queue: Jongmok Kim <jongmok.kim@navercorp.com>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793524}

chrome/browser/ui/views/passwords/password_save_update_view.cc

@@ -287,10 +287,12 @@ PasswordSaveUpdateView::PasswordSaveUpdateView(
   } else {
     std::unique_ptr<views::EditableCombobox> username_dropdown =
         CreateUsernameEditableCombobox(password_form);
-    username_dropdown->set_listener(this);
+    username_dropdown->set_callback(base::BindRepeating(
+        &PasswordSaveUpdateView::OnContentChanged, base::Unretained(this)));
     std::unique_ptr<views::EditableCombobox> password_dropdown =
         CreatePasswordEditableCombobox(password_form, are_passwords_revealed_);
-    password_dropdown->set_listener(this);
+    password_dropdown->set_callback(base::BindRepeating(
+        &PasswordSaveUpdateView::OnContentChanged, base::Unretained(this)));
 
     std::unique_ptr<views::ToggleImageButton> password_view_button =
         CreatePasswordViewButton(this, are_passwords_revealed_);
@@ -337,39 +339,12 @@ bool PasswordSaveUpdateView::Accept() {
   return true;
 }
 
-void PasswordSaveUpdateView::OnDialogCancelled() {
-  UpdateUsernameAndPasswordInModel();
-  if (is_update_bubble_) {
-    controller_.OnNopeUpdateClicked();
-  } else {
-    controller_.OnNeverForThisSiteClicked();
-  }
-}
-
 void PasswordSaveUpdateView::ButtonPressed(views::Button* sender,
                                            const ui::Event& event) {
   DCHECK(sender == password_view_button_);
   TogglePasswordVisibility();
 }
 
-void PasswordSaveUpdateView::OnContentChanged(
-    views::EditableCombobox* editable_combobox) {
-  bool is_update_state_before = controller_.IsCurrentStateUpdate();
-  bool is_ok_button_enabled_before =
-      IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK);
-  UpdateUsernameAndPasswordInModel();
-  // Maybe the buttons should be updated.
-  if (is_update_state_before != controller_.IsCurrentStateUpdate() ||
-      is_ok_button_enabled_before !=
-          IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK)) {
-    UpdateBubbleUIElements();
-    DialogModelChanged();
-    // TODO(ellyjones): This should not be necessary; DialogModelChanged()
-    // implies a re-layout of the dialog.
-    GetWidget()->GetRootView()->Layout();
-  }
-}
-
 gfx::Size PasswordSaveUpdateView::CalculatePreferredSize() const {
   const int width = ChromeLayoutProvider::Get()->GetDistanceMetric(
                         DISTANCE_BUBBLE_PREFERRED_WIDTH) -
@@ -517,3 +492,28 @@ std::unique_ptr<views::View> PasswordSaveUpdateView::CreateFooterView() {
   label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   return label;
 }
+
+void PasswordSaveUpdateView::OnDialogCancelled() {
+  UpdateUsernameAndPasswordInModel();
+  if (is_update_bubble_)
+    controller_.OnNopeUpdateClicked();
+  else
+    controller_.OnNeverForThisSiteClicked();
+}
+
+void PasswordSaveUpdateView::OnContentChanged() {
+  bool is_update_state_before = controller_.IsCurrentStateUpdate();
+  bool is_ok_button_enabled_before =
+      IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK);
+  UpdateUsernameAndPasswordInModel();
+  // Maybe the buttons should be updated.
+  if (is_update_state_before != controller_.IsCurrentStateUpdate() ||
+      is_ok_button_enabled_before !=
+          IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK)) {
+    UpdateBubbleUIElements();
+    DialogModelChanged();
+    // TODO(ellyjones): This should not be necessary; DialogModelChanged()
+    // implies a re-layout of the dialog.
+    GetWidget()->GetRootView()->Layout();
+  }
+}

chrome/browser/ui/views/passwords/password_save_update_view.h

@@ -8,7 +8,6 @@
 #include "chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.h"
 #include "chrome/browser/ui/views/passwords/password_bubble_view_base.h"
 #include "ui/views/controls/button/button.h"
-#include "ui/views/controls/editable_combobox/editable_combobox_listener.h"
 #include "ui/views/view.h"
 
 namespace views {
@@ -22,8 +21,7 @@ class PasswordSignInPromoView;
 // on |is_update_bubble|). Contains a username and password field, along with a
 // "Save"/"Update" button and a "Never"/"Nope" button.
 class PasswordSaveUpdateView : public PasswordBubbleViewBase,
-                               public views::ButtonListener,
-                               public views::EditableComboboxListener {
+                               public views::ButtonListener {
  public:
   PasswordSaveUpdateView(content::WebContents* web_contents,
                          views::View* anchor_view,
@@ -41,10 +39,6 @@ class PasswordSaveUpdateView : public PasswordBubbleViewBase,
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
-  // views::EditableComboboxListener:
-  // Used for both the username and password editable comboboxes.
-  void OnContentChanged(views::EditableCombobox* editable_combobox) override;
-
   // PasswordBubbleViewBase:
   gfx::Size CalculatePreferredSize() const override;
   views::View* GetInitiallyFocusedView() override;
@@ -65,6 +59,9 @@ class PasswordSaveUpdateView : public PasswordBubbleViewBase,
   std::unique_ptr<views::View> CreateFooterView();
   void OnDialogCancelled();
 
+  // Used for both the username and password editable comboboxes.
+  void OnContentChanged();
+
   SaveUpdateBubbleController controller_;
 
   // True iff it is an update password bubble on creation. False iff it is a

chrome/browser/ui/views/passwords/password_save_update_with_account_store_view.cc

@@ -399,10 +399,14 @@ PasswordSaveUpdateWithAccountStoreView::PasswordSaveUpdateWithAccountStoreView(
   } else {
     std::unique_ptr<views::EditableCombobox> username_dropdown =
         CreateUsernameEditableCombobox(password_form);
-    username_dropdown->set_listener(this);
+    username_dropdown->set_callback(base::BindRepeating(
+        &PasswordSaveUpdateWithAccountStoreView::OnContentChanged,
+        base::Unretained(this)));
     std::unique_ptr<views::EditableCombobox> password_dropdown =
         CreatePasswordEditableCombobox(password_form, are_passwords_revealed_);
-    password_dropdown->set_listener(this);
+    password_dropdown->set_callback(base::BindRepeating(
+        &PasswordSaveUpdateWithAccountStoreView::OnContentChanged,
+        base::Unretained(this)));
     std::unique_ptr<views::ToggleImageButton> password_view_button =
         CreatePasswordViewButton(this, are_passwords_revealed_);
     // Set up layout:
@@ -509,21 +513,6 @@ void PasswordSaveUpdateWithAccountStoreView::OnPerformAction(
     CloseIPHBubbleIfOpen();
 }
 
-void PasswordSaveUpdateWithAccountStoreView::OnContentChanged(
-    views::EditableCombobox* editable_combobox) {
-  bool is_update_state_before = controller_.IsCurrentStateUpdate();
-  bool is_ok_button_enabled_before =
-      IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK);
-  UpdateUsernameAndPasswordInModel();
-  // Maybe the buttons should be updated.
-  if (is_update_state_before != controller_.IsCurrentStateUpdate() ||
-      is_ok_button_enabled_before !=
-          IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK)) {
-    UpdateBubbleUIElements();
-    DialogModelChanged();
-  }
-}
-
 void PasswordSaveUpdateWithAccountStoreView::OnWidgetDestroying(
     views::Widget* widget) {
   // IPH bubble is getting closed.
@@ -748,3 +737,17 @@ void PasswordSaveUpdateWithAccountStoreView::CloseIPHBubbleIfOpen() {
     return;
   account_storage_promo_->CloseBubble();
 }
+
+void PasswordSaveUpdateWithAccountStoreView::OnContentChanged() {
+  bool is_update_state_before = controller_.IsCurrentStateUpdate();
+  bool is_ok_button_enabled_before =
+      IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK);
+  UpdateUsernameAndPasswordInModel();
+  // Maybe the buttons should be updated.
+  if (is_update_state_before != controller_.IsCurrentStateUpdate() ||
+      is_ok_button_enabled_before !=
+          IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK)) {
+    UpdateBubbleUIElements();
+    DialogModelChanged();
+  }
+}

chrome/browser/ui/views/passwords/password_save_update_with_account_store_view.h

@@ -9,7 +9,6 @@
 #include "chrome/browser/ui/views/passwords/password_bubble_view_base.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/combobox/combobox_listener.h"
-#include "ui/views/controls/editable_combobox/editable_combobox_listener.h"
 #include "ui/views/layout/animating_layout_manager.h"
 #include "ui/views/view.h"
 
@@ -34,7 +33,6 @@ class FeaturePromoBubbleView;
 class PasswordSaveUpdateWithAccountStoreView
     : public PasswordBubbleViewBase,
       public views::ButtonListener,
-      public views::EditableComboboxListener,
       public views::ComboboxListener,
       public views::WidgetObserver,
       public views::AnimatingLayoutManager::Observer {
@@ -71,10 +69,6 @@ class PasswordSaveUpdateWithAccountStoreView
   // Used for the destination combobox.
   void OnPerformAction(views::Combobox* combobox) override;
 
-  // views::EditableComboboxListener:
-  // Used for both the username and password editable comboboxes.
-  void OnContentChanged(views::EditableCombobox* editable_combobox) override;
-
   // views::WidgetObserver:
   void OnWidgetDestroying(views::Widget* widget) override;
 
@@ -114,6 +108,9 @@ class PasswordSaveUpdateWithAccountStoreView
 
   void CloseIPHBubbleIfOpen();
 
+  // Used for both the username and password editable comboboxes.
+  void OnContentChanged();
+
   SaveUpdateWithAccountStoreBubbleController controller_;
 
   // True iff it is an update password bubble on creation. False iff it is a

ui/views/BUILD.gn

@@ -125,7 +125,6 @@ jumbo_component("views") {
     "controls/combobox/combobox_listener.h",
     "controls/combobox/combobox_util.h",
     "controls/editable_combobox/editable_combobox.h",
-    "controls/editable_combobox/editable_combobox_listener.h",
     "controls/focus_ring.h",
     "controls/focusable_border.h",
     "controls/highlight_path_generator.h",

ui/views/controls/editable_combobox/editable_combobox.cc

@@ -5,7 +5,6 @@
 #include "ui/views/controls/editable_combobox/editable_combobox.h"
 
 #include <memory>
-#include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ -45,7 +44,6 @@
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/button_controller.h"
 #include "ui/views/controls/combobox/combobox_util.h"
-#include "ui/views/controls/editable_combobox/editable_combobox_listener.h"
 #include "ui/views/controls/menu/menu_config.h"
 #include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/controls/menu/menu_types.h"
@@ -475,15 +473,15 @@ void EditableCombobox::OnItemSelected(int index) {
 
 void EditableCombobox::HandleNewContent(const base::string16& new_content) {
   DCHECK(GetText() == new_content);
-  // We notify |listener_| before updating |menu_model_|'s items shown. This
+  // We notify |callback_| before updating |menu_model_|'s items shown. This
   // gives the user a chance to modify the ComboboxModel beforehand if they wish
   // to do so.
   // We disable UpdateItemsShown while we notify the listener in case it
   // modifies the ComboboxModel, then calls OnComboboxModelChanged and thus
   // UpdateItemsShown. That way UpdateItemsShown doesn't do its work twice.
-  if (listener_) {
+  if (!content_changed_callback_.is_null()) {
     menu_model_->DisableUpdateItemsShown();
-    listener_->OnContentChanged(this);
+    content_changed_callback_.Run();
     menu_model_->EnableUpdateItemsShown();
   }
   menu_model_->UpdateItemsShown();

ui/views/controls/editable_combobox/editable_combobox.h

@@ -6,7 +6,9 @@
 #define UI_VIEWS_CONTROLS_EDITABLE_COMBOBOX_EDITABLE_COMBOBOX_H_
 
 #include <memory>
+#include <utility>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/scoped_observer.h"
 #include "base/strings/string16.h"
@@ -32,7 +34,6 @@ class Event;
 
 namespace views {
 class EditableComboboxMenuModel;
-class EditableComboboxListener;
 class EditableComboboxPreTargetHandler;
 class MenuRunner;
 class Textfield;
@@ -80,9 +81,8 @@ class VIEWS_EXPORT EditableCombobox
 
   const gfx::FontList& GetFontList() const;
 
-  // Sets the listener that we will call when a selection is made.
-  void set_listener(EditableComboboxListener* listener) {
-    listener_ = listener;
+  void set_callback(base::RepeatingClosure callback) {
+    content_changed_callback_ = std::move(callback);
   }
 
   // Selects the specified logical text range for the textfield.
@@ -170,8 +170,7 @@ class VIEWS_EXPORT EditableCombobox
   // Set while the drop-down is showing.
   std::unique_ptr<MenuRunner> menu_runner_;
 
-  // Our listener. Not owned. Notified when the selected index changes.
-  EditableComboboxListener* listener_ = nullptr;
+  base::RepeatingClosure content_changed_callback_;
 
   // Whether we are currently showing the passwords for type
   // Type::kPassword.

ui/views/controls/editable_combobox/editable_combobox_listener.h

-// Copyright 2019 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.
-
-#ifndef UI_VIEWS_CONTROLS_EDITABLE_COMBOBOX_EDITABLE_COMBOBOX_LISTENER_H_
-#define UI_VIEWS_CONTROLS_EDITABLE_COMBOBOX_EDITABLE_COMBOBOX_LISTENER_H_
-
-#include "ui/views/views_export.h"
-
-namespace views {
-
-class EditableCombobox;
-
-// Interface used to notify consumers when something interesting happens to an
-// EditableCombobox.
-class VIEWS_EXPORT EditableComboboxListener {
- public:
-  // Called when the content of the main textfield changes, either as the user
-  // types or after selecting an option from the menu.
-  virtual void OnContentChanged(EditableCombobox* editable_combobox) = 0;
-
- protected:
-  virtual ~EditableComboboxListener() = default;
-};
-
-}  // namespace views
-
-#endif  // UI_VIEWS_CONTROLS_EDITABLE_COMBOBOX_EDITABLE_COMBOBOX_LISTENER_H_

ui/views/controls/editable_combobox/editable_combobox_unittest.cc

@@ -9,6 +9,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "base/strings/stringprintf.h"
@@ -28,7 +29,6 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/render_text.h"
 #include "ui/views/context_menu_controller.h"
-#include "ui/views/controls/editable_combobox/editable_combobox_listener.h"
 #include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/test/menu_test_utils.h"
@@ -43,22 +43,6 @@ namespace {
 using base::ASCIIToUTF16;
 using views::test::WaitForMenuClosureAnimation;
 
-class DummyListener : public EditableComboboxListener {
- public:
-  DummyListener() = default;
-  ~DummyListener() override = default;
-  void OnContentChanged(EditableCombobox* editable_combobox) override {
-    change_count_++;
-  }
-
-  int change_count() const { return change_count_; }
-
- private:
-  int change_count_ = 0;
-
-  DISALLOW_COPY_AND_ASSIGN(DummyListener);
-};
-
 // No-op test double of a ContextMenuController
 class TestContextMenuController : public ContextMenuController {
  public:
@@ -116,6 +100,9 @@ class EditableComboboxTest : public ViewsTestBase {
                     bool shift = false,
                     bool ctrl_cmd = false);
 
+  int change_count() const { return change_count_; }
+  void OnContentChanged() { ++change_count_; }
+
   // The widget where the control will appear.
   Widget* widget_ = nullptr;
 
@@ -128,8 +115,7 @@ class EditableComboboxTest : public ViewsTestBase {
   // scenarios.
   View* parent_of_combobox_ = nullptr;
 
-  // Listener for our EditableCombobox.
-  std::unique_ptr<DummyListener> listener_;
+  int change_count_ = 0;
 
   std::unique_ptr<ui::test::EventGenerator> event_generator_;
 
@@ -168,8 +154,8 @@ void EditableComboboxTest::InitEditableCombobox(
   combobox_ =
       new EditableCombobox(std::make_unique<ui::SimpleComboboxModel>(items),
                            filter_on_edit, show_on_empty, type);
-  listener_ = std::make_unique<DummyListener>();
-  combobox_->set_listener(listener_.get());
+  combobox_->set_callback(base::BindRepeating(
+      &EditableComboboxTest::OnContentChanged, base::Unretained(this)));
   combobox_->SetID(2);
   dummy_focusable_view_ = new View();
   dummy_focusable_view_->SetFocusBehavior(View::FocusBehavior::ALWAYS);
@@ -714,30 +700,30 @@ TEST_F(EditableComboboxTest, DontShowOnEmpty) {
   ASSERT_EQ(ASCIIToUTF16("item1"), combobox_->GetItemForTest(1));
 }
 
-TEST_F(EditableComboboxTest, NoFilteringNotifiesListener) {
+TEST_F(EditableComboboxTest, NoFilteringNotifiesCallback) {
   std::vector<base::string16> items = {ASCIIToUTF16("item0"),
                                        ASCIIToUTF16("item1")};
   InitEditableCombobox(items, /*filter_on_edit=*/false, /*show_on_empty=*/true);
 
-  ASSERT_EQ(0, listener_->change_count());
+  ASSERT_EQ(0, change_count());
   combobox_->SetText(ASCIIToUTF16("a"));
-  ASSERT_EQ(1, listener_->change_count());
+  ASSERT_EQ(1, change_count());
   combobox_->SetText(ASCIIToUTF16("ab"));
-  ASSERT_EQ(2, listener_->change_count());
+  ASSERT_EQ(2, change_count());
 }
 
-TEST_F(EditableComboboxTest, FilteringNotifiesListener) {
+TEST_F(EditableComboboxTest, FilteringNotifiesCallback) {
   std::vector<base::string16> items = {ASCIIToUTF16("item0"),
                                        ASCIIToUTF16("item1")};
   InitEditableCombobox(items, /*filter_on_edit=*/true, /*show_on_empty=*/true);
 
-  ASSERT_EQ(0, listener_->change_count());
+  ASSERT_EQ(0, change_count());
   combobox_->SetText(ASCIIToUTF16("i"));
-  ASSERT_EQ(1, listener_->change_count());
+  ASSERT_EQ(1, change_count());
   combobox_->SetText(ASCIIToUTF16("ix"));
-  ASSERT_EQ(2, listener_->change_count());
+  ASSERT_EQ(2, change_count());
   combobox_->SetText(ASCIIToUTF16("ixy"));
-  ASSERT_EQ(3, listener_->change_count());
+  ASSERT_EQ(3, change_count());
 }
 
 TEST_F(EditableComboboxTest, PasswordCanBeHiddenAndRevealed) {