Use a ui::MenuModel in views::Combobox

Use a ui::MenuModel in views::Combobox

views::Combobox currently does its own menu-building rather than using a
ui::MenuModel. However, the only thing missing from MenuModel is the
ability to set a minimum menu width. (Combobox currently does this by
adding an invisible SubmenuView with a given width.)

Moving the menu building out of Combobox allows Mac to build a native
menu instead, which gives a much nicer menu experience there. In
particular, Comboboxes on Mac center the menu over the button vertically,
and have fancy scrolling/repositioning mechanisms when the menu is shown
near the edge of the screen.

This CL adds an Adapter class that adapts a ui::ComboboxModel to the
ui::MenuModel interface, allowing views:Comboxbox to be simplified
elsewhere. MenuRunnerImplCocoa will be extended to support comboboxes in
a follow-up. For the minimum width, MenuController::CalculateMenuBounds()
can just use the width of the anchor rectangle for the same effect.

Note that TranslateBubbleView is the only user of
Combobox::STYLE_ACTION. TranslateBubbleView puts a separator after the
first item which Combobox always removes. MenuModel::IsVisibleAt() isn't
checked for separators, so simply don't add the separator to keep things
simple.

BUG=522365
Committed: https://crrev.com/e989104781ae77c9bdb31d24b96830bf27a2c717
Cr-Commit-Position: refs/heads/master@{#347867}

[tapted, 2015-09-03 12:27:57]

tapted@chromium.org changed reviewers:
+ sky@chromium.org

[tapted, 2015-09-03 12:52:27]

Hi sky, please take a look. Let me know what you think.

There's a strong preference for this menu positioning style for Mac, where the
selected item appears over the button and the menu goes either side. I initially
put together a CL that positions views menus this way rather than using NSMenu
(in http://crrev.com/1297353002/). That CL wasn't scary (mostly tests), but it
only solves the initial positioning -- Mac also does some fancy
scrolling+repositioning when the menu is near the screen edge. Also, it's super
hard to get the menu to look right on Yosemite, which introduced this
blurry+transparent theme - more details and screenshots on
http://crbug.com/522365 (the MacViews style looks OK now, but ideally we just
use it for Bookmarks toolbar folders, where MacCocoa doesn't use native menus
either).

So, I'm trying this! I've split off the MenuRunnerImplCocoa change to
http://crrev.com/1321023005 (pretty small change..). The CL here is just the
conversion to use a ui::MenuModel for Combobox (this allows the Cocoa
MenuController to generate the native menu). Maybe it's even a bit neater
generating combobox menus this way - some things in combobox.h/cc get simpler,
but there's a bit of boilerplate for pure virtuals.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:422: selected_index_ =
std::min(model_->GetDefaultIndex(), model_->GetItemCount());
The old code did `index_ = std::min(0, itemcount)`, which I think is the same as
`index_ = 0`. using DefaultIndex allows ModelChanged() to be called from the
constructor and helps testing (also added coverage for this).

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

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:236: const int num_items =
GetItemCount();
move into for loop.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:422: selected_index_ =
std::min(model_->GetDefaultIndex(), model_->GetItemCount());
On 2015/09/03 12:52:26, tapted wrote:
> The old code did `index_ = std::min(0, itemcount)`, which I think is the same
as
> `index_ = 0`. using DefaultIndex allows ModelChanged() to be called from the
> constructor and helps testing (also added coverage for this).

I suspect the old code really wanted something like:
index_ = std::max(0, std::min(index_, model_->GetItemCount() - 1));
In other words, try to maintain the same selected index, unless it's beyond the
length of the new model.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:845: if (!menu_runner_) {
This makes me nervous, in particular what happens if ShowDropDownMenu is invoked
recursively? Maybe protect against that?

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.h:109: friend class test::ComboboxTestApi;
If you're going to friend this, then nuke the GetMenuModelForTest and have
ComboboxTestApi have the ability to return the menumodel.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:44: int menu_show_count_ = 0;
Is there a reason you need this as a member rather that at the places that use
it?

[tapted, 2015-09-04 00:46:55]

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:236: const int num_items =
GetItemCount();
On 2015/09/03 17:52:27, sky wrote:
> move into for loop.

Done.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:422: selected_index_ =
std::min(model_->GetDefaultIndex(), model_->GetItemCount());
On 2015/09/03 17:52:27, sky wrote:
> On 2015/09/03 12:52:26, tapted wrote:
> > The old code did `index_ = std::min(0, itemcount)`, which I think is the
same
> as
> > `index_ = 0`. using DefaultIndex allows ModelChanged() to be called from the
> > constructor and helps testing (also added coverage for this).
> 
> I suspect the old code really wanted something like:
> index_ = std::max(0, std::min(index_, model_->GetItemCount() - 1));
> In other words, try to maintain the same selected index, unless it's beyond
the
> length of the new model.

Ah, yeah using GetItemCount() would only make sense if the count was 0. So I
guess this currently supports a model that has a default index that isn't zero
before adding any items. But if it's adding items it would have to call
ModelChanged() again anyway.

So, I think this could just be 

selected_index_ = model_->GetDefaultIndex();

Unfortunately, I don't think we can DCHECK_LT since returning 0 is probably the
best you can do if you haven't added items yet. So maybe better to keep the
std::min.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:845: if (!menu_runner_) {
On 2015/09/03 17:52:27, sky wrote:
> This makes me nervous, in particular what happens if ShowDropDownMenu is
invoked
> recursively? Maybe protect against that?

Done.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.h:109: friend class test::ComboboxTestApi;
On 2015/09/03 17:52:27, sky wrote:
> If you're going to friend this, then nuke the GetMenuModelForTest and have
> ComboboxTestApi have the ability to return the menumodel.

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.

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox_unittest.cc:44: int menu_show_count_ = 0;
On 2015/09/03 17:52:27, sky wrote:
> Is there a reason you need this as a member rather that at the places that use
> it?

Nope. Moved it out (now an argument of InstallTestRunner).

[sky, 2015-09-04 14:34:17]

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:422: selected_index_ =
std::min(model_->GetDefaultIndex(), model_->GetItemCount());
On 2015/09/04 00:46:54, tapted wrote:
> On 2015/09/03 17:52:27, sky wrote:
> > On 2015/09/03 12:52:26, tapted wrote:
> > > The old code did `index_ = std::min(0, itemcount)`, which I think is the
> same
> > as
> > > `index_ = 0`. using DefaultIndex allows ModelChanged() to be called from
the
> > > constructor and helps testing (also added coverage for this).
> > 
> > I suspect the old code really wanted something like:
> > index_ = std::max(0, std::min(index_, model_->GetItemCount() - 1));
> > In other words, try to maintain the same selected index, unless it's beyond
> the
> > length of the new model.
> 
> Ah, yeah using GetItemCount() would only make sense if the count was 0. So I
> guess this currently supports a model that has a default index that isn't zero
> before adding any items. But if it's adding items it would have to call
> ModelChanged() again anyway.
> 
> So, I think this could just be 
> 
> selected_index_ = model_->GetDefaultIndex();
> 
> Unfortunately, I don't think we can DCHECK_LT since returning 0 is probably
the
> best you can do if you haven't added items yet. So maybe better to keep the
> std::min.

I think GetDefaultIndex() is used during construction, but if the content of the
model changes we try to preserve the selected index, not reset to the default.
Like I outlined above.

Your patch always tries to use the default. That doesn't seem right.

[tapted, 2015-09-07 00:37:27]

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1310903004/diff/180001/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:422: selected_index_ =
std::min(model_->GetDefaultIndex(), model_->GetItemCount());
On 2015/09/04 14:34:17, sky wrote:
> On 2015/09/04 00:46:54, tapted wrote:
> > On 2015/09/03 17:52:27, sky wrote:
> > > On 2015/09/03 12:52:26, tapted wrote:
> > > > The old code did `index_ = std::min(0, itemcount)`, which I think is the
> > same
> > > as
> > > > `index_ = 0`. using DefaultIndex allows ModelChanged() to be called from
> the
> > > > constructor and helps testing (also added coverage for this).
> > > 
> > > I suspect the old code really wanted something like:
> > > index_ = std::max(0, std::min(index_, model_->GetItemCount() - 1));
> > > In other words, try to maintain the same selected index, unless it's
beyond
> > the
> > > length of the new model.
> > 
> > Ah, yeah using GetItemCount() would only make sense if the count was 0. So I
> > guess this currently supports a model that has a default index that isn't
zero
> > before adding any items. But if it's adding items it would have to call
> > ModelChanged() again anyway.
> > 
> > So, I think this could just be 
> > 
> > selected_index_ = model_->GetDefaultIndex();
> > 
> > Unfortunately, I don't think we can DCHECK_LT since returning 0 is probably
> the
> > best you can do if you haven't added items yet. So maybe better to keep the
> > std::min.
> 
> I think GetDefaultIndex() is used during construction, but if the content of
the
> model changes we try to preserve the selected index, not reset to the default.
> Like I outlined above.
> 
> Your patch always tries to use the default. That doesn't seem right.

Sorry - misunderstood your comment. Changed this to try to maintain the current
selection.

I think it should check separators so that it's consistent with a
`DCHECK(!model()->IsItemSeparatorAt(new_index));` in OnKeyPressed(). So it
includes logic for that. It doesn't properly support a model-change that alters
"GetDefaultIndex()", but there's no current use case for that.

Also added a test.

[sky, 2015-09-08 16:23:12]

LGTM

https://codereview.chromium.org/1310903004/diff/200002/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1310903004/diff/200002/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:409: selected_index_ =
model_->GetDefaultIndex();
nit: {}

[tapted, 2015-09-09 00:30:58]

https://codereview.chromium.org/1310903004/diff/200002/ui/views/controls/comb...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1310903004/diff/200002/ui/views/controls/comb...
ui/views/controls/combobox/combobox.cc:409: selected_index_ =
model_->GetDefaultIndex();
On 2015/09/08 16:23:12, sky wrote:
> nit: {}

Done.

[tapted, 2015-09-09 00:31:05]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-09-09 00:31:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1310903004/#ps230001
(title: "+curlies")

[commit-bot: I haz the power, 2015-09-09 00:31:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1310903004/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1310903004/230001

[commit-bot: I haz the power, 2015-09-09 09:05:07]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-09-09 09:06:15]

Patchset 13 (id:??) landed as
https://crrev.com/e989104781ae77c9bdb31d24b96830bf27a2c717
Cr-Commit-Position: refs/heads/master@{#347867}