Implement API features for the Extension API feature system

Implement API features for the Extension API feature system

Now features can be declared in the _api_features.json file, and dependencies
of the form "api:<api_name>" will refer to these features.

This patch also prevents schemas for APIs using the feature system from being
loaded when calling ExtensionAPI::IsAvailable().

TBR=ben@chromium.org
BUG=55316, 120070, 224206
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190836

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

[not at google - send to devlin, 2013-03-26 00:34:48]

awesome, basically lg but there are a bunch of comments so might as well go
another round. also, in
chrome/test/data/extensions/extension_api_unittest/is_privileged_features_[12345].json
there is "uses_feature_system" in there - which we should now remove all
references to.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_api_features.json:15: "contexts":
["blessed_extension"],
hm, the json has a bug, this should include web_page too. And somewhere we
should assert that allowing web_page context must specify a matches.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:427: std::string api_name =
GetAPINameFromFullName(feature_name, &child_name);
nit: eyes want a newline after api_name.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:432: return
IsNonFeatureAPIAvailable(api_name, context, extension, url) ?
I think the GetSchema thing should go in IsNonFeatureAPIAvailable. It's an
implementation detail of that method.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:439: CHECK(feature) << full_name;
yeah, here we're CHECKing anyway.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:463: // in a blessed context.
I don't think this checking dependencies really makes sense anymore, since we
(well, you) fixed the type dependency problem properly. So this can be 1 line,
yay.

return feature->GetContexts()->count(Feature::BLESSED_EXTENSION_CONTEXT) > 0;

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:464: Feature* feature =
GetFeatureDependency(full_name);
and here we could assign Feature up on line 460 and then check it for null.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:480: const DictionaryValue* schema
= GetSchema(api_name);
nice catch.

// get now to populate |completely_unprivileged_apis_|s.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:610: Feature*
ExtensionAPI::TryGetFeatureDependency(const std::string& full_name) {
I think this is my favourite method. Let's delete UsesFeatureSystem and
GetFeatureDependency and rename this GetFeatureDependency.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:621: // Try getting the feature
for the parent API, if this was a child.
In the general case I think we need to step all the way up until there is no
more api name.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:106: Feature*
GetFeatureDependency(const std::string& dependency_name);
This doesn't appear to be used outside of this class, so it can be private.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/base_feature_provider.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:76: return
feature->Parse(value);
yeah, this could be

std::string error = feature->Parse(value);
if (error != "")
  LOG(ERROR) << error;
return error == "";

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/feature.h (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/feature.h:136: virtual std::set<Context>*
GetContexts() = 0;
i guess we're here now, but I kind of prefer keeping all the feature parsing in
the one place. remove the todo?

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/simple_feature.cc:228: return true;
i.e. return "" here

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/simple_feature.h (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/simple_feature.h:34: virtual bool Parse(const
DictionaryValue* value);
I think this should return a std::string error rather than everything logging it
then returning a bool.

https://codereview.chromium.org/12846011/diff/1/chrome/test/data/extensions/e...
File chrome/test/data/extensions/extension_api_unittest/api_features.json
(right):

https://codereview.chromium.org/12846011/diff/1/chrome/test/data/extensions/e...
chrome/test/data/extensions/extension_api_unittest/api_features.json:21: },
it would also be good to test just ['content_scripts'] here too.

https://codereview.chromium.org/12846011/diff/1/chrome/test/data/extensions/e...
chrome/test/data/extensions/extension_api_unittest/api_features.json:37: }
some more tests:
- matches
- how it interacts with web_page context
- deeper nesting
- a content_script-only API namespace with blessed children.

[cduvall, 2013-03-26 19:24:05]

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_api_features.json:15: "contexts":
["blessed_extension"],
On 2013/03/26 00:34:48, kalman wrote:
> hm, the json has a bug, this should include web_page too. And somewhere we
> should assert that allowing web_page context must specify a matches.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:427: std::string api_name =
GetAPINameFromFullName(feature_name, &child_name);
On 2013/03/26 00:34:48, kalman wrote:
> nit: eyes want a newline after api_name.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:432: return
IsNonFeatureAPIAvailable(api_name, context, extension, url) ?
On 2013/03/26 00:34:48, kalman wrote:
> I think the GetSchema thing should go in IsNonFeatureAPIAvailable. It's an
> implementation detail of that method.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:439: CHECK(feature) << full_name;
On 2013/03/26 00:34:48, kalman wrote:
> yeah, here we're CHECKing anyway.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:463: // in a blessed context.
On 2013/03/26 00:34:48, kalman wrote:
> I don't think this checking dependencies really makes sense anymore, since we
> (well, you) fixed the type dependency problem properly. So this can be 1 line,
> yay.
> 
> return feature->GetContexts()->count(Feature::BLESSED_EXTENSION_CONTEXT) > 0;

Taking out the dependency checks causes ExtensionAPI.IsPrivilegedFeatures to
fail. For example, using the fake api_features.json for the test, if you check
IsPrivileged("test4.bar") should it return true, even though its dependency
"test4.foo" isn't privileged?

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:464: Feature* feature =
GetFeatureDependency(full_name);
On 2013/03/26 00:34:48, kalman wrote:
> and here we could assign Feature up on line 460 and then check it for null.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:480: const DictionaryValue* schema
= GetSchema(api_name);
On 2013/03/26 00:34:48, kalman wrote:
> nice catch.
> 
> // get now to populate |completely_unprivileged_apis_|s.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:610: Feature*
ExtensionAPI::TryGetFeatureDependency(const std::string& full_name) {
On 2013/03/26 00:34:48, kalman wrote:
> I think this is my favourite method. Let's delete UsesFeatureSystem and
> GetFeatureDependency and rename this GetFeatureDependency.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:621: // Try getting the feature
for the parent API, if this was a child.
On 2013/03/26 00:34:48, kalman wrote:
> In the general case I think we need to step all the way up until there is no
> more api name.

GetAPINameFromFullName() guarantees to either return a real API name or an empty
string. There doesn't seem to be any point of stepping all the way up, unless
I'm misunderstanding you.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:106: Feature*
GetFeatureDependency(const std::string& dependency_name);
On 2013/03/26 00:34:48, kalman wrote:
> This doesn't appear to be used outside of this class, so it can be private.

It's used in the unit test.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/base_feature_provider.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/base_feature_provider.cc:76: return
feature->Parse(value);
On 2013/03/26 00:34:48, kalman wrote:
> yeah, this could be
> 
> std::string error = feature->Parse(value);
> if (error != "")
>   LOG(ERROR) << error;
> return error == "";

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/feature.h (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/feature.h:136: virtual std::set<Context>*
GetContexts() = 0;
On 2013/03/26 00:34:48, kalman wrote:
> i guess we're here now, but I kind of prefer keeping all the feature parsing
in
> the one place. remove the todo?

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/simple_feature.cc:228: return true;
On 2013/03/26 00:34:48, kalman wrote:
> i.e. return "" here

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/simple_feature.h (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/simple_feature.h:34: virtual bool Parse(const
DictionaryValue* value);
On 2013/03/26 00:34:48, kalman wrote:
> I think this should return a std::string error rather than everything logging
it
> then returning a bool.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/test/data/extensions/e...
File chrome/test/data/extensions/extension_api_unittest/api_features.json
(right):

https://codereview.chromium.org/12846011/diff/1/chrome/test/data/extensions/e...
chrome/test/data/extensions/extension_api_unittest/api_features.json:21: },
On 2013/03/26 00:34:48, kalman wrote:
> it would also be good to test just ['content_scripts'] here too.

Done.

https://codereview.chromium.org/12846011/diff/1/chrome/test/data/extensions/e...
chrome/test/data/extensions/extension_api_unittest/api_features.json:37: }
On 2013/03/26 00:34:48, kalman wrote:
> some more tests:
> - matches
> - how it interacts with web_page context
> - deeper nesting
> - a content_script-only API namespace with blessed children.

Done.

[not at google - send to devlin, 2013-03-26 21:31:21]

lgtm with some more comments, ping it back if that comment about IsPrivileged is
too confusing (sorry about that) - I also thought of another thing to test but I
forgot, but maybe you can think of it. Maybe it was just ensuring the
corner-case behaviour I described is correct.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:463: // in a blessed context.
On 2013/03/26 19:24:05, cduvall wrote:
> On 2013/03/26 00:34:48, kalman wrote:
> > I don't think this checking dependencies really makes sense anymore, since
we
> > (well, you) fixed the type dependency problem properly. So this can be 1
line,
> > yay.
> > 
> > return feature->GetContexts()->count(Feature::BLESSED_EXTENSION_CONTEXT) >
0;
> 
> For example, using the fake api_features.json for the test, if you check
> IsPrivileged("test4.bar") should it return true, even though its dependency
> "test4.foo" isn't privileged?

I can't find this. Are you referring to privileged_api_features.json? That
doesn't have a test4.bar. The other file with a test4.bar doesn't look valid
since it doesn't have a context specified.

Anyway, possibly answering your question -

Thinking about this some more, checking dependencies introduces ambiguity. What
does it mean if a feature specifies [content_scripts] but a dependency only
specifies [blessed]?  Vice versa? The feature specification doesn't really
attach a hierarchy to these contexts.

I think we should just enforce that all API dependencies include contexts, then
IsPrivileged doesn't get involved with dependencies. GetAvailability obviously
does, though.

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:621: // Try getting the feature
for the parent API, if this was a child.
On 2013/03/26 19:24:05, cduvall wrote:
> On 2013/03/26 00:34:48, kalman wrote:
> > In the general case I think we need to step all the way up until there is no
> > more api name.
> 
> GetAPINameFromFullName() guarantees to either return a real API name or an
empty
> string. There doesn't seem to be any point of stepping all the way up, unless
> I'm misunderstanding you.

Oh ok, never mind.

https://codereview.chromium.org/12846011/diff/20002/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

https://codereview.chromium.org/12846011/diff/20002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:183:
/*scoped_refptr<Extension> CreateExtensionWithType(Manifest::Type type) {
hm?

https://codereview.chromium.org/12846011/diff/20002/chrome/common/extensions/...
File chrome/common/extensions/features/api_feature.cc (right):

https://codereview.chromium.org/12846011/diff/20002/chrome/common/extensions/...
chrome/common/extensions/features/api_feature.cc:21: return name() + ": API
features must specify at least one context.";
oh - we do enforce the context thing.

[cduvall, 2013-03-26 22:08:48]

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12846011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.cc:463: // in a blessed context.
On 2013/03/26 21:31:21, kalman wrote:
> On 2013/03/26 19:24:05, cduvall wrote:
> > On 2013/03/26 00:34:48, kalman wrote:
> > > I don't think this checking dependencies really makes sense anymore, since
> we
> > > (well, you) fixed the type dependency problem properly. So this can be 1
> line,
> > > yay.
> > > 
> > > return feature->GetContexts()->count(Feature::BLESSED_EXTENSION_CONTEXT) >
> 0;
> > 
> > For example, using the fake api_features.json for the test, if you check
> > IsPrivileged("test4.bar") should it return true, even though its dependency
> > "test4.foo" isn't privileged?
> 
> I can't find this. Are you referring to privileged_api_features.json? That
> doesn't have a test4.bar. The other file with a test4.bar doesn't look valid
> since it doesn't have a context specified.
> 
> Anyway, possibly answering your question -
> 
> Thinking about this some more, checking dependencies introduces ambiguity.
What
> does it mean if a feature specifies [content_scripts] but a dependency only
> specifies [blessed]?  Vice versa? The feature specification doesn't really
> attach a hierarchy to these contexts.
> 
> I think we should just enforce that all API dependencies include contexts,
then
> IsPrivileged doesn't get involved with dependencies. GetAvailability obviously
> does, though.

Ok, I removed dependency checking from IsPrivileged(), and tested to make sure
dependencies aren't checked.

https://codereview.chromium.org/12846011/diff/20002/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

https://codereview.chromium.org/12846011/diff/20002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:183:
/*scoped_refptr<Extension> CreateExtensionWithType(Manifest::Type type) {
On 2013/03/26 21:31:21, kalman wrote:
> hm?

Oops

[commit-bot: I haz the power, 2013-03-26 22:10:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12846011/10006

[commit-bot: I haz the power, 2013-03-26 22:10:51]

Presubmit check for 12846011-10006 failed and returned exit status 1.

INFO:root:Found 21 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/common/common_resources.grd
    chrome/chrome_common.gypi

Presubmit checks took 7.8s to calculate.

[commit-bot: I haz the power, 2013-03-26 22:30:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12846011/10006

[commit-bot: I haz the power, 2013-03-26 23:01:11]

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.

[commit-bot: I haz the power, 2013-03-26 23:11:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12846011/38001

[commit-bot: I haz the power, 2013-03-27 06:53:50]

Change committed as 190836

[commit-bot: I haz the power, 2013-03-27 17:33:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12846011/59001

[commit-bot: I haz the power, 2013-03-27 20:17:31]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-27 20:22:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12846011/59001

[commit-bot: I haz the power, 2013-03-28 12:40:27]

Change committed as 191133