Move BrowserAction out of Extension

Move BrowserAction out of Extension

BUG=159265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179186

[Devlin, 2012-12-20 23:32:40]

FYI, had to change a few tests not directly related to browser action since they
had browser actions in their manifests, and there may be some stragglers -
trying bots to find out.

[Yoyo Zhou, 2012-12-21 23:50:59]

https://codereview.chromium.org/11644057/diff/7001/chrome/browser/ui/cocoa/ex...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/11644057/diff/7001/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:27: #include
"chrome/common/extensions/api/extension_action/browser_action_handler.h"
alphabetize

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/extension_action/browser_action_handler.h
(right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/extension_action/browser_action_handler.h:16:
struct BrowserActionInfo : public Extension::ManifestData {
I'm not sure that we need different wrapper types around an ActionInfo. Can we
have a single ActionInfo that's used for script badges, page actions, browser
actions?

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
File
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc
(right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc:23:
virtual void SetUp() OVERRIDE {
This should call the superclass SetUp also (even if it currently does nothing).

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1278: 
newline is extraneous

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1279: DCHECK(!finished_parsing_manifest_);
&& thread_checker_.CalledOnValidThread()

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util_unittest.cc (right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util_unittest.cc:27: class
ExtensionFileUtil : public testing::Test {
ExtensionFileUtilTest is a better name.

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util_unittest.cc:29: virtual void
SetUp() OVERRIDE {
call base class SetUp

[Devlin, 2012-12-27 20:36:46]

https://codereview.chromium.org/11644057/diff/7001/chrome/browser/ui/cocoa/ex...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/11644057/diff/7001/chrome/browser/ui/cocoa/ex...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:27: #include
"chrome/common/extensions/api/extension_action/browser_action_handler.h"
On 2012/12/21 23:50:59, Yoyo Zhou wrote:
> alphabetize

Done.

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/extension_action/browser_action_handler.h
(right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/extension_action/browser_action_handler.h:16:
struct BrowserActionInfo : public Extension::ManifestData {
On 2012/12/21 23:50:59, Yoyo Zhou wrote:
> I'm not sure that we need different wrapper types around an ActionInfo. Can we
> have a single ActionInfo that's used for script badges, page actions, browser
> actions?

Done.

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
File
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc
(right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc:23:
virtual void SetUp() OVERRIDE {
On 2012/12/21 23:50:59, Yoyo Zhou wrote:
> This should call the superclass SetUp also (even if it currently does
nothing).

Done.

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1278: 
On 2012/12/21 23:50:59, Yoyo Zhou wrote:
> newline is extraneous

Done.

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:1279: DCHECK(!finished_parsing_manifest_);
On 2012/12/21 23:50:59, Yoyo Zhou wrote:
> && thread_checker_.CalledOnValidThread()

Done.

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util_unittest.cc (right):

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util_unittest.cc:27: class
ExtensionFileUtil : public testing::Test {
On 2012/12/21 23:50:59, Yoyo Zhou wrote:
> ExtensionFileUtilTest is a better name.

I thought so too, but wasn't sure I should change the full test suite name (I
typically avoid that, so that searching trybot records, etc, is the same when
possible). But since you think the name's a poor choice, too, done.

https://codereview.chromium.org/11644057/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util_unittest.cc:29: virtual void
SetUp() OVERRIDE {
On 2012/12/21 23:50:59, Yoyo Zhou wrote:
> call base class SetUp

Done.

[Yoyo Zhou, 2012-12-28 05:19:59]

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/common/exte...
File chrome/common/extensions/api/extension_action/action_info.h (right):

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/common/exte...
chrome/common/extensions/api/extension_action/action_info.h:38: struct
ActionInfoData : public Extension::ManifestData {
Can you hide this in the anonymous namespace in the .cc? It's not necessary to
expose this part of the implementation.

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/common/exte...
chrome/common/extensions/api/extension_action/action_info.h:47: static const
ActionInfo* GetBrowserActionInfo(const Extension* extension);
Can you put these in ActionInfo? It reads better than ActionInfoData in the
interface, especially since you get an ActionInfo*.

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/common/exte...
File
chrome/common/extensions/manifest_tests/extension_manifests_pageaction_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/common/exte...
chrome/common/extensions/manifest_tests/extension_manifests_pageaction_unittest.cc:7:
#include "chrome/common/extensions/api/extension_action/action_info.h"
Is this used?

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/renderer/ex...
File chrome/renderer/extensions/page_actions_custom_bindings.cc (right):

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/renderer/ex...
chrome/renderer/extensions/page_actions_custom_bindings.cc:9: #include
"chrome/common/extensions/api/extension_action/action_info.h"
ditto

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/utility/chr...
File chrome/utility/chrome_content_utility_client.cc (right):

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/utility/chr...
chrome/utility/chrome_content_utility_client.cc:50: // Explicitly register all
ManifestHandlers, since the subprocess won't have the
Comment should read "all ManifestHandlers needed in the utility process."

https://chromiumcodereview.appspot.com/11644057/diff/17014/chrome/utility/chr...
chrome/utility/chrome_content_utility_client.cc:52: void
RegisterManifestHandlers() {
RegisterExtensionManifestHandlers is better.

[Devlin, 2012-12-28 20:25:36]

https://codereview.chromium.org/11644057/diff/17014/chrome/common/extensions/...
File chrome/common/extensions/api/extension_action/action_info.h (right):

https://codereview.chromium.org/11644057/diff/17014/chrome/common/extensions/...
chrome/common/extensions/api/extension_action/action_info.h:38: struct
ActionInfoData : public Extension::ManifestData {
On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> Can you hide this in the anonymous namespace in the .cc? It's not necessary to
> expose this part of the implementation.

Possible, as long as we also put in Set|BrowserAction,ScriptBadge|Info methods;
done.

https://codereview.chromium.org/11644057/diff/17014/chrome/common/extensions/...
chrome/common/extensions/api/extension_action/action_info.h:47: static const
ActionInfo* GetBrowserActionInfo(const Extension* extension);
On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> Can you put these in ActionInfo? It reads better than ActionInfoData in the
> interface, especially since you get an ActionInfo*.

Agreed, and done.

https://codereview.chromium.org/11644057/diff/17014/chrome/common/extensions/...
File
chrome/common/extensions/manifest_tests/extension_manifests_pageaction_unittest.cc
(right):

https://codereview.chromium.org/11644057/diff/17014/chrome/common/extensions/...
chrome/common/extensions/manifest_tests/extension_manifests_pageaction_unittest.cc:7:
#include "chrome/common/extensions/api/extension_action/action_info.h"
On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> Is this used?

Yeah, on lines 18 - 23, we access page_action_info()->*.  Previously, it relied
on transitive includes for both Extension and ActionInfo.

https://codereview.chromium.org/11644057/diff/17014/chrome/renderer/extension...
File chrome/renderer/extensions/page_actions_custom_bindings.cc (right):

https://codereview.chromium.org/11644057/diff/17014/chrome/renderer/extension...
chrome/renderer/extensions/page_actions_custom_bindings.cc:9: #include
"chrome/common/extensions/api/extension_action/action_info.h"
On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> ditto

ditto :) (line 35)

https://codereview.chromium.org/11644057/diff/17014/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/11644057/diff/17014/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:50: // Explicitly register all
ManifestHandlers, since the subprocess won't have the
On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> Comment should read "all ManifestHandlers needed in the utility process."

Done.

https://codereview.chromium.org/11644057/diff/17014/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:52: void
RegisterManifestHandlers() {
On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> RegisterExtensionManifestHandlers is better.

Done.

[Yoyo Zhou, 2012-12-28 21:35:32]

LGTM

https://codereview.chromium.org/11644057/diff/17014/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/11644057/diff/17014/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:50: // Explicitly register all
ManifestHandlers, since the subprocess won't have the
On 2012/12/28 20:25:36, D Cronin wrote:
> On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> > Comment should read "all ManifestHandlers needed in the utility process."
> 
> Done.

I meant you could also remove the "since..." bit.

[Devlin, 2013-01-23 23:48:00]

+ ben@ for gypi changes and chrome/utility/chrome_content_utility_client.cc
+ nileshagrawal@ for gypi/android changes

https://codereview.chromium.org/11644057/diff/17014/chrome/utility/chrome_con...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/11644057/diff/17014/chrome/utility/chrome_con...
chrome/utility/chrome_content_utility_client.cc:50: // Explicitly register all
ManifestHandlers, since the subprocess won't have the
On 2012/12/28 21:35:33, Yoyo Zhou wrote:
> On 2012/12/28 20:25:36, D Cronin wrote:
> > On 2012/12/28 05:19:59, Yoyo Zhou wrote:
> > > Comment should read "all ManifestHandlers needed in the utility process."
> > 
> > Done.
> 
> I meant you could also remove the "since..." bit.

Done.

[Agrawal, 2013-01-24 04:08:22]

On 2013/01/23 23:48:00, D Cronin wrote:
> + ben@ for gypi changes and chrome/utility/chrome_content_utility_client.cc
> + nileshagrawal@ for gypi/android changes
 lgtm

[Ben Goodger (Google), 2013-01-28 17:35:16]

lgtm

[commit-bot: I haz the power, 2013-01-28 18:32:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11644057/8...

[commit-bot: I haz the power, 2013-01-28 20:38:17]

Change committed as 179186