Move ScriptBadge, ActionInfo out of Extension; preparation for BrowserAction

Move ScriptBadge, ActionInfo out of Extension; preparation for BrowserAction

Moves the parsing of the ScriptBadge key in the manifest out of extension.cc.  Also moves out the ActionInfo struct, which Extension should not need to know about.  Move some common parsing functions (e.g. LoadIconsFromDirectory) into a common helper file.  Rename extension_script_badge_api to script_badge_api; make a ScriptBadgeAPI PKS.

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

[Devlin, 2012-12-17 20:14:53]

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (left):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3074: } else {
Do we know yet how we want to handle circumstances like this, where we do stuff
even if the key isn't present?

[Devlin, 2012-12-17 20:16:42]

With one exception (see the comment), this should be mostly ready. There might
be some more fine-tuning, but feel free to look it over. :)

[Yoyo Zhou, 2012-12-18 02:01:41]

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/extension_action/script_badge_api.cc (right):

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/extension_action/script_badge_api.cc:7: #include
"chrome/browser/extensions/api/extension_action/script_badge_api_factory.h"
This file seems to be missing.

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/extension_action/script_badge_api.h (right):

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/extension_action/script_badge_api.h:14: class
ScriptBadgeAPI : public ProfileKeyedService {
By the way, I think it's reasonable to have a single ExtensionActionAPI to cover
script badges, browser actions, and page actions.

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:551: void
BrowserEventRouter::ExtensionActionExecuted(
FYI, this should eventually get moved out of this file. See this bug:
https://code.google.com/p/chromium/issues/detail?id=165209

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (left):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3074: } else {
On 2012/12/17 20:14:53, D Cronin wrote:
> Do we know yet how we want to handle circumstances like this, where we do
stuff
> even if the key isn't present?

This case is currently not yet handled in this CL, correct? I suggested a
default value being supplied in _manifest_features.json, but I can see there
might be a risk of the default values getting written down in the prefs
accidentally.

How about this: a manifest handler can have a default behavior if its key isn't
present. The default default behavior does nothing. The script badge default
behavior is to create the default script badge. So (since Parse doesn't make
sense, nothing is there to parse) let's say the handler can override
ManifestHandler::HasNoKey(extension, error), who has a default empty
implementation.

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

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:272: // browser_action entries in the
manifest.
or system_indicator?

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (left):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.h:58: 
doesn't seem necessary to remove this

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.h:1007: ActionInfo::Type action_type,
Cool, this was never used!

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util.cc (right):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util.cc:331: const
extensions::ActionInfo* action = extension->page_action_info();
Note that it's also not good for this class to know how to validate particular
API-specific manifest fields. So it's fine for now, but eventually these will
want to be pulled out of here. (Similar to ManifestHandler::Parse, we may want
ManifestHandler::Validate.)

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc
(right):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc:32:
// are used in manifest-parsing) are initialized.
It would be preferable to just register the manifest handlers directly here. But
additionally, you should subclass ExtensionManifestTest and do that in the SetUp
function.

I would also advocate for putting this test in
chrome/common/extensions/api/extension_action/script_badge_manifest_unittest.cc.
(I have no idea who pluralized manifests here.)

[Devlin, 2012-12-18 20:42:07]

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/extension_action/script_badge_api.cc (right):

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/extension_action/script_badge_api.cc:7: #include
"chrome/browser/extensions/api/extension_action/script_badge_api_factory.h"
On 2012/12/18 02:01:41, Yoyo Zhou wrote:
> This file seems to be missing.

Whoops; forgot to include that one in the cl. Added.

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/extension_action/script_badge_api.h (right):

https://codereview.chromium.org/11588004/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/extension_action/script_badge_api.h:14: class
ScriptBadgeAPI : public ProfileKeyedService {
On 2012/12/18 02:01:41, Yoyo Zhou wrote:
> By the way, I think it's reasonable to have a single ExtensionActionAPI to
cover
> script badges, browser actions, and page actions.

Okay.  Is it alright if that waits for another CL?  This one's already getting
fairly large.  If you think it should go in this one, though, I can fit it in.

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (left):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3074: } else {
On 2012/12/18 02:01:41, Yoyo Zhou wrote:
> On 2012/12/17 20:14:53, D Cronin wrote:
> > Do we know yet how we want to handle circumstances like this, where we do
> stuff
> > even if the key isn't present?
> 
> This case is currently not yet handled in this CL, correct? I suggested a
> default value being supplied in _manifest_features.json, but I can see there
> might be a risk of the default values getting written down in the prefs
> accidentally.
> 
> How about this: a manifest handler can have a default behavior if its key
isn't
> present. The default default behavior does nothing. The script badge default
> behavior is to create the default script badge. So (since Parse doesn't make
> sense, nothing is there to parse) let's say the handler can override
> ManifestHandler::HasNoKey(extension, error), who has a default empty
> implementation.

PatchSet 1 did not handle that, correct.  HasNoKey() has been implemented (and
works for this case with no issues), but I seem to recall that you didn't like
the fact that this meant we'd have to check each Handler - is that no longer a
big concern?

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

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:272: // browser_action entries in the
manifest.
On 2012/12/18 02:01:41, Yoyo Zhou wrote:
> or system_indicator?

Cut-and-pasted comment corrected.

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (left):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/e...
chrome/common/extensions/extension.h:58: 
On 2012/12/18 02:01:41, Yoyo Zhou wrote:
> doesn't seem necessary to remove this

Didn't realize I did that; whoops.

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc
(right):

https://codereview.chromium.org/11588004/diff/4001/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc:32:
// are used in manifest-parsing) are initialized.
On 2012/12/18 02:01:41, Yoyo Zhou wrote:
> It would be preferable to just register the manifest handlers directly here.
But
> additionally, you should subclass ExtensionManifestTest and do that in the
SetUp
> function.
> 
> I would also advocate for putting this test in
>
chrome/common/extensions/api/extension_action/script_badge_manifest_unittest.cc.
> (I have no idea who pluralized manifests here.)

File has been moved and ScriptBadgeManifestTest::SetUp has been added.  My only
concern with registering the handlers directly is that we'd have to register
_all_ the used handlers.  Typically, manifest tests appear to be small enough
that this isn't a big deal, but some features (e.g., icons) are used somewhat
frequently throughout multiple tests.  Would we want to instead just have
ExtensionManifestTest::SetUp which registers all existing handlers? (This would
be slower, but perhaps more stable, since we only ever have to modify that one
class; here, if we change icons, we'll also have to change the script badge
unittest, which is somewhat strange.)

I actually have no preference either way; just mentioning it as an option.

[Yoyo Zhou, 2012-12-19 01:38:50]

LGTM with nits. If you want to rename ScriptBadgeAPI to ExtensionActionAPI in
the next CL, that's okay.

https://chromiumcodereview.appspot.com/11588004/diff/4001/chrome/common/exten...
File
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11588004/diff/4001/chrome/common/exten...
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc:32:
// are used in manifest-parsing) are initialized.
On 2012/12/18 20:42:07, D Cronin wrote:
> On 2012/12/18 02:01:41, Yoyo Zhou wrote:
> > It would be preferable to just register the manifest handlers directly here.
> But
> > additionally, you should subclass ExtensionManifestTest and do that in the
> SetUp
> > function.
> > 
> > I would also advocate for putting this test in
> >
>
chrome/common/extensions/api/extension_action/script_badge_manifest_unittest.cc.
> > (I have no idea who pluralized manifests here.)
> 
> File has been moved and ScriptBadgeManifestTest::SetUp has been added.  My
only
> concern with registering the handlers directly is that we'd have to register
> _all_ the used handlers.  Typically, manifest tests appear to be small enough
> that this isn't a big deal, but some features (e.g., icons) are used somewhat
> frequently throughout multiple tests.  Would we want to instead just have
> ExtensionManifestTest::SetUp which registers all existing handlers? (This
would
> be slower, but perhaps more stable, since we only ever have to modify that one
> class; here, if we change icons, we'll also have to change the script badge
> unittest, which is somewhat strange.)
> 
> I actually have no preference either way; just mentioning it as an option.

I think it's okay to make unit tests explicitly register the handlers they need.
Of course, if it gets to be unwieldy for some particular test then we can
reevaluate.

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/script_badge_api.h (right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/script_badge_api.h:23: static
ScriptBadgeAPI* Get(Profile* profile);
If no one calls this, delete it.

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/script_badge_api_factory.h
(right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/script_badge_api_factory.h:17:
static ScriptBadgeAPI* GetForProfile(Profile* profile);
Likewise, if no one calls this, delete it.

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
File chrome/common/extensions/api/extension_action/script_badge_handler.cc
(right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
chrome/common/extensions/api/extension_action/script_badge_handler.cc:98: void
ScriptBadgeHandler::SetActionInfoDefaults(Extension* extension,
should be const Extension* here

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
File chrome/common/extensions/manifest_handler.h (right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
chrome/common/extensions/manifest_handler.h:33: // Perform any initialization
which is necessary even if the Handler's key is
"even if" makes it sound like this also happens when the key is present. Change
this to: "Perform any necessary initialization when the..."

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
chrome/common/extensions/manifest_handler.h:37: virtual bool HasNoKey(Extension*
extension, string16* error) { return true; }
nit: this would be more readable with newlines.

[Devlin, 2012-12-19 19:38:16]

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/script_badge_api.h (right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/script_badge_api.h:23: static
ScriptBadgeAPI* Get(Profile* profile);
On 2012/12/19 01:38:50, Yoyo Zhou wrote:
> If no one calls this, delete it.

Done.

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/script_badge_api_factory.h
(right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/script_badge_api_factory.h:17:
static ScriptBadgeAPI* GetForProfile(Profile* profile);
On 2012/12/19 01:38:50, Yoyo Zhou wrote:
> Likewise, if no one calls this, delete it.

Done.

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
File chrome/common/extensions/api/extension_action/script_badge_handler.cc
(right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
chrome/common/extensions/api/extension_action/script_badge_handler.cc:98: void
ScriptBadgeHandler::SetActionInfoDefaults(Extension* extension,
On 2012/12/19 01:38:50, Yoyo Zhou wrote:
> should be const Extension* here

Done.

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
File chrome/common/extensions/manifest_handler.h (right):

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
chrome/common/extensions/manifest_handler.h:33: // Perform any initialization
which is necessary even if the Handler's key is
On 2012/12/19 01:38:50, Yoyo Zhou wrote:
> "even if" makes it sound like this also happens when the key is present.
Change
> this to: "Perform any necessary initialization when the..."

Done.

https://chromiumcodereview.appspot.com/11588004/diff/36002/chrome/common/exte...
chrome/common/extensions/manifest_handler.h:37: virtual bool HasNoKey(Extension*
extension, string16* error) { return true; }
On 2012/12/19 01:38:50, Yoyo Zhou wrote:
> nit: this would be more readable with newlines.

Done.

[Devlin, 2012-12-19 19:41:38]

+ ben for chrome/*.gypi, c/c/badge_util.*, c/c/icon_with_badge_image_source.*,
c/b/ui/views/
+ erg for chrome/browser/ui/gtk
+ mirandac for c/b/profiles/

Thanks!

[Miranda Callahan, 2013-01-02 14:34:53]

c/b/profiles/* LGTM.

On 2012/12/19 19:41:38, D Cronin wrote:
> + ben for chrome/*.gypi, c/c/badge_util.*, c/c/icon_with_badge_image_source.*,
> c/b/ui/views/
> + erg for chrome/browser/ui/gtk
> + mirandac for c/b/profiles/
> 
> Thanks!

[Elliot Glaysher, 2013-01-02 18:57:28]

Your gtk changes are TBRable. (but lgtm anyway)

[Devlin, 2013-01-04 16:28:32]

-reviewer ben
+tbr ben
(The changes are all small, and do not add any new functionality.)

[commit-bot: I haz the power, 2013-01-04 16:28:53]

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

[commit-bot: I haz the power, 2013-01-04 17:02:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Devlin, 2013-01-04 20:06:45]

Turns out an exception needs to be added to the gypi, since extension apis are
not, by default, compiled in android.
-tbr ben@
+reviewer ben@
+reviewer nileshagrawal@

Please check that this exception is okay.

[nilesh, 2013-01-04 20:21:45]

On 2013/01/04 20:06:45, D Cronin wrote:
> Turns out an exception needs to be added to the gypi, since extension apis are
> not, by default, compiled in android.
> -tbr ben@
> +reviewer ben@
> +reviewer nileshagrawal@
> 
> Please check that this exception is okay.
LGTM for the exception include in chrome/chrome_common.gypi

[Devlin, 2013-01-10 22:27:39]

On 2013/01/04 20:21:45, nileshagrawal1 wrote:
> On 2013/01/04 20:06:45, D Cronin wrote:
> > Turns out an exception needs to be added to the gypi, since extension apis
are
> > not, by default, compiled in android.
> > -tbr ben@
> > +reviewer ben@
> > +reviewer nileshagrawal@
> > 
> > Please check that this exception is okay.
> LGTM for the exception include in chrome/chrome_common.gypi

Ben, could you please take a look and verify the added exception is OK?

[Ben Goodger (Google), 2013-01-11 23:25:37]

lgtm

[commit-bot: I haz the power, 2013-01-16 19:42:09]

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

[commit-bot: I haz the power, 2013-01-16 23:36:38]

Change committed as 177265