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 "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"
+
+// ActionBoxMenu ---------------------------------------------------------------

[Peter Kasting, 2012/07/14 02:08:01]

In some files you use dividers like:

/////////...
// Foo

And in some you use this style.  Be consistent across all files (and ideally
match the rest of the location bar code).

Similarly, be consistent across all files about whether there is one or two
blank lines between #includes and everything else.

[yefimt, 2012/07/17 18:20:37]

Done.

+ActionBoxMenu::ActionBoxMenu(Browser* browser,

[Peter Kasting, 2012/07/14 02:08:01]

Nit: Blank line here

[yefimt, 2012/07/17 18:20:37]

Done.

+                             ActionBoxMenuModel* model,
+                             bool bookmark_item_state)
+    : browser_(browser),
+      root_(NULL),
+      model_(model),
+      bookmark_item_state_(bookmark_item_state) {
+}
+
+ActionBoxMenu::~ActionBoxMenu() {
+}
+
+void ActionBoxMenu::Init() {
+  CHECK(!root_);
+  root_ = new views::MenuItemView(this);
+  // We have icons, set this so we get the taller menu style.
+  root_->set_has_icons(true);
+  PopulateMenu();
+  menu_runner_.reset(new views::MenuRunner(root_));
+}
+
+void ActionBoxMenu::RunMenu(views::MenuButton* host) {
+  gfx::Point screen_location;
+  views::View::ConvertPointToScreen(host, &screen_location);
+  gfx::Rect bounds(screen_location, host->size());

[Peter Kasting, 2012/07/14 02:08:01]

Nit: Inline into next statement

[yefimt, 2012/07/17 18:20:37]

Done.

+  if (menu_runner_->RunMenuAt(host->GetWidget(),

[Peter Kasting, 2012/07/14 02:08:01]

You're not doing anything special for MENU_DELETED, so don't check the return
value.  Use ignore_result() instead and add a comment (see e.g.
infobar_view.cc).

[yefimt, 2012/07/17 18:20:37]

Done.

+                              host,
+                              bounds,
+                              views::MenuItemView::TOPRIGHT,
+                              views::MenuRunner::HAS_MNEMONICS) ==
+          views::MenuRunner::MENU_DELETED) {
+    return;
+  }
+}
+
+void ActionBoxMenu::ExecuteCommand(int id) {
+};
+
+views::Border* ActionBoxMenu::CreateMenuBorder() {
+  return views::Border::CreateSolidBorder(1, SkColorSetRGB(0, 0, 0));

[Peter Kasting, 2012/07/14 02:08:01]

Don't hardcode colors.  You need to use the appropriate system/theme colors.

[yefimt, 2012/07/17 18:20:37]

I don't have written specs yet, just mocks, but I'm pretty sure they want
specific colors, not theme colors.

On 2012/07/14 02:08:01, Peter Kasting wrote:

+}

[yefimt, 2012/07/18 23:18:13]

Can I do it later. This border is temporary, only to let me check in so other
people hookup their code to this menu.
I'll be implementing it later per UI specs.

[Peter Kasting, 2012/07/19 04:27:31]

Yes, doing it later is fine.

+
+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(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;

[Peter Kasting, 2012/07/14 02:08:01]

Nit: If you name this |bookmark_item_id|, you can replace the use in the loop
below with "bookmark_item_id + model_index" and eliminate the increment.

[yefimt, 2012/07/17 18:20:37]

It is going to be a couple more menu items soon (before extensions).

On 2012/07/14 02:08:01, Peter Kasting wrote:

+  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);
+    if (model_->GetTypeAt(model_index) == ui::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()->SetTooltipDisabled(true);
+      browser_action_views_.push_back(linked_ptr<BrowserActionView>(view));
+      menu_item->SetIconView(view);
+    }
+    item_id++;
+  }
+}
+
+views::MenuItemView* ActionBoxMenu::AddBookmarkMenuItem(
+    views::MenuItemView* parent, int* item_id) {

[Peter Kasting, 2012/07/14 02:08:01]

Nit: One arg per line

[yefimt, 2012/07/17 18:20:37]

Done.

+  string16 label =

[Peter Kasting, 2012/07/14 02:08:01]

Nit: Wrap like:

  string16 label = l10n_util::GetStringUTF16(bookmark_item_state_ ?
      IDS_TOOLTIP_STARRED : IDS_TOOLTIP_STAR);

[yefimt, 2012/07/17 18:20:37]

Done.

+      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);
+  views::MenuItemView* item =
+      parent->AppendMenuItemWithIcon(*item_id, label, *icon);
+  (*item_id)++;
+  return item;
+}