Use a ui::MenuModel in views::Combobox

ui/views/controls/combobox/combobox.h

Index: ui/views/controls/combobox/combobox.h
diff --git a/ui/views/controls/combobox/combobox.h b/ui/views/controls/combobox/combobox.h
index 55087645ba72ffa1e7671e8dd17721c4263e2c55..991ab103fd080af0442fb92266f13c09219fc387 100644
--- a/ui/views/controls/combobox/combobox.h
+++ b/ui/views/controls/combobox/combobox.h
@@ -5,34 +5,29 @@
 #ifndef UI_VIEWS_CONTROLS_COMBOBOX_COMBOBOX_H_
 #define UI_VIEWS_CONTROLS_COMBOBOX_COMBOBOX_H_
 
-#include <string>
-
 #include "base/memory/weak_ptr.h"
+#include "base/strings/string16.h"
 #include "base/time/time.h"
-#include "ui/base/models/combobox_model_observer.h"
-#include "ui/gfx/animation/animation_delegate.h"
-#include "ui/gfx/native_widget_types.h"
 #include "ui/views/controls/button/button.h"
-#include "ui/views/controls/menu/menu_delegate.h"
 #include "ui/views/controls/prefix_delegate.h"
 
 namespace gfx {
 class FontList;
-class SlideAnimation;
 }
 
 namespace ui {
 class ComboboxModel;
+class MenuModel;
 }
 
 namespace views {
+namespace test {
+class ComboboxTestApi;
+}
 
 class ComboboxListener;
-class ComboboxMenuRunner;
 class CustomButton;
-class FocusableBorder;
 class MenuRunner;
-class MenuRunnerHandler;
 class Painter;
 class PrefixSelector;
 
@@ -45,10 +40,7 @@ class PrefixSelector;
 // * STYLE_ACTION: clicking on the text notifies the listener. The menu can be
 // shown only by clicking on the arrow. The selected index is always reverted to
 // 0 after the listener is notified.
-class VIEWS_EXPORT Combobox : public MenuDelegate,
-                              public PrefixDelegate,
-                              public ui::ComboboxModelObserver,
-                              public ButtonListener {
+class VIEWS_EXPORT Combobox : public PrefixDelegate, public ButtonListener {
  public:
   // The style of the combobox.
   enum Style {
@@ -104,32 +96,19 @@ class VIEWS_EXPORT Combobox : public MenuDelegate,
   void GetAccessibleState(ui::AXViewState* state) override;
   void Layout() override;
 
-  // Overridden from MenuDelegate:
-  bool IsItemChecked(int id) const override;
-  bool IsCommandEnabled(int id) const override;
-  void ExecuteCommand(int id) override;
-  bool GetAccelerator(int id, ui::Accelerator* accelerator) const override;
-
   // Overridden from PrefixDelegate:
   int GetRowCount() override;
   int GetSelectedRow() override;
   void SetSelectedRow(int row) override;
   base::string16 GetTextForRow(int row) override;
 
-  // Overriden from ComboboxModelObserver:
-  void OnComboboxModelChanged(ui::ComboboxModel* model) override;
-
   // Overriden from ButtonListener:
   void ButtonPressed(Button* sender, const ui::Event& event) override;
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(ComboboxTest, Click);
-  FRIEND_TEST_ALL_PREFIXES(ComboboxTest, ClickButDisabled);
-  FRIEND_TEST_ALL_PREFIXES(ComboboxTest, NotifyOnClickWithMouse);
-  FRIEND_TEST_ALL_PREFIXES(ComboboxTest, ContentWidth);
+  friend class test::ComboboxTestApi;

[sky, 2015/09/03 17:52:27]

If you're going to friend this, then nuke the GetMenuModelForTest and have
ComboboxTestApi have the ability to return the menumodel.

[tapted, 2015/09/04 00:46:54]

Yeah - I didn't really like GetMenuModelForTest() :). The test API doesn't know
how to cast from a ComboboxMenuModelAdapter to a ui::MenuModel and the adapter
leans on its nested-class-is-implicit-friend a lot so I didn't want to expose it
the adapter declaration.

But! I can fix this by moving GetContentSize into Combobox - maybe it even fits
nicer there - and changing the static type of |menu_model_adapter_|

Done.

 
-  // Updates the combobox's content from its model.
-  void UpdateFromModel();
+  class ComboboxMenuModelAdapter;
 
   // Updates the border according to the current state.
   void UpdateBorder();
@@ -148,11 +127,6 @@ class VIEWS_EXPORT Combobox : public MenuDelegate,
 
   // Called when the selection is changed by the user.
   void OnPerformAction();
-  void NotifyPerformAction();
-  void AfterPerformAction();
-
-  // Converts a menu command ID to a menu item index.
-  int MenuCommandToIndex(int menu_command_id) const;
 
   int GetDisclosureArrowLeftPadding() const;
   int GetDisclosureArrowRightPadding() const;
@@ -165,6 +139,8 @@ class VIEWS_EXPORT Combobox : public MenuDelegate,
 
   PrefixSelector* GetPrefixSelector();
 
+  ui::MenuModel* GetMenuModelForTest();
+
   // Our model. Not owned.
   ui::ComboboxModel* model_;
 
@@ -186,14 +162,8 @@ class VIEWS_EXPORT Combobox : public MenuDelegate,
   // A helper used to select entries by keyboard input.
   scoped_ptr<PrefixSelector> selector_;
 
-  // Responsible for showing the context menu.
-  scoped_ptr<MenuRunner> dropdown_list_menu_runner_;
-
-  // Weak. Owned by dropdown_list_menu_runner_.
-  MenuItemView* menu_;
-
-  // Is the drop down list showing
-  bool dropdown_open_;
+  // Adapts a ComboboxModel for use by a views MenuRunner.
+  scoped_ptr<ComboboxMenuModelAdapter> menu_model_adapter_;
 
   // Like MenuButton, we use a time object in order to keep track of when the
   // combobox was closed. The time is used for simulating menu behavior; that
@@ -203,8 +173,8 @@ class VIEWS_EXPORT Combobox : public MenuDelegate,
   // the button is not part of the displayed menu.
   base::Time closed_time_;
 
-  // The maximum dimensions of the content in the dropdown
-  mutable gfx::Size content_size_;
+  // The maximum dimensions of the content in the dropdown.
+  gfx::Size content_size_;
 
   // The painters or images that are used when |style_| is STYLE_BUTTONS. The
   // first index means the state of unfocused or focused.
@@ -222,6 +192,10 @@ class VIEWS_EXPORT Combobox : public MenuDelegate,
   CustomButton* text_button_;
   CustomButton* arrow_button_;
 
+  // Set while the dropdown is showing. Ensures the menu is closed if |this| is
+  // destroyed.
+  scoped_ptr<views::MenuRunner> menu_runner_;
+
   // Used for making calbacks.
   base::WeakPtrFactory<Combobox> weak_ptr_factory_;