Move ExtensionAction from common/ to browser/.

chrome/browser/extensions/extension_action.h

Index: chrome/browser/extensions/extension_action.h
diff --git a/chrome/common/extensions/extension_action.h b/chrome/browser/extensions/extension_action.h
similarity index 93%
rename from chrome/common/extensions/extension_action.h
rename to chrome/browser/extensions/extension_action.h
index 5a655ff69159738a984a27573159aa634a43c14e..43ecc51e84aab030d58e08cb91604e0fce51d668 100644
--- a/chrome/common/extensions/extension_action.h
+++ b/chrome/browser/extensions/extension_action.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_COMMON_EXTENSIONS_EXTENSION_ACTION_H_
-#define CHROME_COMMON_EXTENSIONS_EXTENSION_ACTION_H_
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_ACTION_H_

[Aaron Boodman, 2012/09/16 01:42:50]

Wow, neat that you can literally just move the file. I guess this is evidence it
belonged here :).

+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_ACTION_H_
 
 #include <map>
 #include <string>
@@ -14,10 +14,12 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "chrome/common/extensions/extension.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/animation/linear_animation.h"
 
 class GURL;
+class Profile;
 class SkBitmap;
 class SkDevice;
 
@@ -39,13 +41,6 @@ class ExtensionAction {
   // parameter.
   static const int kDefaultTabId;
 
-  // The types of extension actions.
-  enum Type {
-    TYPE_BROWSER,
-    TYPE_PAGE,
-    TYPE_SCRIPT_BADGE,
-  };
-
   enum Appearance {
     // The action icon is hidden.
     INVISIBLE,
@@ -116,7 +111,8 @@ class ExtensionAction {
     DISALLOW_COPY_AND_ASSIGN(IconAnimation);
   };
 
-  ExtensionAction(const std::string& extension_id, Type action_type);
+  ExtensionAction(const std::string& extension_id,
+                  const extensions::Extension::ActionInfo& manifest_info);
   ~ExtensionAction();
 
   // Gets a copy of this, ownership passed to caller.
@@ -127,7 +123,9 @@ class ExtensionAction {
   const std::string& extension_id() const { return extension_id_; }
 
   // What kind of action is this?
-  Type action_type() const { return action_type_; }
+  extensions::Extension::ActionInfo::Type action_type() const {
+    return action_type_;
+  }
 
   // action id -- only used with legacy page actions API
   std::string id() const { return id_; }
@@ -303,7 +301,7 @@ class ExtensionAction {
   // extension manifest).
   const std::string extension_id_;
 
-  const Type action_type_;
+  const extensions::Extension::ActionInfo::Type action_type_;
 
   // Each of these data items can have both a global state (stored with the key
   // kDefaultTabId), or tab-specific state (stored with the tab_id as the key).
@@ -350,4 +348,10 @@ struct ExtensionAction::ValueTraits<int> {
   }
 };
 
-#endif  // CHROME_COMMON_EXTENSIONS_EXTENSION_ACTION_H_
+namespace extensions {
+ExtensionAction* GetBrowserAction(Profile* profile, const Extension& extension);

[Jeffrey Yasskin, 2012/09/14 19:18:33]

Functions along these lines are important to shorten code that needs to retrieve
ExtensionActions, but they could live in several places. Aaron, do you have an
opinion on where?

Relatedly, should we put ExtensionAction in the extensions namespace in this
change? I don't think removing the Extension prefix makes sense even if we put
it in the namespace.

[Aaron Boodman, 2012/09/16 01:42:50]

namespace: yes, keep 'Extension' prefix: yes

[Aaron Boodman, 2012/09/16 01:42:50]

The downside of free functions like this is that it's not immediately clear when
reading the code where they come from.

I'd typically prefer them as static members of ExtensionAction, but then I'm not
sure if it's enough of a typing savings.

[not at google - send to devlin, 2012/09/27 00:46:10]

I also do not like free/utility functions like this.

Even if they do live as static functions on ExtensionAction, it slightly
obfuscates where the ExtensionActions come from, with the only advantage being
saving a bit of typing. It also means that somebody cargo-culting code might
think they need a Profile when really all they need is an
ExtensionActionManager, which leads to dependency bloat and testing
difficulties.

Aside: I get upset every time I need to find the header file where
chrome::GetActiveWebContents lives. Grr! Y U NOT ON BROWSER.

Yennyway, the question is just who owns the ExtensionActions now. And the answer
is (I hear) ExtensionSystem (or some object which hangs off it like an
ExtensionActionManager -- which I think is a good idea, keep dependencies
small). If a class repeatedly gets page actions and wants to save typing, it can
cache a reference to the ExtensionActionManager (though more likely it's just
going to cache a reference to the ExtensionAction itself).

+ExtensionAction* GetPageAction(Profile* profile, const Extension& extension);
+ExtensionAction* GetScriptBadge(Profile* profile, const Extension& extension);
+}
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_ACTION_H_