Action box menu

chrome/browser/ui/toolbar/action_box_menu_model.h

+// 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 "base/file_path.h"

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

nit: is this include necessary?

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

Done.

+#include "base/memory/scoped_ptr.h"

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

nit: is this include necessary?

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

Done.

+#include "content/public/browser/notification_observer.h"
+#include "ui/base/models/button_menu_item_model.h"

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

nit: is this include necessary?

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

Done.

+#include "ui/base/models/simple_menu_model.h"
+
+class ExtensionService;
+
+namespace extensions {
+class Extension;
+}
+
+

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

nit: remove extra blank line.

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

Done.

+// A menu model that builds the contents of the action box menu.
+class ActionBoxMenuModel : public ui::SimpleMenuModel,
+                           public ui::SimpleMenuModel::Delegate,
+                           public content::NotificationObserver {
+ public:
+  explicit ActionBoxMenuModel(ExtensionService* extension_service);
+  virtual ~ActionBoxMenuModel();
+
+  // Active box extensions.

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

nit: "Action" not "Active"? Capitalize "Box"? Remove or expand upon comment.

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

Done.

+  size_t action_box_extensions_size();
+  const extensions::Extension* GetActionBoxExtensionByIndex(int index);
+
+ private:
+  typedef std::map<int, std::string> IdToEntensionIdMap;
+
+  // Overridden for both SimpleMenuModel::Delegate and SimpleMenuModel.

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

nit: These are all actually from SimpleMenuModel::Delegate, use "Overridden
from" standard language.

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

Done.

+  virtual void ExecuteCommand(int command_id) OVERRIDE;

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

nit: Match OVERRIDE declaration order and visibility (public); same for Observe.

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

Done - for order
As for visibility, aa@ asked me to make them private. I dont remember if it was
this place or not, but I'm sure he asked for all similar places.

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

OK, there's no official style guide rule, so I won't press further, but I
generally prefer to retain the original visibility.

[Peter Kasting, 2012/07/26 02:32:27]

(FWIW, I didn't say anything on this patch to my knowledge, but I generally
direct people to make OVERRIDE functions as private as possible without regard
to their original visibility.  I can see arguments both ways though.)

+  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;
+
+  // Overridden from content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // Adds extensions to the model.
+  void Build();

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

nit: name standard |Init|? Or inline in ctor?

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

Moved

+
+  ExtensionService* extension_service_;
+
+  IdToEntensionIdMap id_to_extension_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActionBoxMenuModel);
+};
+
+#endif  // CHROME_BROWSER_UI_TOOLBAR_ACTION_BOX_MENU_MODEL_H_