Implement FeatureProvider for ExtensionAPI.

Implement FeatureProvider for ExtensionAPI.

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

[not at google - send to devlin, 2012-04-01 09:14:10]

Nice.

Redirecting to koz though, I'm on leave this week.

[koz (OOO until 15th September), 2012-04-02 06:39:48]

Awesome change! lgtm

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.cc (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:430: std::string api_name =
GetAPINameForFullName(full_name, &child_name);
GetAPINameForFullName() -> GetAPINameFromFullName()?

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:440: // An API is 'privileged' if
any of its dependencies requires a blessed
Would this comment be more accurately written as "...any of its dependencies are
available in a blessed extension context."?

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:458: // If this API has been
converted yet, fall back to the old system.
has -> hasn't

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:573: LoadAllSchemas();
Could you add a comment explaining why we need to load all the schemas before
executing this code?

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:578: APIFeatureMap::iterator
feature_map = features_.find(api_namespace);
feature_map -> api_features?

I think that is more consistent with the naming of the features_ field, which is
a map that represents a set of named values.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:638: // This API is controlled by
the feature system. Nothing to do here.
Could you add a comment to the declaration of this function saying that it only
returns APIs that aren't controlled by the feature system?

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:710: // This API is controlled by
the feature system. Nothing to do.
Same comment as above - this should probably be mentioned in the declaration
comment.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.h:90: // Gets a Feature object
describing the API with the specified full_name. This
full_name -> |full_name|

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api_unittest.cc:282: TEST(ExtensionAPI,
Features) {
Features -> TestFeatureProperties?

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/feature.h
File chrome/common/extensions/feature.h (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/featur...
chrome/common/extensions/feature.h:103: // us to implement inheritance by
parsing one value after another.
Cool.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/featur...
chrome/common/extensions/feature.h:124: // Returns true if the feature is
available to used in the specified extension
to used in -> to be used in

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/featur...
File chrome/common/extensions/feature_unittest.cc (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/featur...
chrome/common/extensions/feature_unittest.cc:349:
EXPECT_TRUE(feature2.Equals(feature));
Could you add a test for what happens when a dictionary specifies a value that
the feature already has?

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/simple...
File chrome/common/extensions/simple_feature_provider.h (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/simple...
chrome/common/extensions/simple_feature_provider.h:25: // provider create plain
old Factory instances.
plain old Factory instances -> plain old Feature instances

[Aaron Boodman, 2012-04-02 08:21:54]

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.cc (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:430: std::string api_name =
GetAPINameForFullName(full_name, &child_name);
On 2012/04/02 06:39:48, koz wrote:
> GetAPINameForFullName() -> GetAPINameFromFullName()?

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:440: // An API is 'privileged' if
any of its dependencies requires a blessed
On 2012/04/02 06:39:48, koz wrote:
> Would this comment be more accurately written as "...any of its dependencies
are
> available in a blessed extension context."?

Actually the logic was slightly wrong :/. The unit tests would have caught it
when I converted more of the data files over. Fixed.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:458: // If this API has been
converted yet, fall back to the old system.
On 2012/04/02 06:39:48, koz wrote:
> has -> hasn't

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:573: LoadAllSchemas();
On 2012/04/02 06:39:48, koz wrote:
> Could you add a comment explaining why we need to load all the schemas before
> executing this code?

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:578: APIFeatureMap::iterator
feature_map = features_.find(api_namespace);
On 2012/04/02 06:39:48, koz wrote:
> feature_map -> api_features?
> 
> I think that is more consistent with the naming of the features_ field, which
is
> a map that represents a set of named values.

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:638: // This API is controlled by
the feature system. Nothing to do here.
On 2012/04/02 06:39:48, koz wrote:
> Could you add a comment to the declaration of this function saying that it
only
> returns APIs that aren't controlled by the feature system?

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.cc:710: // This API is controlled by
the feature system. Nothing to do.
On 2012/04/02 06:39:48, koz wrote:
> Same comment as above - this should probably be mentioned in the declaration
> comment.

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.h (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.h:90: // Gets a Feature object
describing the API with the specified full_name. This
On 2012/04/02 06:39:48, koz wrote:
> full_name -> |full_name|

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api_unittest.cc (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api_unittest.cc:282: TEST(ExtensionAPI,
Features) {
On 2012/04/02 06:39:48, koz wrote:
> Features -> TestFeatureProperties?

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/feature.h
File chrome/common/extensions/feature.h (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/featur...
chrome/common/extensions/feature.h:124: // Returns true if the feature is
available to used in the specified extension
On 2012/04/02 06:39:48, koz wrote:
> to used in -> to be used in

Done.

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/featur...
File chrome/common/extensions/feature_unittest.cc (right):

http://codereview.chromium.org/9950046/diff/1/chrome/common/extensions/featur...
chrome/common/extensions/feature_unittest.cc:349:
EXPECT_TRUE(feature2.Equals(feature));
On 2012/04/02 06:39:48, koz wrote:
> Could you add a test for what happens when a dictionary specifies a value that
> the feature already has?

Done.

[commit-bot: I haz the power, 2012-04-03 20:52:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/9950046/4009

[commit-bot: I haz the power, 2012-04-03 20:52:46]

Presubmit check for 9950046-4009 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/common/extensions/api/extension_api_unittest.cc, line 279, 81 chars

Presubmit checks took 3.0s to calculate.

[commit-bot: I haz the power, 2012-04-03 21:24:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/9950046/6009