Action box menu

chrome/browser/ui/views/action_box_menu.cc

Index: chrome/browser/ui/views/action_box_menu.cc
diff --git a/chrome/browser/ui/views/action_box_menu.cc b/chrome/browser/ui/views/action_box_menu.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0f57ab2115128cd25603fb8ca833eba3311837ad
--- /dev/null
+++ b/chrome/browser/ui/views/action_box_menu.cc
@@ -0,0 +1,162 @@
+// Copyright (c) 2012 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/action_box_menu.h"
+
+#include "chrome/browser/ui/toolbar/action_box_menu_model.h"
+#include "chrome/browser/ui/views/browser_action_view.h"
+#include "grit/generated_resources.h"
+#include "grit/theme_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/views/bubble/bubble_border.h"
+#include "ui/views/controls/button/menu_button.h"
+#include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/view.h"
+
+#if defined(OS_WIN)
+#include <vssym32.h>

[msw, 2012/07/24 23:41:55]

nit: comment as to why this is needed.

[yefimt, 2012/07/25 21:09:21]

Done.

+#include "ui/base/native_theme/native_theme_win.h"
+#endif
+
+////////////////////////////////////////////////////////////////////////////////
+// ActionBoxMenu
+
+ActionBoxMenu::ActionBoxMenu(Browser* browser,
+                             ActionBoxMenuModel* model,
+                             bool bookmark_item_state)
+    : browser_(browser),
+      root_(NULL),
+      model_(model),
+      bookmark_item_state_(bookmark_item_state) {
+}
+
+ActionBoxMenu::~ActionBoxMenu() {
+}
+
+void ActionBoxMenu::Init() {
+  DCHECK(!root_);
+  root_ = new views::MenuItemView(this);
+  // We have icons, set this so we get the taller menu style.

[msw, 2012/07/24 23:41:55]

nit: I prefer making statements about the objects without using "we", use
impersonal verb conjugation if needed:
http://en.wikipedia.org/wiki/Impersonal_verb
(this general preference nit applies here, below, and elsewhere)
Perhaps this particular comment isn't needed at all.

[yefimt, 2012/07/25 21:09:21]

Removed.
As I'm new to chrome, I often use existing code as a template. In this case see
wrench_menu.cc:762  :)

[msw, 2012/07/25 23:02:03]

Very true, I just prefer explicitly specifying the sentence subject/object for
clarity. It's only a preference nit, but I think it's worthwhile habit.

+  root_->set_has_icons(true);
+  PopulateMenu();
+  menu_runner_.reset(new views::MenuRunner(root_));
+}
+
+void ActionBoxMenu::RunMenu(views::MenuButton* menu_button) {
+  gfx::Point screen_location;
+  views::View::ConvertPointToScreen(menu_button, &screen_location);
+  // Ignore the result since we don't need to handle a deleted menu specially.

[msw, 2012/07/24 23:41:55]

nit: for clarity, perhaps mention that this menu isn't deletable, as that only
applies to bookmark menus?

[yefimt, 2012/07/25 21:09:21]

Done.

+  ignore_result(
+      menu_runner_->RunMenuAt(menu_button->GetWidget(),
+                              menu_button,
+                              gfx::Rect(screen_location, menu_button->size()),
+                              views::MenuItemView::TOPRIGHT,
+                              views::MenuRunner::HAS_MNEMONICS));
+}
+
+void ActionBoxMenu::ExecuteCommand(int id) {
+};
+
+views::Border* ActionBoxMenu::CreateMenuBorder() {
+#if defined(OS_WIN)

[msw, 2012/07/24 23:41:55]

nit: declare and init an SkColor with SK_ColorBLACK above the preprocessor
block; move the return statement with |border_color| below, and then just
override the SK_ColorBLACK for OS_WIN.

[yefimt, 2012/07/25 21:09:21]

Done.

+  // TODO: Move to Windows only files if possible.

[msw, 2012/07/24 23:41:55]

nit: use TODO(ldap): style here and elsewhere...

[yefimt, 2012/07/25 21:09:21]

Done.

+  SkColor border_color =
+      ui::NativeThemeWin::instance()->GetThemeColorWithDefault(
+          ui::NativeThemeWin::MENU, MENU_POPUPITEM, MPI_NORMAL, TMT_TEXTCOLOR,
+          COLOR_MENUTEXT);
+  return views::Border::CreateSolidBorder(1, border_color);
+#else
+  // TODO: Use correct theme color on non-Windows.
+  return views::Border::CreateSolidBorder(1, SkColorSetRGB(0, 0, 0));
+#endif
+}
+
+views::Background* ActionBoxMenu::CreateMenuBackground() {
+#if defined(OS_WIN)

[msw, 2012/07/24 23:41:55]

nit: employ the pattern I suggested above here as well.

[yefimt, 2012/07/25 21:09:21]

Done.

+  // TODO: Move to Windows only files if possible.
+  SkColor background_color =
+      ui::NativeThemeWin::instance()->GetThemeColorWithDefault(
+          ui::NativeThemeWin::TEXTFIELD, EP_BACKGROUND, EBS_NORMAL,
+          TMT_BACKGROUND, COLOR_WINDOW);
+  return views::Background::CreateSolidBackground(background_color);
+#else
+  // TODO: Use correct theme color on non-Windows.
+  return views::Background::CreateSolidBackground(SkColorSetRGB(255, 255, 255));
+#endif
+}
+
+int ActionBoxMenu::GetCurrentTabId() const {
+  return 0;
+}
+
+void ActionBoxMenu::OnBrowserActionExecuted(BrowserActionButton* button) {
+}
+
+void ActionBoxMenu::OnBrowserActionVisibilityChanged() {
+}
+
+gfx::Size ActionBoxMenu::GetViewContentOffset() const {
+  return gfx::Size(0, 0);
+}
+
+void ActionBoxMenu::WriteDragDataForView(views::View* sender,
+                                         const gfx::Point& press_pt,
+                                         ui::OSExchangeData* data) {
+}
+
+int ActionBoxMenu::GetDragOperationsForView(views::View* sender,
+                                            const gfx::Point& p) {
+  return 0;
+}
+
+bool ActionBoxMenu::CanStartDragForView(views::View* sender,
+                                        const gfx::Point& press_pt,
+                                        const gfx::Point& p) {
+  return false;
+}
+
+void ActionBoxMenu::Observe(int type,
+                            const content::NotificationSource& source,
+                            const content::NotificationDetails& details) {
+}
+
+void ActionBoxMenu::PopulateMenu() {
+  int item_id = 1;
+  AddBookmarkMenuItem(root_, &item_id);
+  if (model_->GetItemCount() > 0)
+    root_->AppendSeparator();
+
+  for (int model_index = 0; model_index < model_->GetItemCount();
+       ++model_index) {
+    views::MenuItemView* menu_item = root_->AppendMenuItemFromModel(
+        model_, model_index, item_id + model_index);
+    if (model_->GetTypeAt(model_index) == ui::MenuModel::TYPE_COMMAND) {

[msw, 2012/07/24 23:41:55]

nit: Can you clarify generally what menu items are and aren't commands, and
why/how they're styled and handled differently?

[yefimt, 2012/07/25 21:09:21]

I did it looking at how other menus are implemented. Currently here they all
command type. I guess if non command menu items will be added later they wont
break this code.

[msw, 2012/07/25 23:02:03]

Then I'd make this a DCHECK instead of a conditional.

[yefimt, 2012/07/31 00:10:11]

Done.

+      menu_item->SetMargins(0, 0);
+      const extensions::Extension* extension =
+          model_->GetActionBoxExtensionByIndex(model_index);
+      BrowserActionView* view = new BrowserActionView(extension,
+          browser_, this);
+      view->button()->SetShowMultipleIconStates(false);
+      view->button()->SetTooltipDisabled(true);
+      browser_action_views_.push_back(view);
+      menu_item->SetIconView(view);
+    }
+  }
+}
+
+views::MenuItemView* ActionBoxMenu::AddBookmarkMenuItem(

[msw, 2012/07/24 23:41:55]

nit: the return value is unused in the sole caller, consider removing it.

[yefimt, 2012/07/25 21:09:21]

Would it be better to have it consistent with all
MenuItemView::Add/AppendMenuItem*() functions?

[msw, 2012/07/25 23:02:03]

Sure, I suppose that's fine.

+    views::MenuItemView* parent,
+    int* item_id) {
+  string16 label = l10n_util::GetStringUTF16(bookmark_item_state_ ?
+      IDS_TOOLTIP_STARRED : IDS_TOOLTIP_STAR);
+  gfx::ImageSkia* icon =
+      ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+          bookmark_item_state_ ? IDR_STAR_LIT : IDR_STAR);
+  DCHECK(icon);

[msw, 2012/07/24 23:41:55]

nit: no need to DCHECK immediately before a de-reference; we'll see the crash
plainly on a NULL de-ref.

[yefimt, 2012/07/25 21:09:21]

Done.

+  views::MenuItemView* item =
+      parent->AppendMenuItemWithIcon(*item_id, label, *icon);
+  (*item_id)++;
+  return item;
+}