Action box menu

chrome/browser/ui/toolbar/action_box_menu_model.h

Index: chrome/browser/ui/toolbar/action_box_menu_model.h
diff --git a/chrome/browser/ui/toolbar/action_box_menu_model.h b/chrome/browser/ui/toolbar/action_box_menu_model.h
new file mode 100644
index 0000000000000000000000000000000000000000..052d7694d6b5df71556f137b003abbf3eed09135
--- /dev/null
+++ b/chrome/browser/ui/toolbar/action_box_menu_model.h
@@ -0,0 +1,53 @@
+// 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_TOOLBAR_ACTION_BOX_MENU_MODEL_H_
+#define CHROME_BROWSER_UI_TOOLBAR_ACTION_BOX_MENU_MODEL_H_
+
+#include <map>
+
+#include "content/public/browser/notification_observer.h"
+#include "ui/base/models/simple_menu_model.h"
+
+class ExtensionService;
+
+namespace extensions {
+class Extension;
+}
+
+// A menu model that builds the contents of the action box menu.
+class ActionBoxMenuModel : public ui::SimpleMenuModel,

[Peter Kasting, 2012/07/26 20:37:17]

It seems a bit strange for this class to be both the menu model and the
delegate.  Based on the usage in ActionBoxMenu, shouldn't this class just be the
menu model, and the ActionBoxMenu that owns it be the delegate?  That way the
overlapping overrides from MenuDelegate and SimpleMenuModel::Delegate can be
implemented by as few functions as possible, all in one place.

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

I'll look into it, dont have answer yet.

[yefimt, 2012/07/31 00:29:08]

Done.

+                           public ui::SimpleMenuModel::Delegate,
+                           public content::NotificationObserver {
+ public:
+  explicit ActionBoxMenuModel(ExtensionService* extension_service);
+  virtual ~ActionBoxMenuModel();
+
+  size_t action_box_extensions_size();

[tfarina, 2012/07/26 02:43:40]

minor/tiny nit: Do they asked you to write in unix_hacker style for this
function? We usually use this style for inlined functions, i.e, size_t count()
const { return count_; }

If not, may be rename this to GetExtensionsCount, or
GetActionBoxExtensionsCount, or something...

[Peter Kasting, 2012/07/26 20:37:17]

The name here is correct, but the actual function definition should be inlined
here as well instead of remaining in the .cc file.

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

Done.

+  const extensions::Extension* GetActionBoxExtensionByIndex(int index);

[Peter Kasting, 2012/07/26 20:37:17]

Nit: This function should also be renamed unix_hacker()-style and inlined.

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

Done.

+
+ private:
+  typedef std::map<int, std::string> IdToEntensionIdMap;
+
+  // Overridden from SimpleMenuModel::Delegate.
+  virtual bool IsCommandIdChecked(int command_id) const OVERRIDE;
+  virtual bool IsCommandIdEnabled(int command_id) const OVERRIDE;
+  virtual bool GetAcceleratorForCommandId(
+      int command_id,
+      ui::Accelerator* accelerator) OVERRIDE;
+  virtual void ExecuteCommand(int command_id) OVERRIDE;
+
+  // Overridden from content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  ExtensionService* extension_service_;
+
+  IdToEntensionIdMap id_to_extension_id_;

[Peter Kasting, 2012/07/26 20:37:17]

Nit: Add "map_" to the end of the name

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ActionBoxMenuModel);
+};
+
+#endif  // CHROME_BROWSER_UI_TOOLBAR_ACTION_BOX_MENU_MODEL_H_