Enables hot-tracking for overflow extension buttons in the app menu

chrome/browser/ui/views/toolbar/app_menu.cc

Index: chrome/browser/ui/views/toolbar/app_menu.cc
diff --git a/chrome/browser/ui/views/toolbar/app_menu.cc b/chrome/browser/ui/views/toolbar/app_menu.cc
index 916ebad4a9e117c7c565961293ed9e35dcf448b2..bdef105a33e37dcbac66cd30a3e30e5cdef0ea0d 100644
--- a/chrome/browser/ui/views/toolbar/app_menu.cc
+++ b/chrome/browser/ui/views/toolbar/app_menu.cc
@@ -46,6 +46,7 @@
 #include "third_party/skia/include/core/SkPaint.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/layout.h"
+#include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font_list.h"
@@ -138,7 +139,11 @@ class InMenuButtonBackground : public views::Background {
   };
 
   explicit InMenuButtonBackground(ButtonType type)
-      : type_(type), left_button_(NULL), right_button_(NULL) {}
+      : type_(type),
+        left_button_(NULL),
+        right_button_(NULL),
+        corner_radius_(ui::MaterialDesignController::IsModeMaterial() ? 2 : 0) {

[Peter Kasting, 2016/02/06 02:51:12]

Why is this a member at all instead of simply being coded in directly at the one
use below?

[varkha, 2016/02/16 19:59:44]

Done.

+  }
 
   // Used when the type is CENTER_BUTTON to determine if the left/right edge
   // needs to be rendered selected.
@@ -163,12 +168,13 @@ class InMenuButtonBackground : public views::Background {
     // Normal buttons get a border drawn on the right side and the rest gets
     // filled in. The left button however does not get a line to combine
     // buttons.
+    gfx::Rect bounds(view->GetLocalBounds());
     if (type_ != RIGHT_BUTTON) {
       canvas->FillRect(gfx::Rect(0, 0, 1, h),
                        BorderColor(view, views::Button::STATE_NORMAL));
+      bounds.Inset(gfx::Insets(0, 1, 0, 0));
     }
 
-    gfx::Rect bounds(view->GetLocalBounds());
     bounds.set_x(view->GetMirroredXForRect(bounds));
     DrawBackground(canvas, view, bounds, state);
   }
@@ -213,11 +219,12 @@ class InMenuButtonBackground : public views::Background {
                       views::Button::ButtonState state) const {
     if (state == views::Button::STATE_HOVERED ||
         state == views::Button::STATE_PRESSED) {
+      ui::NativeTheme::ExtraParams params = ui::NativeTheme::ExtraParams();
+      if (view->GetClassName() == views::MenuButton::kViewClassName)
+        params.menu_item.corner_radius = corner_radius_;
       view->GetNativeTheme()->Paint(canvas->sk_canvas(),
                                     ui::NativeTheme::kMenuItemBackground,
-                                    ui::NativeTheme::kHovered,
-                                    bounds,
-                                    ui::NativeTheme::ExtraParams());
+                                    ui::NativeTheme::kHovered, bounds, params);
     }
   }
 
@@ -239,6 +246,9 @@ class InMenuButtonBackground : public views::Background {
   const CustomButton* left_button_;
   const CustomButton* right_button_;
 
+  // Show rounded button background for extension buttons with Material Design.
+  int corner_radius_;
+
   DISALLOW_COPY_AND_ASSIGN(InMenuButtonBackground);
 };
 
@@ -1138,10 +1148,22 @@ void AppMenu::PopulateMenu(MenuItemView* parent, MenuModel* model) {
         scoped_ptr<ExtensionToolbarMenuView> extension_toolbar(
             new ExtensionToolbarMenuView(browser_, this));
         extension_toolbar_ = extension_toolbar.get();
-        if (extension_toolbar->ShouldShow())
-          item->AddChildView(extension_toolbar.release());
-        else
+        if (!extension_toolbar->ShouldShow()) {
           item->SetVisible(false);
+          break;

[Peter Kasting, 2016/02/06 02:51:12]

It really worries me that |extension_toolbar_| is left pointing to a dangling
object here.

I think this member should only be set in the case where we're going to add the
child view, and it should be reset anywhere the code deletes that child view.

Another way would be to make the member a scoped_ptr and mark this as
client-owned, but that would keep the object alive longer even when not
necessary, which may not be desirable.

[varkha, 2016/02/16 19:59:44]

Done.

+        }
+        if (ui::MaterialDesignController::IsModeMaterial()) {
+          for (int i = 0; i < extension_toolbar->contents()->child_count();
+               ++i) {
+            View* action_view = extension_toolbar->contents()->child_at(i);
+            InMenuButtonBackground* in_menu_background =
+                new InMenuButtonBackground(
+                    InMenuButtonBackground::RIGHT_BUTTON);

[Peter Kasting, 2016/02/06 02:51:12]

Nit: I would inline this into the next statement mostly to avoid creating any
objects of raw pointer type.  (I have a dream that someday Chrome will never
store the result of an allocation directly into a raw pointer type.)

[varkha, 2016/02/16 19:59:44]

Done.

+            action_view->set_background(in_menu_background);
+            DCHECK(action_view->IsFocusable());
+          }
+        }
+        item->AddChildView(extension_toolbar.release());
         break;
       }