Remove Combobox::ModelChanged And Use ComboboxModelObserver Instead

ComboboxModelObserver already exists that provides this functionality,
and ComboboxModel implementors already presume the existence of
ComboboxModelObserver via ComboboxModel::[Add|Remove]Observer.

Additionally, consumers of Combobox really shouldn't manually
call Combobox::ModelChanged(). This should be handled on the model
and the model itself needs to signal when changes occur.

This change as a result has the Combobox start listening to model
changes a la ComboboxModelObserver. This is a no-op for read-only
models as they get the no-op [Add|Remove]Observer default
implementation.

Dependent components have also been updated to account for this change
in behavior.

BUG=946299

Change-Id: Ie71949685dfa8015bad6fdb973512b7efb38c7a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1610229Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659676}

chrome/browser/ui/views/page_info/permission_selector_row.cc

@@ -126,7 +126,6 @@ PermissionCombobox::PermissionCombobox(ComboboxModelAdapter* model,
   SetEnabled(enabled);
   UpdateSelectedIndex(use_default);
   set_size_to_largest_label(false);
-  ModelChanged();
 }
 
 PermissionCombobox::~PermissionCombobox() {}

chrome/browser/ui/views/payments/validating_combobox.cc

@@ -14,12 +14,10 @@ ValidatingCombobox::ValidatingCombobox(
     std::unique_ptr<ui::ComboboxModel> model,
     std::unique_ptr<ValidationDelegate> delegate)
     : Combobox(std::move(model)), delegate_(std::move(delegate)) {
-  // No need to remove observer on owned model.
-  this->model()->AddObserver(this);
   SetFocusBehavior(FocusBehavior::ALWAYS);
 }
 
-ValidatingCombobox::~ValidatingCombobox() {}
+ValidatingCombobox::~ValidatingCombobox() = default;
 
 void ValidatingCombobox::OnBlur() {
   Combobox::OnBlur();
@@ -41,9 +39,8 @@ void ValidatingCombobox::OnContentsChanged() {
   Validate();
 }
 
-void ValidatingCombobox::OnComboboxModelChanged(
-    ui::ComboboxModel* unused_model) {
-  ModelChanged();
+void ValidatingCombobox::OnComboboxModelChanged(ui::ComboboxModel* model) {
+  views::Combobox::OnComboboxModelChanged(model);
   delegate_->ComboboxModelChanged(this);
 }
 

chrome/browser/ui/views/payments/validating_combobox.h

@@ -14,8 +14,7 @@
 
 namespace payments {
 
-class ValidatingCombobox : public views::Combobox,
-                           public ui::ComboboxModelObserver {
+class ValidatingCombobox : public views::Combobox {
  public:
   ValidatingCombobox(std::unique_ptr<ui::ComboboxModel> model,
                      std::unique_ptr<ValidationDelegate> delegate);
@@ -31,7 +30,7 @@ class ValidatingCombobox : public views::Combobox,
   // Called when the combobox contents is changed. May do validation.
   void OnContentsChanged();
 
-  // ui::ComboboxModelObserver:
+  // views::Combobox:
   void OnComboboxModelChanged(ui::ComboboxModel* model) override;
 
   // Identifies whether the current content if valid or not.

ui/views/controls/combobox/combobox.cc

@@ -121,15 +121,11 @@ int GetAdjacentIndex(ui::ComboboxModel* model, int increment, int index) {
 const char Combobox::kViewClassName[] = "views/Combobox";
 
 // Adapts a ui::ComboboxModel to a ui::MenuModel.
-class Combobox::ComboboxMenuModel : public ui::MenuModel,
-                                    public ui::ComboboxModelObserver {
+class Combobox::ComboboxMenuModel : public ui::MenuModel {
  public:
   ComboboxMenuModel(Combobox* owner, ui::ComboboxModel* model)
-      : owner_(owner), model_(model) {
-    model_->AddObserver(this);
-  }
-
-  ~ComboboxMenuModel() override { model_->RemoveObserver(this); }
+      : owner_(owner), model_(model) {}
+  ~ComboboxMenuModel() override = default;
 
  private:
   bool UseCheckmarks() const {
@@ -201,11 +197,6 @@ class Combobox::ComboboxMenuModel : public ui::MenuModel,
 
   MenuModel* GetSubmenuModelAt(int index) const override { return nullptr; }
 
-  // Overridden from ComboboxModelObserver:
-  void OnComboboxModelChanged(ui::ComboboxModel* model) override {
-    owner_->ModelChanged();
-  }
-
   Combobox* owner_;           // Weak. Owns this.
   ui::ComboboxModel* model_;  // Weak.
 
@@ -232,7 +223,8 @@ Combobox::Combobox(ui::ComboboxModel* model, int text_context, int text_style)
       menu_model_(new ComboboxMenuModel(this, model)),
       arrow_button_(new TransparentButton(this)),
       size_to_largest_label_(true) {
-  ModelChanged();
+  model_->AddObserver(this);
+  OnComboboxModelChanged(model_);
 #if defined(OS_MACOSX)
   SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
 #else
@@ -257,26 +249,13 @@ Combobox::~Combobox() {
     // Combobox should have been blurred before destroy.
     DCHECK(selector_.get() != GetInputMethod()->GetTextInputClient());
   }
+  model_->RemoveObserver(this);
 }
 
 const gfx::FontList& Combobox::GetFontList() const {
   return style::GetFont(text_context_, text_style_);
 }
 
-void Combobox::ModelChanged() {
-  // If the selection is no longer valid (or the model is empty), restore the
-  // default index.
-  if (selected_index_ >= model_->GetItemCount() ||
-      model_->GetItemCount() == 0 ||
-      model_->IsItemSeparatorAt(selected_index_)) {
-    selected_index_ = model_->GetDefaultIndex();
-  }
-
-  content_size_ = GetContentSize();
-  PreferredSizeChanged();
-  SchedulePaint();
-}
-
 void Combobox::SetSelectedIndex(int index) {
   if (selected_index_ == index)
     return;
@@ -541,6 +520,22 @@ void Combobox::ButtonPressed(Button* sender, const ui::Event& event) {
   ShowDropDownMenu(source_type);
 }
 
+void Combobox::OnComboboxModelChanged(ui::ComboboxModel* model) {
+  DCHECK_EQ(model_, model);
+
+  // If the selection is no longer valid (or the model is empty), restore the
+  // default index.
+  if (selected_index_ >= model_->GetItemCount() ||
+      model_->GetItemCount() == 0 ||
+      model_->IsItemSeparatorAt(selected_index_)) {
+    selected_index_ = model_->GetDefaultIndex();
+  }
+
+  content_size_ = GetContentSize();
+  PreferredSizeChanged();
+  SchedulePaint();
+}
+
 void Combobox::UpdateBorder() {
   std::unique_ptr<FocusableBorder> border(new FocusableBorder());
   if (invalid_)

ui/views/controls/combobox/combobox.h

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
+#include "ui/base/models/combobox_model_observer.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/prefix_delegate.h"
 #include "ui/views/style/typography.h"
@@ -35,7 +36,8 @@ class PrefixSelector;
 // Combobox has two distinct parts, the drop down arrow and the text.
 class VIEWS_EXPORT Combobox : public View,
                               public PrefixDelegate,
-                              public ButtonListener {
+                              public ButtonListener,
+                              public ui::ComboboxModelObserver {
  public:
   METADATA_HEADER(Combobox);
 
@@ -59,9 +61,6 @@ class VIEWS_EXPORT Combobox : public View,
   // Sets the listener which will be called when a selection has been made.
   void set_listener(ComboboxListener* listener) { listener_ = listener; }
 
-  // Informs the combobox that its model changed.
-  void ModelChanged();
-
   // Gets/Sets the selected index.
   int GetSelectedIndex() const { return selected_index_; }
   void SetSelectedIndex(int index);
@@ -112,6 +111,9 @@ class VIEWS_EXPORT Combobox : public View,
     size_to_largest_label_ = size_to_largest_label;
   }
 
+  // Overridden from ComboboxModelObserver:
+  void OnComboboxModelChanged(ui::ComboboxModel* model) override;
+
  private:
   friend class test::ComboboxTestApi;
 

ui/views/controls/combobox/combobox_unittest.cc

@@ -14,6 +14,7 @@
 #include "ui/base/ime/input_method.h"
 #include "ui/base/ime/text_input_client.h"
 #include "ui/base/models/combobox_model.h"
+#include "ui/base/models/combobox_model_observer.h"
 #include "ui/base/models/menu_model.h"
 #include "ui/events/event.h"
 #include "ui/events/event_constants.h"
@@ -100,13 +101,30 @@ class TestComboboxModel : public ui::ComboboxModel {
     return 0;
   }
 
+  void AddObserver(ui::ComboboxModelObserver* observer) override {
+    observers_.AddObserver(observer);
+  }
+  void RemoveObserver(ui::ComboboxModelObserver* observer) override {
+    observers_.RemoveObserver(observer);
+  }
+
   void SetSeparators(const std::set<int>& separators) {
     separators_ = separators;
+    OnModelChanged();
   }
 
-  void set_item_count(int item_count) { item_count_ = item_count; }
+  void set_item_count(int item_count) {
+    item_count_ = item_count;
+    OnModelChanged();
+  }
 
  private:
+  void OnModelChanged() {
+    for (auto& observer : observers_)
+      observer.OnComboboxModelChanged(this);
+  }
+
+  base::ObserverList<ui::ComboboxModelObserver>::Unchecked observers_;
   std::set<int> separators_;
   int item_count_ = kItemCount;
 
@@ -131,10 +149,22 @@ class VectorComboboxModel : public ui::ComboboxModel {
   }
   bool IsItemSeparatorAt(int index) override { return false; }
   int GetDefaultIndex() const override { return default_index_; }
+  void AddObserver(ui::ComboboxModelObserver* observer) override {
+    observers_.AddObserver(observer);
+  }
+  void RemoveObserver(ui::ComboboxModelObserver* observer) override {
+    observers_.RemoveObserver(observer);
+  }
+
+  void ValuesChanged() {
+    for (auto& observer : observers_)
+      observer.OnComboboxModelChanged(this);
+  }
 
  private:
-  std::vector<std::string>* values_;
+  base::ObserverList<ui::ComboboxModelObserver>::Unchecked observers_;
   int default_index_ = 0;
+  std::vector<std::string>* const values_;
 
   DISALLOW_COPY_AND_ASSIGN(VectorComboboxModel);
 };
@@ -693,19 +723,19 @@ TEST_F(ComboboxTest, ContentWidth) {
 
   values.resize(1);
   values[0] = long_item;
-  combobox.ModelChanged();
+  model.ValuesChanged();
 
   const int long_item_width = test_api.content_size().width();
 
   values[0] = short_item;
-  combobox.ModelChanged();
+  model.ValuesChanged();
 
   const int short_item_width = test_api.content_size().width();
 
   values.resize(2);
   values[0] = short_item;
   values[1] = long_item;
-  combobox.ModelChanged();
+  model.ValuesChanged();
 
   // The width will fit with the longest item.
   EXPECT_EQ(long_item_width, test_api.content_size().width());
@@ -724,12 +754,10 @@ TEST_F(ComboboxTest, ModelChanged) {
   EXPECT_EQ(4, combobox_->GetSelectedRow());
 
   model_->set_item_count(5);
-  combobox_->ModelChanged();
   EXPECT_EQ(5, combobox_->GetRowCount());
   EXPECT_EQ(4, combobox_->GetSelectedRow());  // Unchanged.
 
   model_->set_item_count(4);
-  combobox_->ModelChanged();
   EXPECT_EQ(4, combobox_->GetRowCount());
   EXPECT_EQ(0, combobox_->GetSelectedRow());  // Resets.
 
@@ -741,7 +769,6 @@ TEST_F(ComboboxTest, ModelChanged) {
   std::set<int> separators;
   separators.insert(2);
   model_->SetSeparators(separators);
-  combobox_->ModelChanged();
   EXPECT_EQ(4, combobox_->GetRowCount());
   EXPECT_EQ(0, combobox_->GetSelectedRow());  // Resets.
 
@@ -751,7 +778,6 @@ TEST_F(ComboboxTest, ModelChanged) {
 
   // Test an empty model.
   model_->set_item_count(0);
-  combobox_->ModelChanged();
   EXPECT_EQ(0, combobox_->GetRowCount());
   EXPECT_EQ(0, combobox_->GetSelectedRow());  // Resets.
 }

ui/views/examples/examples_window.cc

@@ -144,12 +144,11 @@ class ExamplesWindowContents : public WidgetDelegateView,
         on_close_(std::move(on_close)) {
     auto combobox_model = std::make_unique<ComboboxModelExampleList>();
     combobox_model_ = combobox_model.get();
+    combobox_model_->SetExamples(std::move(examples));
     combobox_ = new Combobox(std::move(combobox_model));
 
     instance_ = this;
     combobox_->set_listener(this);
-    combobox_model_->SetExamples(std::move(examples));
-    combobox_->ModelChanged();
 
     SetBackground(CreateStandardPanelBackground());
     GridLayout* layout =