Implement ComplexFeature to support permission features with multiple rules.

Implement ComplexFeature to support permission features with multiple rules.

This moves the existing Feature class functionality into SimpleFeature and makes Feature an abstract class. ComplexFeature implements Feature and returns the availability of the Features that compose it.

Also move SimpleFeatureProvider to BaseFeatureProvider since they can return either Simple or Complex features now.

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

[justinlin, 2012-11-26 12:33:14]

Hi Aaron, PTAL. Tried to preserve the existing functionality while extending to
allow multiple rules, so there is a few TODO's left to move everything over to
be able to support multiple permission rules for features, and a few things to
maybe think about (i.e. error messages for a feature with multiple rules (it
will just use the error for the first rule, if the feature is not available
right now)).

[justinlin, 2012-11-28 00:58:56]

On 2012/11/26 12:33:14, justinlin wrote:
> Hi Aaron, PTAL. Tried to preserve the existing functionality while extending
to
> allow multiple rules, so there is a few TODO's left to move everything over to
> be able to support multiple permission rules for features, and a few things to
> maybe think about (i.e. error messages for a feature with multiple rules (it
> will just use the error for the first rule, if the feature is not available
> right now)).

ping

[not at google - send to devlin, 2012-11-28 01:02:58]

On 2012/11/28 00:58:56, justinlin wrote:
> On 2012/11/26 12:33:14, justinlin wrote:
> > Hi Aaron, PTAL. Tried to preserve the existing functionality while extending
> to
> > allow multiple rules, so there is a few TODO's left to move everything over
to
> > be able to support multiple permission rules for features, and a few things
to
> > maybe think about (i.e. error messages for a feature with multiple rules (it
> > will just use the error for the first rule, if the feature is not available
> > right now)).
> 
> ping

That wasn't the correct address for Aaron, and this is the first time I've seen
this code review.

I'm happy to review it though Aaron is out until Thursday, and I want to give
him a chance first.

[justinlin, 2012-11-28 01:05:14]

On 2012/11/28 01:02:58, kalman wrote:
> On 2012/11/28 00:58:56, justinlin wrote:
> > On 2012/11/26 12:33:14, justinlin wrote:
> > > Hi Aaron, PTAL. Tried to preserve the existing functionality while
extending
> > to
> > > allow multiple rules, so there is a few TODO's left to move everything
over
> to
> > > be able to support multiple permission rules for features, and a few
things
> to
> > > maybe think about (i.e. error messages for a feature with multiple rules
(it
> > > will just use the error for the first rule, if the feature is not
available
> > > right now)).
> > 
> > ping
> 
> That wasn't the correct address for Aaron, and this is the first time I've
seen
> this code review.
> 
> I'm happy to review it though Aaron is out until Thursday, and I want to give
> him a chance first.

Oh, thanks for correcting that! Ahh.. aa.chromium.org@gmail.com !=
aa@chromium.org, fooled me.

Sure, but if you're familiar with this section of the code, it'd be nice to get
a first review too, but no worries if you wanted to wait until Aaron had a look.

[not at google - send to devlin, 2012-11-30 00:48:57]

I chatted with Aaron, and I'll take this.

I think the approach we want is as follows:
1. Make Feature an interface with just IsAvailableToManifest and
IsAvailableToContext.
2. The current Feature implementation becomes SimpleFeature (given out by
SimpleFeatureProvider)
3. Write a new implementation ComplexFeature and a new FeatureProvider called
ComplexFeatureProvider

[not at google - send to devlin, 2012-11-30 01:07:55]

Oh no, I meant that I'll take the review :)

[justinlin, 2012-12-12 11:07:01]

On 2012/11/30 01:07:55, kalman wrote:
> Oh no, I meant that I'll take the review :)

Hi kalman@, please take a look. This patch looks big, but it's mostly just due
to the Feature->SimpleFeature move :), there isn't that much new code. Thanks.

[not at google - send to devlin, 2012-12-12 17:42:41]

sweet change

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

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:289: SimpleFeature* feature = new
SimpleFeature();
s/SimpleFeature/Feature/ in the LHS of expressions in this file

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

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.h:98: virtual SimpleFeature*
GetFeature(const std::string& full_name) OVERRIDE;
should return a Feature?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.h:177: typedef std::map<std::string,
linked_ptr<SimpleFeature> > FeatureMap;
Feature?

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

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:21: using
extensions::Feature;
this file is in the extensions namespace, this shouldn't be necessary

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:32: virtual
SimpleFeature* GetFeature(const std::string& name) OVERRIDE {
Feature etc

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:738: ComplexFeatureProvider*
permission_features =
FeatureProvider on LHS

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:20:
DCHECK_GT(simple_features_.size(), 0UL);
hopefully you can pass these in in the constructor, then you can make this CHECK
once there

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:27: if
(!first_availability.is_available()) {
nit: early exit here

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:52: if
(!first_availability.is_available()) {
nit: early exit

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:72: return
simple_features_[0].GetAvailabilityMessage(result, type);
it would be nice for all of these to actually form an availability and message
out of all its features, but that's a bit of extra work and I'm happy to leave
it as a TODO for now.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:18: // only support
PermissionFeatures. A ComplexFeature is available if any
"we only support PermissionFeatures" -> is there any reason that needs to be
true?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:26: // Adds a SimpleFeature
to the list that composes this ComplexFeature.
SimpleFeature -> Feature. In any case, is it possible to replace this method and
pass the set of Features in via the constructor?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:27: void
AddSimpleFeature(const PermissionFeature* feature);
PermissionFeature -> Feature, take a scoped_ptr<Feature>

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:43: typedef
std::vector<PermissionFeature> SimpleFeatureList;
PermissionFeature -> Feature

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:44: SimpleFeatureList
simple_features_;
DISALLOW_COPY_AND_ASSIGN

This might be an interesting change actually, it seems like Features are
currently copyable. Hopefully it's not a big web to untangle. But - a start here
would be to make this a ScopedVector<Feature>.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:45: return
scoped_ptr<ComplexFeatureProvider>(
you can use make_scoped_ptr here and avoid template arguments

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:56: if
(iter.value().GetType() != Value::TYPE_LIST) {
It think it'd be simpler to have a single point for some feature provider, not
try to spread out the load between a SimpleFeatureProvider version of the
permission features and this complexfeatureprovider version. I don't see a
reason why ComplexFeatureProvider shouldn't know how to create SimpleFeatures
too?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:60:
linked_ptr<ComplexFeature> feature(new ComplexFeature());
delay construction of this and collect simplefeatures in a vector rather than
using AddSimpleFeature - that way ComplexFeature can be created with its
features as a constant.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:65: continue;
I'm not sure it makes sense to have an empty list. Perhaps DCHECK this?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:82: continue;
does SimpleFeature not do this validation itself?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:89: }
why is that? Btw make this DCHECK so that we notice it during tests.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:125: return
SimpleFeatureProvider::GetPermissionFeatures()->GetFeature(name);
See comment about about not spreading load

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.h:21: //
PermissionFeatures) in addition to supporting the existing functionality from
why only PermissionFeatures?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.h:22: //
SimpleFeatureProvider.
"is an extension of SimpleFeatureProvider" -> well it doesn't actually extend
that so it's a slightly misleading comment, also it's an implementation detail.

I'd say more like, ComplexFeatureProvider provides support for ComplexFeatures
(i.e. specifying multiple rules for Features). It can also parse SimpleFeatures.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.h:25: explicit
ComplexFeatureProvider(DictionaryValue* root);
Does this take ownership? In which case pass scoped_ptr<DictionaryValue>.
Otherwise pass const DictionaryValue&

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.h:30: static
ComplexFeatureProvider* GetPermissionFeatures();
hm I find it a bit confusing that users need to know to get the permission
features from here, and the other features from SimpleFeatureProvider. I guess
that's where all the comments about only supporting PermissionFeature comes
from.

Consider moving this method to a new class PermissionFeatureProvider with a
method GetInstance, likewise for ManifestFeatureProvider. They can just return
FeatureProvider instances where internally they construct
ComplexFeatureProviders.

I note that manifest.h refers to a ManifestFeatureProvider, perhaps this is what
aa had in mind.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider_unittest.cc
(right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider_unittest.cc:16:
ListValue* feature1 = new ListValue();
note: handy classes for these kind of tests is the ValueBuilder classes
(chrome/common/extensions/value_builder.h)

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_unittest.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_unittest.cc:41: 
ditto

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/manifest_feature.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/manifest_feature.h:12: class ManifestFeature :
public SimpleFeature {
now that Feature is an interface, can we make this compose with a Feature rather
than extending it?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/permission_feature.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/permission_feature.h:12: class
PermissionFeature : public SimpleFeature {
ditto, compose with feature?

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.cc:74: DVLOG(2) <<
iter.key() << ": Feature is a complex feature.";
I would hope this, by design, can never happen and would indicate a bug in our
C++ code rather than the feature files.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.cc:90: continue;
ah I see, this validation is here. It would be nice not to have to repeat it.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/manifest.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/manifest.cc:14: #include
"chrome/common/extensions/features/complex_feature_provider.h"
unused

[justinlin, 2012-12-14 12:26:26]

Hey kalman@, thanks for the review!

Addressed most of the small comments. Some high level changes/comments:
- ComplexFeatureProvider is gone, we just create them inside
SimpleFeatureProvider now (maybe it should be renamed? BaseFeatureProvider?).
- There are places where the LHS needs to be SimpleFeature* because we're in a
place where we're creating the SimpleFeature (or in a test) and thus need to
access some of those non-interface getters/accessors.
- I'm not sure what you meant by making {Manifest,Permission}Feature compose
with Feature, so didn't do that (yet).

PTAL, thanks!

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

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:289: SimpleFeature* feature = new
SimpleFeature();
On 2012/12/12 17:42:41, kalman wrote:
> s/SimpleFeature/Feature/ in the LHS of expressions in this file

Done, except for this instance in the file. In this case, we're creating a
SimpleFeature explicitly, so we want a SimpleFeature pointer.

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

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.h:98: virtual SimpleFeature*
GetFeature(const std::string& full_name) OVERRIDE;
On 2012/12/12 17:42:41, kalman wrote:
> should return a Feature?

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.h:177: typedef std::map<std::string,
linked_ptr<SimpleFeature> > FeatureMap;
On 2012/12/12 17:42:41, kalman wrote:
> Feature?

Done.

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

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:21: using
extensions::Feature;
On 2012/12/12 17:42:41, kalman wrote:
> this file is in the extensions namespace, this shouldn't be necessary

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api_unittest.cc:32: virtual
SimpleFeature* GetFeature(const std::string& name) OVERRIDE {
On 2012/12/12 17:42:41, kalman wrote:
> Feature etc

I changed the method signature, but I think the others should stay
SimpleFeature. Otherwise, I'd have to expose a lot of accessors/mutators in
Feature, which I don't think makes sense.

This unit test file is really just to test SimpleFeature anyway.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:738: ComplexFeatureProvider*
permission_features =
On 2012/12/12 17:42:41, kalman wrote:
> FeatureProvider on LHS

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:20:
DCHECK_GT(simple_features_.size(), 0UL);
On 2012/12/12 17:42:41, kalman wrote:
> hopefully you can pass these in in the constructor, then you can make this
CHECK
> once there

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:27: if
(!first_availability.is_available()) {
On 2012/12/12 17:42:41, kalman wrote:
> nit: early exit here

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:52: if
(!first_availability.is_available()) {
On 2012/12/12 17:42:41, kalman wrote:
> nit: early exit

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:72: return
simple_features_[0].GetAvailabilityMessage(result, type);
On 2012/12/12 17:42:41, kalman wrote:
> it would be nice for all of these to actually form an availability and message
> out of all its features, but that's a bit of extra work and I'm happy to leave
> it as a TODO for now.

Yea, I think this requires some thought. The UI in about:extensions would also
need to be updated somewhat to show that the missing requirements are ORs (i.e.
say we wanted to whitelist sockets for only some extensions, then it's (either
(is_extension and whitelisted) OR (is_app))). Added TODO.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:18: // only support
PermissionFeatures. A ComplexFeature is available if any
On 2012/12/12 17:42:41, kalman wrote:
> "we only support PermissionFeatures" -> is there any reason that needs to be
> true?

Done. Removed.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:26: // Adds a SimpleFeature
to the list that composes this ComplexFeature.
On 2012/12/12 17:42:41, kalman wrote:
> SimpleFeature -> Feature. In any case, is it possible to replace this method
and
> pass the set of Features in via the constructor?

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:27: void
AddSimpleFeature(const PermissionFeature* feature);
On 2012/12/12 17:42:41, kalman wrote:
> PermissionFeature -> Feature, take a scoped_ptr<Feature>

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:43: typedef
std::vector<PermissionFeature> SimpleFeatureList;
On 2012/12/12 17:42:41, kalman wrote:
> PermissionFeature -> Feature

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:44: SimpleFeatureList
simple_features_;
On 2012/12/12 17:42:41, kalman wrote:
> DISALLOW_COPY_AND_ASSIGN
> 
> This might be an interesting change actually, it seems like Features are
> currently copyable. Hopefully it's not a big web to untangle. But - a start
here
> would be to make this a ScopedVector<Feature>.

Done. Since we pretty much never reference ComplexFeature directly, this is easy
:).

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:45: return
scoped_ptr<ComplexFeatureProvider>(
On 2012/12/12 17:42:41, kalman wrote:
> you can use make_scoped_ptr here and avoid template arguments

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:56: if
(iter.value().GetType() != Value::TYPE_LIST) {
On 2012/12/12 17:42:41, kalman wrote:
> It think it'd be simpler to have a single point for some feature provider, not
> try to spread out the load between a SimpleFeatureProvider version of the
> permission features and this complexfeatureprovider version. I don't see a
> reason why ComplexFeatureProvider shouldn't know how to create SimpleFeatures
> too?

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:60:
linked_ptr<ComplexFeature> feature(new ComplexFeature());
On 2012/12/12 17:42:41, kalman wrote:
> delay construction of this and collect simplefeatures in a vector rather than
> using AddSimpleFeature - that way ComplexFeature can be created with its
> features as a constant.

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:65: continue;
On 2012/12/12 17:42:41, kalman wrote:
> I'm not sure it makes sense to have an empty list. Perhaps DCHECK this?

This is from "file input" though. Do we want to crash if the
{permission,manifest}_features_.json is malformed? Added a LOG(ERROR) for now.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:82: continue;
On 2012/12/12 17:42:41, kalman wrote:
> does SimpleFeature not do this validation itself?

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:89: }
On 2012/12/12 17:42:41, kalman wrote:
> why is that? Btw make this DCHECK so that we notice it during tests.

Hmm.. I'm not sure if I want to attempt to move this check into SimpleFeature in
this CL. There might have been a valid reason (or maybe not). I see
SimpleFeatures created elsewhere, so maybe those instances don't require these
checks.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:125: return
SimpleFeatureProvider::GetPermissionFeatures()->GetFeature(name);
On 2012/12/12 17:42:41, kalman wrote:
> See comment about about not spreading load

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.h:25: explicit
ComplexFeatureProvider(DictionaryValue* root);
On 2012/12/12 17:42:41, kalman wrote:
> Does this take ownership? In which case pass scoped_ptr<DictionaryValue>.
> Otherwise pass const DictionaryValue&

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.h:30: static
ComplexFeatureProvider* GetPermissionFeatures();
On 2012/12/12 17:42:41, kalman wrote:
> hm I find it a bit confusing that users need to know to get the permission
> features from here, and the other features from SimpleFeatureProvider. I guess
> that's where all the comments about only supporting PermissionFeature comes
> from.
> 
> Consider moving this method to a new class PermissionFeatureProvider with a
> method GetInstance, likewise for ManifestFeatureProvider. They can just return
> FeatureProvider instances where internally they construct
> ComplexFeatureProviders.
> 
> I note that manifest.h refers to a ManifestFeatureProvider, perhaps this is
what
> aa had in mind.

Yea, it gets kind of complicated with a bunch of classes. I moved everything
into SimpleFeatureProvider. That should be the unified way to get
Manifest/Permission features. It should probably be renamed though.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider_unittest.cc
(right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider_unittest.cc:16:
ListValue* feature1 = new ListValue();
On 2012/12/12 17:42:41, kalman wrote:
> note: handy classes for these kind of tests is the ValueBuilder classes
> (chrome/common/extensions/value_builder.h)

Done. Awesome, thanks!

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_unittest.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_unittest.cc:41: 
On 2012/12/12 17:42:41, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/manifest_feature.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/manifest_feature.h:12: class ManifestFeature :
public SimpleFeature {
On 2012/12/12 17:42:41, kalman wrote:
> now that Feature is an interface, can we make this compose with a Feature
rather
> than extending it?

Hmm.. not sure how you mean by this? I thought about it a bit, but can't come up
with a way that this would make things nicer yet.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/permission_feature.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/permission_feature.h:12: class
PermissionFeature : public SimpleFeature {
On 2012/12/12 17:42:41, kalman wrote:
> ditto, compose with feature?

Same..

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.cc:74: DVLOG(2) <<
iter.key() << ": Feature is a complex feature.";
On 2012/12/12 17:42:41, kalman wrote:
> I would hope this, by design, can never happen and would indicate a bug in our
> C++ code rather than the feature files.

Updated.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.cc:90: continue;
On 2012/12/12 17:42:41, kalman wrote:
> ah I see, this validation is here. It would be nice not to have to repeat it.

Done.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/manifest.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/manifest.cc:14: #include
"chrome/common/extensions/features/complex_feature_provider.h"
On 2012/12/12 17:42:41, kalman wrote:
> unused

Done.

[not at google - send to devlin, 2012-12-14 19:10:42]

almost there!

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:65: continue;
On 2012/12/14 12:26:26, justinlin wrote:
> On 2012/12/12 17:42:41, kalman wrote:
> > I'm not sure it makes sense to have an empty list. Perhaps DCHECK this?
> 
> This is from "file input" though. Do we want to crash if the
> {permission,manifest}_features_.json is malformed? Added a LOG(ERROR) for now.

Those files being malformed implies a bug in Chrome, and (D)CHECKing for it
increases our chance for finding it. So yes we should crash.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/manifest_feature.h (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/manifest_feature.h:12: class ManifestFeature :
public SimpleFeature {
On 2012/12/14 12:26:26, justinlin wrote:
> On 2012/12/12 17:42:41, kalman wrote:
> > now that Feature is an interface, can we make this compose with a Feature
> rather
> > than extending it?
> 
> Hmm.. not sure how you mean by this? I thought about it a bit, but can't come
up
> with a way that this would make things nicer yet.

Something like this: https://codereview.chromium.org/11568034/

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.cc (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:13:
features_.swap(*features.get());
just *features is fine

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:27: }
no {} for single-line if blocks.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:47: }
ditto

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.h (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:18: // A ComplexFeature is
composed of one or many SimpleFeatures. A ComplexFeature
s/SimpleFeatures/Features/

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:23: typedef
std::vector<SimpleFeature*> FeatureList;
std::vector<Feature*> (actually, ScopedVector<SimpleFeature>)

there is no reason why ComplexFeature only supports SimpleFeatures, it can
support any type of Feature.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/feature.h (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:86: Feature() {}
If the constructor is just this then it can be left out

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:87: virtual ~Feature() {}
IIRC virtual constructors in non-pure-virtual classes shouldn't be inlined, and
there are those CreateAvailability methods. If you can make those pure-virtual
(or move them someplace else) then that'd be cool, but nbd if not. I just don't
think this can be inline-virtual.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:126: virtual std::set<Context>*
contexts() = 0;
Generally speaking you can't guarantee that virtual methods have trivial
implementations, which is what calling this contexts() rather than GetContexts()
implies. One reasonable exception is for when things are virtual for testing.

But stepping back: why does this need to be on the interface for Feature?

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.cc:76: return false;
if these DCHECKed then this method can't fail, and logic gets simpler

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature_provider.h (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.h:20: class
SimpleFeatureProvider : public FeatureProvider {
yes please, rename to (Default|Base|Standard)FeatureProvider or something.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.h:22: typedef
SimpleFeature*(*FeatureFactory)();
Feature*(*FeatureFactory) not SimpleFeature*(*FeatureFactory)

[justinlin, 2012-12-14 21:07:52]

thanks, ptal.

I tried to do the manifest/permissionFeatures change to use composition, but it
gets weird inside SimpleFeatureProvider:
https://codereview.chromium.org/11585003/

The main problem is we end up needing a SimpleFeature(Feature* feature)
constructor due to the STL factory stuff, which ends up not making much sense.
Do you think that's still a needed change or can I leave as is? Thanks.

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/14001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature_provider.cc:65: continue;
On 2012/12/14 19:10:42, kalman wrote:
> On 2012/12/14 12:26:26, justinlin wrote:
> > On 2012/12/12 17:42:41, kalman wrote:
> > > I'm not sure it makes sense to have an empty list. Perhaps DCHECK this?
> > 
> > This is from "file input" though. Do we want to crash if the
> > {permission,manifest}_features_.json is malformed? Added a LOG(ERROR) for
now.
> 
> Those files being malformed implies a bug in Chrome, and (D)CHECKing for it
> increases our chance for finding it. So yes we should crash.

Done. OK, added CHECK. If users aren't supposed to be able to modify it, then
makes sense.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.cc (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:13:
features_.swap(*features.get());
On 2012/12/14 19:10:42, kalman wrote:
> just *features is fine

Done.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:27: }
On 2012/12/14 19:10:42, kalman wrote:
> no {} for single-line if blocks.

Done.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:47: }
On 2012/12/14 19:10:42, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.h (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:18: // A ComplexFeature is
composed of one or many SimpleFeatures. A ComplexFeature
On 2012/12/14 19:10:42, kalman wrote:
> s/SimpleFeatures/Features/

Done.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.h:23: typedef
std::vector<SimpleFeature*> FeatureList;
On 2012/12/14 19:10:42, kalman wrote:
> std::vector<Feature*> (actually, ScopedVector<SimpleFeature>)
> 
> there is no reason why ComplexFeature only supports SimpleFeatures, it can
> support any type of Feature.

The "reason" was that GetAvailabilityMessage was a protected member, so
ComplexFeature couldn't call SimpleFeature::GetAvailabilityMessage. Looking at
it again, I guess it's OK to make that a public method of Feature now.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/feature.h (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:86: Feature() {}
On 2012/12/14 19:10:42, kalman wrote:
> If the constructor is just this then it can be left out

Done.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:87: virtual ~Feature() {}
On 2012/12/14 19:10:42, kalman wrote:
> IIRC virtual constructors in non-pure-virtual classes shouldn't be inlined,
and
> there are those CreateAvailability methods. If you can make those pure-virtual
> (or move them someplace else) then that'd be cool, but nbd if not. I just
don't
> think this can be inline-virtual.

Done. Right, put it in the cc for now.
I moved the CreateAvailability functions to SimpleFeature, maybe that makes more
sense since they're more like helpers.

The get/set_name are in this class, so it won't be pure virtual anyway.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:126: virtual std::set<Context>*
contexts() = 0;
On 2012/12/14 19:10:42, kalman wrote:
> Generally speaking you can't guarantee that virtual methods have trivial
> implementations, which is what calling this contexts() rather than
GetContexts()
> implies. One reasonable exception is for when things are virtual for testing.
> 
> But stepping back: why does this need to be on the interface for Feature?

extension_api.cc needs this method in a couple places (3) where it doesn't look
like it's safe to case to SimpleFeature. Renamed. I think this maybe can be
removed eventually by somebody who fully understands the logic in extension_api.
I didn't want to change it and risk breaking something yet.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature_provider.cc (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.cc:76: return false;
On 2012/12/14 19:10:42, kalman wrote:
> if these DCHECKed then this method can't fail, and logic gets simpler

Done. Oops, had to undo. Tests do things like this.. so we have to skip.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature_provider.h (right):

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.h:20: class
SimpleFeatureProvider : public FeatureProvider {
On 2012/12/14 19:10:42, kalman wrote:
> yes please, rename to (Default|Base|Standard)FeatureProvider or something.

OK, next patch :) Probably going with BaseFeatureProvider.

https://codereview.chromium.org/11316164/diff/23006/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature_provider.h:22: typedef
SimpleFeature*(*FeatureFactory)();
On 2012/12/14 19:10:42, kalman wrote:
> Feature*(*FeatureFactory) not SimpleFeature*(*FeatureFactory)

Problem with this is that in the .cc file, we'd have to do more casting because
the Parse() methods are on the SimpleFeature class, not Feature.

[not at google - send to devlin, 2012-12-14 21:30:43]

lgtm, just need to rename simple_feature_provider

https://codereview.chromium.org/11316164/diff/31001/chrome/common/extensions/...
File chrome/common/extensions/features/feature.h (right):

https://codereview.chromium.org/11316164/diff/31001/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:125: virtual std::set<Context>*
GetContexts() = 0;
TODO(justinlin): move to APIFeature when it exists.

[not at google - send to devlin, 2012-12-14 21:30:43]

lgtm, just need to rename simple_feature_provider

https://codereview.chromium.org/11316164/diff/31001/chrome/common/extensions/...
File chrome/common/extensions/features/feature.h (right):

https://codereview.chromium.org/11316164/diff/31001/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:125: virtual std::set<Context>*
GetContexts() = 0;
TODO(justinlin): move to APIFeature when it exists.

[justinlin, 2012-12-14 21:56:06]

Done the rename SimpleFeatureProvider -> BaseFeatureProvider.

sky@: Could I get your owner stamp for the gypi changes? Thanks.

https://codereview.chromium.org/11316164/diff/31001/chrome/common/extensions/...
File chrome/common/extensions/features/feature.h (right):

https://codereview.chromium.org/11316164/diff/31001/chrome/common/extensions/...
chrome/common/extensions/features/feature.h:125: virtual std::set<Context>*
GetContexts() = 0;
On 2012/12/14 21:30:43, kalman wrote:
> TODO(justinlin): move to APIFeature when it exists.

Done.

[Lei Zhang, 2012-12-14 22:53:44]

lgtm for the gypi changes.

[commit-bot: I haz the power, 2012-12-14 23:18:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/11316164/34001

[commit-bot: I haz the power, 2012-12-15 03:45:21]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 03:50:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/11316164/34001

[commit-bot: I haz the power, 2012-12-15 09:30:08]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 20:03:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/11316164/34001