Convert BrowserActionsContainer context menu from Menu2 to MenuItemView.

chrome/browser/ui/views/browser_actions_container.cc

Index: chrome/browser/ui/views/browser_actions_container.cc
diff --git a/chrome/browser/ui/views/browser_actions_container.cc b/chrome/browser/ui/views/browser_actions_container.cc
index ebec34e192426c13b7b424c7f8ae04ec582eebb6..c2c020161f423cb7d04ec6d54a98cc387ebb45ee 100644
--- a/chrome/browser/ui/views/browser_actions_container.cc
+++ b/chrome/browser/ui/views/browser_actions_container.cc
@@ -42,7 +42,8 @@
 #include "ui/gfx/canvas_skia.h"
 #include "views/controls/button/menu_button.h"
 #include "views/controls/button/text_button.h"
-#include "views/controls/menu/menu_2.h"
+#include "views/controls/menu/menu_item_view.h"
+#include "views/controls/menu/menu_model_adapter.h"
 #include "views/drag_utils.h"
 #include "views/metrics.h"
 #include "views/window/window.h"
@@ -80,7 +81,7 @@ BrowserActionButton::BrowserActionButton(const Extension* extension,
 
 void BrowserActionButton::Destroy() {
   if (showing_context_menu_) {
-    context_menu_menu_->CancelMenu();
+    context_menu_menu_->Cancel();
     MessageLoop::current()->DeleteSoon(FROM_HERE, this);
   } else {
     delete this;
@@ -219,6 +220,10 @@ bool BrowserActionButton::OnMousePressed(const views::MouseEvent& event) {
   gfx::Point point = gfx::Point(0, 0);
   ConvertPointToScreen(this, &point);
 
+  // Align with button left edge for RTL, right edge otherwise.

[Peter Kasting, 2011/06/08 20:29:43]

This seems backwards.  We want to align to the left edge for LTR.  The old code
ought to have been correct.

[rhashimoto, 2011/06/09 00:26:23]

It looks like we indeed align to the left edge on Windows, but the right edge on
Linux (and ChromiumOS), and the mouse location for Mac.  Patch 4 makes it
TOPLEFT.

+  if (!base::i18n::IsRTL())
+    point.Offset(width(), 0);
+
   // Make the menu appear below the button.
   point.Offset(0, height());
 
@@ -259,11 +264,15 @@ void BrowserActionButton::ShowContextMenu(const gfx::Point& p,
   // Reconstructs the menu every time because the menu's contents are dynamic.
   context_menu_contents_ =
       new ExtensionContextMenuModel(extension(), panel_->browser(), panel_);
-  context_menu_menu_.reset(new views::Menu2(context_menu_contents_.get()));
-  context_menu_menu_->RunContextMenuAt(p);
+  views::MenuModelAdapter menu_model_adapter(context_menu_contents_.get());
+  context_menu_menu_.reset(new views::MenuItemView(&menu_model_adapter));
+  menu_model_adapter.BuildMenu(context_menu_menu_.get());
+  context_menu_menu_->RunMenuAt(GetWindow()->GetNativeWindow(), NULL,
+      gfx::Rect(p, gfx::Size()), views::MenuItemView::TOPRIGHT, true);

[Peter Kasting, 2011/06/08 20:29:43]

Nit: I mentioned this in the last code review and it apparently hasn't been
fixed yet -- why does this API take a rect instead of a point?

[rhashimoto, 2011/06/09 00:26:23]

I believe the API allows the specification of a Rect to represent the invoking
object.  The menu controller will then choose the bottom left or bottom right
corner to place the menu depending on whether the anchor position is TOPLEFT or
TOPRIGHT.  For context menus a Rect of zero size is usually used, but because we
have a invoking object in this case we can pass its bounds.  Patch 4 does this.

[Peter Kasting, 2011/06/09 00:39:09]

This makes callers who use just a point construct an extra rect with empty size,
while saving nothing for callers who have a rect (since it's just as easy for
them to pass rect.x() or rect.right(), and they must already know which side
they wish to align to).

Please fix this API to use a point instead.  This can be a separate change.

[rhashimoto, 2011/06/09 01:15:27]

I can add convenience API to pass a Point, but I think the Rect version is still
useful and should be retained.  The menu controller automatically swaps the
alignment and anchor position if IsRTL() is true.

[Peter Kasting, 2011/06/09 17:43:57]

This worries me.  The old APIs took a point.  That means that anywhere we now
take a rect, we're making a behavior change.  Either the old behavior was
actually wrong in RTL mode or the new behavior is.  I would like to see concrete
examples of the former before I'm OK with making this change.  We do enough
auto-mirroring that it would not surprise me at all if the old code was correct.

Another possibility is that callers should be passing something like
GetMirroredX() in cases where neither x() nor right() is correct.  This obviates
any need to ever take a rect.

Please file a bug to change this so we can continue there and stop dragging this
code review out with this issue.

[rhashimoto, 2011/06/09 18:19:00]

I filed crosbug.com/85585.

 
   SetButtonNotPushed();
   showing_context_menu_ = false;
+  context_menu_menu_.reset(NULL);

[Peter Kasting, 2011/06/08 20:29:43]

Do we really need to use a scoped_ptr if we're resetting it here?  Can we just
use a raw pointer?

[rhashimoto, 2011/06/09 00:26:23]

Done.

 }
 
 void BrowserActionButton::SetButtonPushed() {