Convert BrowserActionsContainer context menu from Menu2 to MenuItemView.

chrome/browser/ui/views/browser_actions_container.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/browser_actions_container.h"
 
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_browser_event_router.h"
@@ skipping 24 matching lines @@
 #include "third_party/skia/include/effects/SkGradientShader.h"
 #include "ui/base/accessibility/accessible_view_state.h"
 #include "ui/base/animation/slide_animation.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/canvas.h"
 #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"
 
 // Horizontal spacing between most items in the container, as well as after the
 // last item or chevron (if visible).
 static const int kItemSpacing = ToolbarView::kStandardSpacing;
 // Horizontal spacing before the chevron (if visible).
 static const int kChevronSpacing = kItemSpacing - 2;
 
@@ skipping 17 matching lines @@
 
   // No UpdateState() here because View hierarchy not setup yet. Our parent
   // should call UpdateState() after creation.
 
   registrar_.Add(this, NotificationType::EXTENSION_BROWSER_ACTION_UPDATED,
                  Source<ExtensionAction>(browser_action_));
 }
 
 void BrowserActionButton::Destroy() {
   if (showing_context_menu_) {
-    context_menu_menu_->CancelMenu();
+    context_menu_menu_->Cancel();
     MessageLoop::current()->DeleteSoon(FROM_HERE, this);
   } else {
     delete this;
   }
 }
 
 void BrowserActionButton::ViewHierarchyChanged(
     bool is_add, View* parent, View* child) {
   if (is_add && child == this) {
     // The Browser Action API does not allow the default icon path to be
@@ skipping 118 matching lines @@
 bool BrowserActionButton::OnMousePressed(const views::MouseEvent& event) {
   if (!event.IsRightMouseButton()) {
     return IsPopup() ?
         MenuButton::OnMousePressed(event) : TextButton::OnMousePressed(event);
   }
 
   // Get the top left point of this button in screen coordinates.
   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());
 
   ShowContextMenu(point, true);
   return false;
 }
 
 void BrowserActionButton::OnMouseReleased(const views::MouseEvent& event) {
   if (IsPopup() || showing_context_menu_) {
     // TODO(erikkay) this never actually gets called (probably because of the
@@ skipping 20 matching lines @@
                                           bool is_mouse_gesture) {
   if (!extension()->ShowConfigureContextMenus())
     return;
 
   showing_context_menu_ = true;
   SetButtonPushed();
 
   // 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() {
   SetState(views::CustomButton::BS_PUSHED);
   menu_visible_ = true;
 }
 
 void BrowserActionButton::SetButtonNotPushed() {
   SetState(views::CustomButton::BS_NORMAL);
   menu_visible_ = false;
@@ skipping 818 matching lines @@
   }
 }
 
 bool BrowserActionsContainer::ShouldDisplayBrowserAction(
     const Extension* extension) {
   // Only display incognito-enabled extensions while in incognito mode.
   return
       (!profile_->IsOffTheRecord() ||
        profile_->GetExtensionService()->IsIncognitoEnabled(extension->id()));
 }