Make Combobox Default Constructible

BUG=1108460

Change-Id: I95b3e7b6832bbd56775cee959a6bc17f24ae99fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2332688
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Auto-Submit: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794639}

ui/views/controls/combobox/combobox.cc

@@ -51,6 +51,23 @@ namespace {
 // Used to indicate that no item is currently selected by the user.
 constexpr int kNoSelection = -1;
 
+// An empty model for a combo box.
+class EmptyComboboxModel final : public ui::ComboboxModel {
+ public:
+  EmptyComboboxModel() = default;
+  EmptyComboboxModel(EmptyComboboxModel&) = delete;
+  EmptyComboboxModel& operator=(const EmptyComboboxModel&) = delete;
+  ~EmptyComboboxModel() override = default;
+
+  // ui::ComboboxModel:
+  int GetItemCount() const override { return 0; }
+  base::string16 GetItemAt(int index) const override {
+    NOTREACHED();
+    return base::string16();
+  }
+  int GetDefaultIndex() const override { return -1; }
+};
+
 SkColor GetTextColorForEnableState(const Combobox& combobox, bool enabled) {
   const int style = enabled ? style::STYLE_PRIMARY : style::STYLE_DISABLED;
   return style::GetColor(combobox, style::CONTEXT_TEXTFIELD, style);
@@ -230,6 +247,9 @@ class Combobox::ComboboxMenuModel : public ui::MenuModel {
 ////////////////////////////////////////////////////////////////////////////////
 // Combobox, public:
 
+Combobox::Combobox(int text_context, int text_style)
+    : Combobox(std::make_unique<EmptyComboboxModel>()) {}
+
 Combobox::Combobox(std::unique_ptr<ui::ComboboxModel> model,
                    int text_context,
                    int text_style)
@@ -238,17 +258,10 @@ Combobox::Combobox(std::unique_ptr<ui::ComboboxModel> model,
 }
 
 Combobox::Combobox(ui::ComboboxModel* model, int text_context, int text_style)
-    : model_(model),
-      text_context_(text_context),
+    : text_context_(text_context),
       text_style_(text_style),
-      listener_(nullptr),
-      selected_index_(model_->GetDefaultIndex()),
-      invalid_(false),
-      menu_model_(new ComboboxMenuModel(this, model)),
-      arrow_button_(new TransparentButton(this)),
-      size_to_largest_label_(true) {
-  observer_.Add(model_);
-  OnComboboxModelChanged(model_);
+      arrow_button_(new TransparentButton(this)) {
+  SetModel(model);
 #if defined(OS_APPLE)
   SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
 #else
@@ -302,6 +315,28 @@ bool Combobox::SelectValue(const base::string16& value) {
   return false;
 }
 
+void Combobox::SetOwnedModel(std::unique_ptr<ui::ComboboxModel> model) {
+  // The swap keeps the outgoing model alive for SetModel().
+  owned_model_.swap(model);
+  SetModel(owned_model_.get());
+}
+
+void Combobox::SetModel(ui::ComboboxModel* model) {
+  DCHECK(model) << "After construction, the model must not be null.";
+
+  if (model_)
+    observer_.Remove(model_);
+
+  model_ = model;
+
+  if (model_) {
+    menu_model_ = std::make_unique<ComboboxMenuModel>(this, model_);
+    observer_.Add(model_);
+    selected_index_ = model_->GetDefaultIndex();
+    OnComboboxModelChanged(model_);
+  }
+}
+
 void Combobox::SetTooltipText(const base::string16& tooltip_text) {
   arrow_button_->SetTooltipText(tooltip_text);
   if (accessible_name_.empty())

ui/views/controls/combobox/combobox.h

@@ -47,6 +47,10 @@ class VIEWS_EXPORT Combobox : public View,
   static constexpr int kDefaultComboboxTextContext = style::CONTEXT_BUTTON;
   static constexpr int kDefaultComboboxTextStyle = style::STYLE_PRIMARY;
 
+  // A combobox with an empty model.
+  explicit Combobox(int text_context = kDefaultComboboxTextContext,
+                    int text_style = kDefaultComboboxTextStyle);
+
   // |model| is owned by the combobox when using this constructor.
   explicit Combobox(std::unique_ptr<ui::ComboboxModel> model,
                     int text_context = kDefaultComboboxTextContext,
@@ -70,6 +74,9 @@ class VIEWS_EXPORT Combobox : public View,
   // the found index and returns true. Otherwise simply noops and returns false.
   bool SelectValue(const base::string16& value);
 
+  void SetOwnedModel(std::unique_ptr<ui::ComboboxModel> model);
+  void SetModel(ui::ComboboxModel* model);
+
   ui::ComboboxModel* model() const { return model_; }
 
   // Set the tooltip text, and the accessible name if it is currently empty.
@@ -153,7 +160,7 @@ class VIEWS_EXPORT Combobox : public View,
   std::unique_ptr<ui::ComboboxModel> owned_model_;
 
   // Reference to our model, which may be owned or not.
-  ui::ComboboxModel* model_;
+  ui::ComboboxModel* model_ = nullptr;
 
   // Typography context for the text written in the combobox and the options
   // shown in the drop-down menu.
@@ -164,13 +171,13 @@ class VIEWS_EXPORT Combobox : public View,
   const int text_style_;
 
   // Our listener. Not owned. Notified when the selected index change.
-  ComboboxListener* listener_;
+  ComboboxListener* listener_ = nullptr;
 
   // The current selected index; -1 and means no selection.
-  int selected_index_;
+  int selected_index_ = -1;
 
   // True when the selection is visually denoted as invalid.
-  bool invalid_;
+  bool invalid_ = false;
 
   // The accessible name of this combobox.
   base::string16 accessible_name_;
@@ -204,7 +211,7 @@ class VIEWS_EXPORT Combobox : public View,
   // When true, the size of contents is defined by the selected label.
   // Otherwise, it's defined by the widest label in the menu. If this is set to
   // true, the parent view must relayout in ChildPreferredSizeChanged().
-  bool size_to_largest_label_;
+  bool size_to_largest_label_ = true;
 
   // The focus ring for this Combobox.
   FocusRing* focus_ring_ = nullptr;

ui/views/controls/combobox/combobox_unittest.cc

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "ui/accessibility/ax_action_data.h"
@@ -823,4 +824,106 @@ TEST_F(ComboboxTest, MenuModel) {
   EXPECT_TRUE(menu_model->IsVisibleAt(0));
 }
 
+namespace {
+
+using ComboboxDefaultTest = ViewsTestBase;
+
+class ConfigurableComboboxModel final : public ui::ComboboxModel {
+ public:
+  explicit ConfigurableComboboxModel(bool* destroyed = nullptr)
+      : destroyed_(destroyed) {
+    if (destroyed_)
+      *destroyed_ = false;
+  }
+  ConfigurableComboboxModel(ConfigurableComboboxModel&) = delete;
+  ConfigurableComboboxModel& operator=(const ConfigurableComboboxModel&) =
+      delete;
+  ~ConfigurableComboboxModel() override {
+    if (destroyed_)
+      *destroyed_ = true;
+  }
+
+  // ui::ComboboxModel:
+  int GetItemCount() const override { return item_count_; }
+  base::string16 GetItemAt(int index) const override {
+    DCHECK_LT(index, item_count_);
+    return base::NumberToString16(index);
+  }
+  int GetDefaultIndex() const override { return default_index_; }
+
+  void SetItemCount(int item_count) { item_count_ = item_count; }
+
+  void SetDefaultIndex(int default_index) { default_index_ = default_index; }
+
+ private:
+  bool* const destroyed_;
+  int item_count_ = 0;
+  int default_index_ = -1;
+};
+
+}  // namespace
+
+TEST_F(ComboboxDefaultTest, Default) {
+  auto combobox = std::make_unique<Combobox>();
+  EXPECT_EQ(0, combobox->GetRowCount());
+  EXPECT_EQ(-1, combobox->GetSelectedRow());
+}
+
+TEST_F(ComboboxDefaultTest, SetModel) {
+  bool destroyed = false;
+  std::unique_ptr<ConfigurableComboboxModel> model =
+      std::make_unique<ConfigurableComboboxModel>(&destroyed);
+  model->SetItemCount(42);
+  model->SetDefaultIndex(27);
+  {
+    auto combobox = std::make_unique<Combobox>();
+    combobox->SetModel(model.get());
+    EXPECT_EQ(42, combobox->GetRowCount());
+    EXPECT_EQ(27, combobox->GetSelectedRow());
+  }
+  EXPECT_FALSE(destroyed);
+}
+
+TEST_F(ComboboxDefaultTest, SetOwnedModel) {
+  bool destroyed = false;
+  std::unique_ptr<ConfigurableComboboxModel> model =
+      std::make_unique<ConfigurableComboboxModel>(&destroyed);
+  model->SetItemCount(42);
+  model->SetDefaultIndex(27);
+  {
+    auto combobox = std::make_unique<Combobox>();
+    combobox->SetOwnedModel(std::move(model));
+    EXPECT_EQ(42, combobox->GetRowCount());
+    EXPECT_EQ(27, combobox->GetSelectedRow());
+  }
+  EXPECT_TRUE(destroyed);
+}
+
+TEST_F(ComboboxDefaultTest, SetModelOverwriteOwned) {
+  bool destroyed = false;
+  std::unique_ptr<ConfigurableComboboxModel> model =
+      std::make_unique<ConfigurableComboboxModel>(&destroyed);
+  auto combobox = std::make_unique<Combobox>();
+  combobox->SetModel(model.get());
+  ASSERT_FALSE(destroyed);
+  combobox->SetOwnedModel(std::make_unique<ConfigurableComboboxModel>());
+  EXPECT_FALSE(destroyed);
+}
+
+TEST_F(ComboboxDefaultTest, SetOwnedModelOverwriteOwned) {
+  bool destroyed_first = false;
+  bool destroyed_second = false;
+  {
+    auto combobox = std::make_unique<Combobox>();
+    combobox->SetOwnedModel(
+        std::make_unique<ConfigurableComboboxModel>(&destroyed_first));
+    ASSERT_FALSE(destroyed_first);
+    combobox->SetOwnedModel(
+        std::make_unique<ConfigurableComboboxModel>(&destroyed_second));
+    EXPECT_TRUE(destroyed_first);
+    ASSERT_FALSE(destroyed_second);
+  }
+  EXPECT_TRUE(destroyed_second);
+}
+
 }  // namespace views