Convert BrowserActionOverflowMenu context menu from Menu2 to MenuItemView.

Convert BrowserActionOverflowMenu context menu from Menu2 to MenuItemView.

This is part of the GTK dependency removal.

BUG=chromium-os:13887
TEST=none


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

[rhashimoto, 2011-06-08 17:38:26]

finnur: I think you authored the original code.
pkasting: as OWNER of chrome/browser/ui.

Thanks,
Roy

[Peter Kasting, 2011-06-08 20:32:11]

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:101:
scoped_refptr<ExtensionContextMenuModel> context_menu_contents =
Why a scoped_refptr here?  Can't we just declare this on the stack?

[rhashimoto, 2011-06-09 01:18:40]

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:101:
scoped_refptr<ExtensionContextMenuModel> context_menu_contents =
On 2011/06/08 20:32:11, Peter Kasting wrote:
> Why a scoped_refptr here?  Can't we just declare this on the stack?

Allocating a ref counted object on the stack throws a debug assertion on
destruction:

[15480:15480:1037887493904:FATAL:ref_counted.cc(24)] Check failed: in_dtor_.
RefCounted object deleted without calling Release()

[Finnur, 2011-06-09 11:37:51]

Can you make sure this bug does not regress as a result of this?
http://crbug.com/40066
Note: The browser action button must be in the overflow area.

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:110:
NULL, gfx::Rect(p ,gfx::Size()), views::MenuItemView::TOPLEFT, true);
nit, space after comma, not before it.

[rhashimoto, 2011-06-09 16:03:06]

I tried uninstalling extensions from the overflow menu (click on overflow menu,
right-click on extension, choose Uninstall).  It worked fine on windows and
linux_chromeos.

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/7077033/diff/1001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:110:
NULL, gfx::Rect(p ,gfx::Size()), views::MenuItemView::TOPLEFT, true);
On 2011/06/09 11:37:51, Finnur wrote:
> nit, space after comma, not before it.

Done.

[rhashimoto, 2011-06-09 16:04:02]

On 2011/06/09 16:03:06, rhashimoto wrote:
> I tried uninstalling extensions from the overflow menu (click on overflow
menu,
> right-click on extension, choose Uninstall).  It worked fine on windows and
> linux_chromeos.

"Worked fine" meaning that the extension was actually uninstalled.

[Finnur, 2011-06-09 16:05:51]

In that case, LGTM

[Peter Kasting, 2011-06-09 17:44:51]

LGTM

http://codereview.chromium.org/7077033/diff/7001/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/7077033/diff/7001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:105:
views::MenuItemView context_menu_menu(&context_menu_model_adapter);
Nit: Can this just be |context_menu|?  Or |context_menu_view| if not?

[rhashimoto, 2011-06-09 19:00:01]

http://codereview.chromium.org/7077033/diff/7001/chrome/browser/ui/views/exte...
File
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc
(right):

http://codereview.chromium.org/7077033/diff/7001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.cc:105:
views::MenuItemView context_menu_menu(&context_menu_model_adapter);
On 2011/06/09 17:44:51, Peter Kasting wrote:
> Nit: Can this just be |context_menu|?  Or |context_menu_view| if not?

Done.

[commit-bot: I haz the power, 2011-06-09 21:30:50]

Change committed as 88594