Add MenuModelAdapter to wrap ui::MenuModel with views::MenuDelegate interface.

Add MenuModelAdapter to wrap ui::MenuModel with views::MenuDelegate interface.

- added ViewsDelegate::GetDispositionForEvent() pure virtual
- added MenuModelAdapter
- added unit test for MenuModelAdapter
- added comment for MenuModel::GetFirstItemIndex() that callers may pass NULL for NativeMenu argument
- fixed index offset bug in MenuModel::GetModelAndIndexForCommandId()
- fixed spurious call to MenuDelegate::IsCommandEnabled()

BUG=none
TEST=included


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

[rhashimoto, 2011-05-24 23:21:44]

Hi Scott -

I've attempted to follow your recommendations from
http://codereview.chromium.org/7044016/ to create a wrapper for ui::MenuModel to
views::MenuDelegate.

One line I'd like to ask you to look closely at is the change to
menu_item_view.cc.  There is always an initial call to
MenuDelegate::IsCommandEnabled() for container MenuItemView instances with id=0.
 I think this is a spurious call and disabled it for the root item because it
would weaken run-time checking in MenuModelAdapter (because it has to allow for
id=0).  I'd like to verify I'm not missing a use for that.

Thanks,
Roy

[sky, 2011-05-25 15:41:12]

LGTM

http://codereview.chromium.org/7067032/diff/5002/chrome/browser/ui/views/chro...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

http://codereview.chromium.org/7067032/diff/5002/chrome/browser/ui/views/chro...
chrome/browser/ui/views/chrome_views_delegate.h:51: virtual int
GetDispositionForEvent(int event_flags) OVERRIDE;
It looks weird to have one OVERRIDE, but not all. Please add OVERRIDE to all the
ViewsDelegate methods.

http://codereview.chromium.org/7067032/diff/5002/ui/base/models/menu_model.cc
File ui/base/models/menu_model.cc (right):

http://codereview.chromium.org/7067032/diff/5002/ui/base/models/menu_model.cc...
ui/base/models/menu_model.cc:21: const int candidate_index = i +
(*model)->GetFirstItemIndex(NULL);
Move this call outside the loop.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:566: if (parent && root_delegate)
Yes, this is a good change. I just did one similar in
http://codereview.chromium.org/7066034/ to avoid calling through for (empty)
menu items. Just add a comment though.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
File views/controls/menu/menu_model_adapter.h (right):

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter.h:10: #include
"views/controls/menu/menu_delegate.h"
newline between 9 and 10

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
File views/controls/menu/menu_model_adapter_unittest.cc (right):

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:35: virtual int
GetFirstItemIndex(gfx::NativeMenu native_menu) const {
OVERRIDE

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:126: ItemType type_;
don't use underscores for structs. That is, this should be 'type'.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:154:
items_.push_back(Item(TYPE_COMMAND, L"submenu item 0", NULL));
Don't use wide, instead use ascii.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:184: class
MenuModelAdapterTest : public ViewsTestBase {
use a typedef.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:192:
scoped_ptr<MenuModelBase> model(new RootModel);
You never reset these, so you may as well declare them on the stack.

http://codereview.chromium.org/7067032/diff/5002/views/views_delegate.h
File views/views_delegate.h (right):

http://codereview.chromium.org/7067032/diff/5002/views/views_delegate.h#newco...
views/views_delegate.h:83: // Convert event to window disposition.
How about: Converts views::Event::flags to a WindowOpenDisposition.

[rhashimoto, 2011-05-25 16:24:36]

Thanks, Scott!

http://codereview.chromium.org/7067032/diff/5002/chrome/browser/ui/views/chro...
File chrome/browser/ui/views/chrome_views_delegate.h (right):

http://codereview.chromium.org/7067032/diff/5002/chrome/browser/ui/views/chro...
chrome/browser/ui/views/chrome_views_delegate.h:51: virtual int
GetDispositionForEvent(int event_flags) OVERRIDE;
On 2011/05/25 15:41:12, sky wrote:
> It looks weird to have one OVERRIDE, but not all. Please add OVERRIDE to all
the
> ViewsDelegate methods.

Done.

http://codereview.chromium.org/7067032/diff/5002/ui/base/models/menu_model.cc
File ui/base/models/menu_model.cc (right):

http://codereview.chromium.org/7067032/diff/5002/ui/base/models/menu_model.cc...
ui/base/models/menu_model.cc:21: const int candidate_index = i +
(*model)->GetFirstItemIndex(NULL);
On 2011/05/25 15:41:12, sky wrote:
> Move this call outside the loop.

Done.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.cc:566: if (parent && root_delegate)
On 2011/05/25 15:41:12, sky wrote:
> Yes, this is a good change. I just did one similar in
> http://codereview.chromium.org/7066034/ to avoid calling through for (empty)
> menu items. Just add a comment though.

Done.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
File views/controls/menu/menu_model_adapter.h (right):

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter.h:10: #include
"views/controls/menu/menu_delegate.h"
On 2011/05/25 15:41:12, sky wrote:
> newline between 9 and 10

Done.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
File views/controls/menu/menu_model_adapter_unittest.cc (right):

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:35: virtual int
GetFirstItemIndex(gfx::NativeMenu native_menu) const {
On 2011/05/25 15:41:12, sky wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:126: ItemType type_;
On 2011/05/25 15:41:12, sky wrote:
> don't use underscores for structs. That is, this should be 'type'.

Done.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:154:
items_.push_back(Item(TYPE_COMMAND, L"submenu item 0", NULL));
On 2011/05/25 15:41:12, sky wrote:
> Don't use wide, instead use ascii.

Done.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:184: class
MenuModelAdapterTest : public ViewsTestBase {
On 2011/05/25 15:41:12, sky wrote:
> use a typedef.

Done.

http://codereview.chromium.org/7067032/diff/5002/views/controls/menu/menu_mod...
views/controls/menu/menu_model_adapter_unittest.cc:192:
scoped_ptr<MenuModelBase> model(new RootModel);
On 2011/05/25 15:41:12, sky wrote:
> You never reset these, so you may as well declare them on the stack.

Done.

http://codereview.chromium.org/7067032/diff/5002/views/views_delegate.h
File views/views_delegate.h (right):

http://codereview.chromium.org/7067032/diff/5002/views/views_delegate.h#newco...
views/views_delegate.h:83: // Convert event to window disposition.
On 2011/05/25 15:41:12, sky wrote:
> How about: Converts views::Event::flags to a WindowOpenDisposition.

Done.

[commit-bot: I haz the power, 2011-05-25 19:41:37]

Change committed as 86677