Action box menu

chrome/browser/ui/views/action_box_menu.h

Index: chrome/browser/ui/views/action_box_menu.h
diff --git a/chrome/browser/ui/views/action_box_menu.h b/chrome/browser/ui/views/action_box_menu.h
new file mode 100644
index 0000000000000000000000000000000000000000..a788830ba989ad0f76f47a754ec2461bc6b95258
--- /dev/null
+++ b/chrome/browser/ui/views/action_box_menu.h
@@ -0,0 +1,91 @@
+// 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_ACTION_BOX_MENU_H_

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

nit: do these files belong in the toolbar subdir?

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

Not sure. wrench_menu.* is in this folder too.
I dont think toolbar is a right place, maybe location_bar?

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

Ah, I made that recommendation too hastily, thinking that
chrome/browser/ui/toolbar/action_box_menu_model.[h|cc] was actually in
ui/views... Perhaps this does belong in location_bar? I think it's fine to leave
it here for now.

+#define CHROME_BROWSER_UI_VIEWS_ACTION_BOX_MENU_H_
+
+#include <map>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "chrome/browser/ui/views/browser_action_view.h"
+#include "content/public/browser/notification_observer.h"
+#include "ui/base/models/menu_model.h"

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

nit: is this include necessary?

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

Done.

+#include "ui/views/controls/menu/menu_delegate.h"
+
+class ActionBoxMenuModel;
+
+namespace views {
+class Background;
+class Border;
+class MenuItemView;
+class MenuRunner;
+class View;
+}
+
+// ActionBoxMenu adapts the ActionBoxMenuModel to view's menu related classes.
+class ActionBoxMenu : public views::MenuDelegate,
+                      public BrowserActionView::Delegate,
+                      public content::NotificationObserver {
+ public:
+  ActionBoxMenu(Browser* browser,
+                ActionBoxMenuModel* model,
+                bool bookmark_item_state);
+  virtual ~ActionBoxMenu();
+
+  void Init();
+
+  // Shows the menu relative to the specified button.
+  void RunMenu(views::MenuButton* menu_button);
+
+ private:
+  // MenuDelegate overrides:

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

nit: Standardize terminology here with what you use elsewhere (other files).

+  virtual void ExecuteCommand(int id) OVERRIDE;

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

nit: Match OVERRIDE declaration visibility (public), same for others below.

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

As in other place I was asked by reviewer always make them private unless they
need to be called from outside.

[tfarina, 2012/07/26 02:37:54]

Yes, please, keep it private! outside of the public interface of ActionBoxMenu.
this is just an implementation detail that should not be called directly by the
clients.
No one is going to do:
ActionBoxMenu* menu = new ActionBoxMenu(...);
menu->ExecuteCommand(..);
This will be done through the delegate_->ExecuteCommand(...);

+  virtual views::Border* CreateMenuBorder() OVERRIDE;
+  virtual views::Background* CreateMenuBackground() OVERRIDE;
+
+  // BrowserActionView::Delegate and DragController overrides:

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

nit: Please differentiate OVERRIDEs by declaration, if possible.

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

I was asked lump them together :)

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

Okay, this is fine then.

+  virtual int GetCurrentTabId() const OVERRIDE;
+  virtual void OnBrowserActionExecuted(BrowserActionButton* button) OVERRIDE;
+  virtual void OnBrowserActionVisibilityChanged() OVERRIDE;
+  virtual gfx::Size GetViewContentOffset() const OVERRIDE;
+  virtual void WriteDragDataForView(views::View* sender,
+                                    const gfx::Point& press_pt,
+                                    ui::OSExchangeData* data) OVERRIDE;
+  virtual int GetDragOperationsForView(views::View* sender,
+                                       const gfx::Point& p) OVERRIDE;
+  virtual bool CanStartDragForView(views::View* sender,
+                                   const gfx::Point& press_pt,
+                                   const gfx::Point& p) OVERRIDE;
+
+  // NotificationObserver overrides:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // Creates bookmark menu item and adds it to menu, |next_id| is incremented.

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

grammar nit: consider "Adds a new bookmark menu item to the menu, ..."

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

Done.

+  views::MenuItemView* AddBookmarkMenuItem(views::MenuItemView* parent,
+                                           int* next_id);
+
+  // Populates |parent| with all the child menus in |model|.

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

nit: what's |parent| and |model|? old args? Please update.

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

Done.

+  void PopulateMenu();
+
+  Browser* browser_;
+
+  // The views menu. Owned by |menu_runner_|.
+  views::MenuItemView* root_;
+
+  scoped_ptr<views::MenuRunner> menu_runner_;
+
+  // The model that tracks the order of the toolbar icons.
+  ActionBoxMenuModel* model_;
+
+  bool bookmark_item_state_;

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

nit: description?

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

Done.

+
+  ScopedVector<BrowserActionView> browser_action_views_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActionBoxMenu);
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_ACTION_BOX_MENU_H_