Action box menu

chrome/browser/ui/views/action_box_menu.cc

+// 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 "grit/theme_resources_standard.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"
+
+using ui::MenuModel;
+using views::MenuItemView;

[tfarina, 2012/07/02 22:49:18]

nit: please, avoid using declarations if possible. Up to you though!

[yefimt, 2012/07/11 22:34:34]

Done.

+using views::View;
+
+// ActionBoxMenu ---------------------------------------------------------------

[Aaron Boodman, 2012/07/02 22:41:34]

Remove this line.

[yefimt, 2012/07/11 22:34:34]

Done.

+
+ActionBoxMenu::ActionBoxMenu(Browser* browser,
+                             ActionBoxMenuModel* model,
+                             bool bookmark_item_state)
+    : browser_(browser),
+      root_(NULL),
+      model_(model),
+      bookmark_item_state_(bookmark_item_state),
+      model_index_offset_(0) {
+}
+
+ActionBoxMenu::~ActionBoxMenu() {
+  STLDeleteElements(&browser_action_views_);

[Aaron Boodman, 2012/07/02 22:41:34]

Use std::vector<linked_ptr<BrowserActionView> > to avoid manual delete here.

[tfarina, 2012/07/02 22:49:18]

Ham? What is that? :/

Do you mean using ScopedVector instead?

[Aaron Boodman, 2012/07/03 00:03:32]

What is what? base/memory/linked_ptr.h

[yefimt, 2012/07/11 22:34:34]

Done.

+}
+
+void ActionBoxMenu::Init() {
+  DCHECK(!root_);

[Aaron Boodman, 2012/07/02 22:41:34]

CHECK(!root_)

[yefimt, 2012/07/11 22:34:34]

Done.

+  root_ = new MenuItemView(this);
+  root_->set_has_icons(true);  // We have checks, radios and icons, set this

[Aaron Boodman, 2012/07/02 22:41:34]

Put this comment on the line above. Multiline trailing comments are hard to
maintain.

[Aaron Boodman, 2012/07/02 22:41:34]

We actually only have icons right, not checks or radios?

[yefimt, 2012/07/11 22:34:34]

Done.

+                               // so we get the taller menu style.
+  PopulateMenu();
+  menu_runner_.reset(new views::MenuRunner(root_));
+}
+
+void ActionBoxMenu::RunMenu(views::MenuButton* host) {
+  gfx::Point screen_loc;

[Aaron Boodman, 2012/07/02 22:41:34]

Avoid abbreviation: screen_location

[yefimt, 2012/07/11 22:34:34]

Done.

+  views::View::ConvertPointToScreen(host, &screen_loc);
+  gfx::Rect bounds(screen_loc, host->size());
+  if (menu_runner_->RunMenuAt(host->GetWidget(), host, bounds,
+          MenuItemView::TOPRIGHT, views::MenuRunner::HAS_MNEMONICS) ==

[Aaron Boodman, 2012/07/02 22:41:34]

Left column of all params lines should line up.

[yefimt, 2012/07/11 22:34:34]

Done.

+          views::MenuRunner::MENU_DELETED) {
+    return;
+  }
+}
+
+void ActionBoxMenu::ExecuteCommand(int id) {
+};
+
+views::Border* ActionBoxMenu::CreateMenuBorder() {
+  return views::Border::CreateSolidBorder(1, SkColorSetRGB(0, 0, 0));
+//  views::BubbleBorder* border = new views::BubbleBorder(

[Aaron Boodman, 2012/07/02 22:41:34]

Kill dead code.

[yefimt, 2012/07/11 22:34:34]

Done.

+//    views::BubbleBorder::NONE, views::BubbleBorder::NO_SHADOW);
+//  return border;
+}
+
+views::Background* ActionBoxMenu::CreateMenuBackground() {
+  return views::Background::CreateSolidBackground(SkColorSetRGB(255, 255, 255));
+}
+
+Browser* ActionBoxMenu::GetBrowser() const {
+  return browser_;
+}
+
+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(View* sender,
+                                         const gfx::Point& press_pt,
+                                         ui::OSExchangeData* data) {
+}
+
+int ActionBoxMenu::GetDragOperationsForView(View* sender,
+                                            const gfx::Point& p) {
+  return 0;
+}
+
+bool ActionBoxMenu::CanStartDragForView(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 id = 1;

[Aaron Boodman, 2012/07/02 22:41:34]

Consider changing this counter into a member variable like next_id_ so that you
don't have to pass it around.

[yefimt, 2012/07/11 22:34:34]

Would be weird to have a member variable that keeps state after function call,
when this function is called only one time and variable is used inside that
function only.

On 2012/07/02 22:41:34, Aaron Boodman wrote:

+  int menu_index = 0;
+  AddBookmarkMenuItem(root_, &menu_index, &id);
+  root_->AppendSeparator();
+  menu_index++;
+  model_index_offset_ = menu_index;
+
+  for (int model_index = 0; model_index < model_->GetItemCount();
+       ++model_index) {
+    MenuItemView* menu_item = root_->AppendMenuItemFromModel(model_,
+                                                             model_index, id);
+    if (model_->GetTypeAt(model_index) == MenuModel::TYPE_COMMAND) {
+      menu_item->SetMargins(0, 0);
+      const extensions::Extension* extension =
+          model_->GetActionBoxExtensionByIndex(model_index);
+      BrowserActionView* view = new BrowserActionView(extension, this);
+      view->button()->SetShowMultipleIconStates(false);
+      view->button()->DisableTooltip(true);
+      browser_action_views_.push_back(view);
+      menu_item->SetIconView(view);
+    }
+    id++;
+  }
+}
+
+MenuItemView* ActionBoxMenu::AddBookmarkMenuItem(MenuItemView* parent,
+                                                 int* index, int* 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);
+  MenuItemView* item = parent->AddMenuItemAt(*index, *id, label, *icon,
+                                             MenuItemView::NORMAL);

[Aaron Boodman, 2012/07/02 22:41:34]

Don't any of the AppendMenuItem* methods work? If so, you could get rid of the
|index| variable from here and the call site.

[yefimt, 2012/07/11 22:34:34]

Done.

+  (*id)++;
+  (*index)++;
+  return item;
+}