Replace ButtonDropDown menu implementation (Menu2) with MenuItemView.

Replace ButtonDropDown menu implementation (Menu2) with MenuItemView.

- Added views::MenuDelegate::WillHideMenu()
- Added EMPTY to views::MenuItemView::Type to suppress delegate call.
- Minor fixes for views::MenuModelAdapter

There was a Windows-only views unit test that a click-drag would post the ButtonDropDown menu.  This test relied on TrackPopupMenu not blocking for input if the menu was empty.  Because MenuItemView does block, this test could not work as a unit test.  I removed this test and added an interactive ui test to do the same thing.

BUG=chromium-os:13887
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87490

[rhashimoto, 2011-05-19 19:04:43]

Hi Scott -

This is another Menu2 replacement.  This one has an issue so it can't be
committed as-is - I'd like to ask for some guidance from you.

I'm adapting the MenuModel interface to the MenuDelegate interface for the calls
from the menu controller.  The tricky one is MenuDelegate::ExecuteCommand() with
the mouse event argument.  I can call event_utils::DispositionFromEventFlags()
to translate the event to a WindowOpenDisposition (as done in the CL), but that
function and disposition_utils::DispositionFromClick() are not part of views.

Can I just move these utilities into the views code (e.g. under views/events)
and namespace?  Or would you suggest a different approach?  

Thanks,
Roy

[rhashimoto, 2011-05-19 19:08:57]

Sorry, there was one other issue.  MenuModel::GetFirstItemIndex() takes a
NativeMenu argument, which I don't have.  I'm passing NULL, which requires that
the MenuModel not do anything with that argument.  The wrench menu does that
with its external model.  I assume that's okay?

Roy

[sky, 2011-05-19 20:42:26]

On 2011/05/19 19:04:43, rhashimoto wrote:
> Hi Scott -
> 
> This is another Menu2 replacement.  This one has an issue so it can't be
> committed as-is - I'd like to ask for some guidance from you.
> 
> I'm adapting the MenuModel interface to the MenuDelegate interface for the
calls
> from the menu controller.  The tricky one is MenuDelegate::ExecuteCommand()
with
> the mouse event argument.  I can call event_utils::DispositionFromEventFlags()
> to translate the event to a WindowOpenDisposition (as done in the CL), but
that
> function and disposition_utils::DispositionFromClick() are not part of views.

Looks like whoever added ActivatedAtWithDisposition never wired up the views
side in the first place. The best place for this sort of thing is ViewsDelegate.
It'll let you bridge the two.

> Can I just move these utilities into the views code (e.g. under views/events)
> and namespace?  Or would you suggest a different approach?  

I don't believe views depends upon webkit (where window open disposition is
defined), so that wouldn't really help you either.

You should writer a generic wrapper that we can use in other places.

Also, this change effects windows too. Meaning you're moving windows from using
a native window to the non-native window.

[sky, 2011-05-19 20:43:25]

On 2011/05/19 19:08:57, rhashimoto wrote:
> Sorry, there was one other issue.  MenuModel::GetFirstItemIndex() takes a
> NativeMenu argument, which I don't have.  I'm passing NULL, which requires
that
> the MenuModel not do anything with that argument.  The wrench menu does that
> with its external model.  I assume that's okay?
> 
> Roy

Yes, that should be ok. Can you update the docs for GetFirstItemIndex to
indicate in some cases NULL is supplied.

[rhashimoto, 2011-05-21 00:40:12]

Hi Scott -

Sorry for being dense but I'm having a little trouble grokking
your advice.

On 2011/05/19 20:42:26, sky wrote:
> > I can call event_utils::DispositionFromEventFlags() to
> > translate the event to a WindowOpenDisposition (as done in
> > the CL), but that function and
> > disposition_utils::DispositionFromClick() are not part of
> > views.
>
> Looks like whoever added ActivatedAtWithDisposition never
> wired up the views side in the first place. The best place for
> this sort of thing is ViewsDelegate.  It'll let you bridge the
> two.

Do you mean something like declare an enum WindowOpenDisposition
{...} within ViewsDelegate and a pure virtual method for
platforms to translate mouse events into
ViewsDelegate::WindowOpenDisposition?

> > Can I just move these utilities into the views
> > code (e.g. under views/events) and namespace?  Or would you
> > suggest a different approach?
> 
> I don't believe views depends upon webkit (where window open
> disposition is defined), so that wouldn't really help you
> either.
>
> You should writer a generic wrapper that we can use in other
> places.

You mean promote the MenuModelAdapter from anonymous namespace
in the button_dropdown file into the views namespace, that wrapper?

Can I ask why we have both MenuModel and MenuDelegate?

> Also, this change effects windows too. Meaning you're moving
> windows from using a native window to the non-native window.

Does this just mean that I need to test thoroughly on Windows or
is it a warning that I'm doing something wrong or need to consider
additional repercussions?

Thanks,
Roy

[sky, 2011-05-23 14:26:44]

On Fri, May 20, 2011 at 5:40 PM,  <rhashimoto@chromium.org> wrote:
> Hi Scott -
>
> Sorry for being dense but I'm having a little trouble grokking
> your advice.
>
> On 2011/05/19 20:42:26, sky wrote:
>>
>> > I can call event_utils::DispositionFromEventFlags() to
>> > translate the event to a WindowOpenDisposition (as done in
>> > the CL), but that function and
>> > disposition_utils::DispositionFromClick() are not part of
>> > views.
>
>> Looks like whoever added ActivatedAtWithDisposition never
>> wired up the views side in the first place. The best place for
>> this sort of thing is ViewsDelegate.  It'll let you bridge the
>> two.
>
> Do you mean something like declare an enum WindowOpenDisposition
> {...} within ViewsDelegate and a pure virtual method for
> platforms to translate mouse events into
> ViewsDelegate::WindowOpenDisposition?

MenuModel takes an int (probably to avoid dependency issues). So, I
was thinking you would add:

int GetDispositionForEvent(xxx);

To ViewsDelegate. The implementation (in ChromeViewsDelegate) would
call through to the event_utils function.

>> > Can I just move these utilities into the views
>> > code (e.g. under views/events) and namespace?  Or would you
>> > suggest a different approach?
>
>> I don't believe views depends upon webkit (where window open
>> disposition is defined), so that wouldn't really help you
>> either.
>
>> You should writer a generic wrapper that we can use in other
>> places.
>
> You mean promote the MenuModelAdapter from anonymous namespace
> in the button_dropdown file into the views namespace, that wrapper?

Exactly.

> Can I ask why we have both MenuModel and MenuDelegate?

MenuDelegate predates MenuModel. There is a bug on unifying the two,
but no one has gotten around to it.

>> Also, this change effects windows too. Meaning you're moving
>> windows from using a native window to the non-native window.
>
> Does this just mean that I need to test thoroughly on Windows or
> is it a warning that I'm doing something wrong or need to consider
> additional repercussions?

I'm a bit worried about switching from native to
MenuItemView/MenuDelegate classes, primarily because they haven't seen
as much usage. But that isn't a good reason not to switch.

  -Scott

[rhashimoto, 2011-05-31 22:49:23]

Hi Scott -

I'm now using the views::MenuModelAdapter committed in
http://codereview.chromium.org/7067032/ to convert ButtonDropDown.  It seems to
work okay on ChromiumOS and Windows (used for forward/back buttons, including
compact navigation).  The look is definitely different:

https://docs.google.com/leaf?id=0B6MVLdM1B58rMzY1OGQ3OTctODUyYy00N2I4LWJkOTkt...
https://docs.google.com/leaf?id=0B6MVLdM1B58rYjljNjg4NGMtZjAzYi00YWNkLTg1ZjYt...

I added an EMPTY value to the MenuItemView::Type enumeration for EmptyMenuItem
to avoid calling the delegate for IsCommandEnabled().  Empty menus are still
allowed to call the delegate to get a font.

There was a Windows-only unit test for ButtonDropDown that hung because the
native Windows call TrackPopupMenu() did not block for input with an empty menu
but the views menu does.  I deleted that test and added an interactive ui test
that does the same thing.  I added MenuDelegate::WillHideMenu() to support the
test, and I think it will also be needed for RenderViewContextMenu with Chrome
Frame to coordinate menu lifetime.

Thanks,
Roy

[sky, 2011-05-31 23:10:40]

LGTM

http://codereview.chromium.org/7044016/diff/26001/chrome/browser/ui/views/but...
File chrome/browser/ui/views/button_dropdown_test.cc (right):

http://codereview.chromium.org/7044016/diff/26001/chrome/browser/ui/views/but...
chrome/browser/ui/views/button_dropdown_test.cc:15: ButtonDropDownDragTest() :
button_(NULL),
nit: if you can't fit all the members on the initial line, then wrap the : to
the next line
(http://www.corp.google.com/eng/doc/cppguide.xml#Constructor_Initializer_Lists).

[rhashimoto, 2011-05-31 23:36:47]

http://codereview.chromium.org/7044016/diff/26001/chrome/browser/ui/views/but...
File chrome/browser/ui/views/button_dropdown_test.cc (right):

http://codereview.chromium.org/7044016/diff/26001/chrome/browser/ui/views/but...
chrome/browser/ui/views/button_dropdown_test.cc:15: ButtonDropDownDragTest() :
button_(NULL),
On 2011/05/31 23:10:40, sky wrote:
> nit: if you can't fit all the members on the initial line, then wrap the : to
> the next line
>
(http://www.corp.google.com/eng/doc/cppguide.xml#Constructor_Initializer_Lists).

Done.

[commit-bot: I haz the power, 2011-06-01 16:57:08]

Change committed as 87490