Move ExtensionAction from common/ to browser/.

Move ExtensionAction from common/ to browser/.

Chrome builds, but not the unit tests, yet.

[Jeffrey Yasskin, 2012-09-13 23:51:58]

This is hacky in several places, but it appears to be possible.

http://codereview.chromium.org/10911300/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10911300/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:831: // Keyed by Extension ID. 
These maps are populated when the extension is
These actually need to be stored in an ExtensionActionManager of some sort, but
this was easier for the demonstration that it's possible.

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

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_action.h (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_action.h:352: ExtensionAction*
GetBrowserAction(Profile* profile, const Extension& extension);
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.

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1865:
STLDeleteValues(&page_actions_);
Need to double-check if there are other places that can unload extensions
without sending the EXTENSION_UNLOADED notification, and call into the
ExtensionActionManager from there too.

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:835: std::map<std::string,
ExtensionAction*> page_actions_;
These actually need to be stored in an ExtensionActionManager of some sort
instead of directly in the ExtensionService. I think it makes sense to put the
ExtensionActionManager in the ExtensionService instead of the ExtensionSystem,
but I'm not particularly clear on the difference between those classes.

(Repeated because I deleted the patch set that held this comment.)

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:26: #include
"chrome/browser/extensions/extension_action.h"
I haven't re-sorted all the includes yet.

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

This will be a really nice cleanup.

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_action.h (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_action.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_EXTENSION_ACTION_H_
Wow, neat that you can literally just move the file. I guess this is evidence it
belonged here :).

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_action.h:352: ExtensionAction*
GetBrowserAction(Profile* profile, const Extension& extension);
On 2012/09/14 19:18:33, Jeffrey Yasskin wrote:
> 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.

namespace: yes, keep 'Extension' prefix: yes

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_action.h:352: ExtensionAction*
GetBrowserAction(Profile* profile, const Extension& extension);
On 2012/09/14 19:18:33, Jeffrey Yasskin wrote:
> 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?

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.

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2422: case
chrome::NOTIFICATION_EXTENSION_LOADED: {
This is the kind of code I'm trying to keep out of ExtensionService. Should be
in ExtensionActionManager and use EXTENSION_LOADED/UNLOADED notifications to
populate self.

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:835: std::map<std::string,
ExtensionAction*> page_actions_;
On 2012/09/14 19:18:33, Jeffrey Yasskin wrote:
> These actually need to be stored in an ExtensionActionManager of some sort
> instead of directly in the ExtensionService. I think it makes sense to put the
> ExtensionActionManager in the ExtensionService instead of the ExtensionSystem,
> but I'm not particularly clear on the difference between those classes.
> 
> (Repeated because I deleted the patch set that held this comment.)

Per current design, it should be ExtensionSystem.

ExtensionService is an older class that was mostly about managing the list of
loaded extensions, but slowly grew to include lots of glue code integrating
extensions with many other subsystems.

The idea for ExtensionSystem was basically to be ExtensionService v2, but minus
all the glue. It just owns other parts of the extension system and maybe calls
Init().

It seems like maybe it isn't working out and we should reconsider it (again),
but that's the current intent anyway.

http://codereview.chromium.org/10911300/diff/7001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:214: TYPE_BROWSER,
Do we need to carry around the type? The caller knows what type it is because of
the member it comes from. Perhaps this information only needed in browser?

[Aaron Boodman, 2012-09-26 18:06:42]

Is someone going to finish this? I think it's a really nice and important
cleanup.

[Aaron Boodman, 2012-09-26 18:07:02]

+mpcomplete, kalman fyi

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

Some thoughts from reading the review comments. I haven't actually gone through
the code itself.

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_action.h (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_action.h:352: ExtensionAction*
GetBrowserAction(Profile* profile, const Extension& extension);
On 2012/09/16 01:42:50, Aaron Boodman wrote:
> On 2012/09/14 19:18:33, Jeffrey Yasskin wrote:
> > 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?
> 
> 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.

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).

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:2422: case
chrome::NOTIFICATION_EXTENSION_LOADED: {
On 2012/09/16 01:42:50, Aaron Boodman wrote:
> This is the kind of code I'm trying to keep out of ExtensionService. Should be
> in ExtensionActionManager and use EXTENSION_LOADED/UNLOADED notifications to
> populate self.

Not that this solves the LOADED/UNLOADED problem, but what we need is an
ExtensionMap like ExtensionSet.

http://codereview.chromium.org/10911300/diff/7001/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10911300/diff/7001/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:214: TYPE_BROWSER,
On 2012/09/16 01:42:50, Aaron Boodman wrote:
> Do we need to carry around the type? The caller knows what type it is because
of
> the member it comes from. Perhaps this information only needed in browser?

It was initially added for when page actions got implicitly converted into
browser actions, for the script badges. Even though extension->browser_action()
returned an ExtensionAction, it was actually a page action and the API calls
needed to be made as such.

Since then it's also being used to decide what sizes to make the icons, I think.

While explaining that it makes me think that this is the wrong model to use, and
what we actually want is for the browser version of ExtensionAction to have a
"location" property, like

enum Location {
  LOCATION_BAR,
  TOOLBAR,
  ACTION_BOX
};

and then for the "type" to be removed and code which relies on it to use the
Location property instead.