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

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

 // Copyright 2013 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/toolbar/app_menu.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 28 matching lines @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/common/feature_switch.h"
 #include "grit/theme_resources.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #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"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/label_button.h"
@@ skipping 72 matching lines @@
 class InMenuButtonBackground : public views::Background {
  public:
   enum ButtonType {
     LEFT_BUTTON,
     CENTER_BUTTON,
     RIGHT_BUTTON,
     SINGLE_BUTTON,
   };
 
   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.
   void SetOtherButtons(const CustomButton* left_button,
                        const CustomButton* right_button) {
     if (base::i18n::IsRTL()) {
       left_button_ = right_button;
       right_button_ = left_button;
     } else {
       left_button_ = left_button;
       right_button_ = right_button;
     }
   }
 
   // Overridden from views::Background.
   void Paint(gfx::Canvas* canvas, View* view) const override {
     CustomButton* button = CustomButton::AsCustomButton(view);
     views::Button::ButtonState state =
         button ? button->state() : views::Button::STATE_NORMAL;
     int h = view->height();
 
     // 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);
   }
 
  private:
   static SkColor BorderColor(View* view, views::Button::ButtonState state) {
     ui::NativeTheme* theme = view->GetNativeTheme();
     switch (state) {
       case views::Button::STATE_HOVERED:
         return theme->GetSystemColor(
@@ skipping 24 matching lines @@
             ui::NativeTheme::kColorId_MenuBackgroundColor);
     }
   }
 
   void DrawBackground(gfx::Canvas* canvas,
                       const views::View* view,
                       const gfx::Rect& bounds,
                       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);
     }
   }
 
   ButtonType TypeAdjustedForRTL() const {
     if (!base::i18n::IsRTL())
       return type_;
 
     switch (type_) {
       case LEFT_BUTTON:   return RIGHT_BUTTON;
       case RIGHT_BUTTON:  return LEFT_BUTTON;
       default:            break;
     }
     return type_;
   }
 
   const ButtonType type_;
 
   // See description above setter for details.
   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);
 };
 
 base::string16 GetAccessibleNameForAppMenuItem(ButtonMenuItemModel* model,
                                                int item_index,
                                                int accessible_string_id) {
   base::string16 accessible_name =
       l10n_util::GetStringUTF16(accessible_string_id);
   base::string16 accelerator_text;
 
@@ skipping 879 matching lines @@
     }
 
     if (model->GetTypeAt(i) == MenuModel::TYPE_SUBMENU)
       PopulateMenu(item, model->GetSubmenuModelAt(i));
 
     switch (model->GetCommandIdAt(i)) {
       case IDC_EXTENSIONS_OVERFLOW_MENU: {
         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;
       }
 
       case IDC_EDIT_MENU: {
         ui::ButtonMenuItemModel* submodel = model->GetButtonMenuItemAt(i);
         DCHECK_EQ(IDC_CUT, submodel->GetCommandIdAt(0));
         DCHECK_EQ(IDC_COPY, submodel->GetCommandIdAt(1));
         DCHECK_EQ(IDC_PASTE, submodel->GetCommandIdAt(2));
         item->SetTitle(l10n_util::GetStringUTF16(IDS_EDIT2));
         item->AddChildView(new CutCopyPasteView(this, submodel, 0, 1, 2));
@@ skipping 111 matching lines @@
                                 0,
                                 BookmarkMenuDelegate::SHOW_PERMANENT_FOLDERS,
                                 BOOKMARK_LAUNCH_LOCATION_WRENCH_MENU);
 }
 
 int AppMenu::ModelIndexFromCommandId(int command_id) const {
   CommandIDToEntry::const_iterator ix = command_id_to_entry_.find(command_id);
   DCHECK(ix != command_id_to_entry_.end());
   return ix->second.second;
 }