[Extensions] Add a SimpleFeature::Validate function

extensions/common/features/simple_feature.cc

Index: extensions/common/features/simple_feature.cc
diff --git a/extensions/common/features/simple_feature.cc b/extensions/common/features/simple_feature.cc
index b4a39198bcd62498e6f5c0ab5ee210556a1b7227..f449e8f7bf1b2ccf8078d25cb6ab9ee81b80211e 100644
--- a/extensions/common/features/simple_feature.cc
+++ b/extensions/common/features/simple_feature.cc
@@ -310,11 +310,12 @@ SimpleFeature::SimpleFeature()
     : location_(UNSPECIFIED_LOCATION),
       min_manifest_version_(0),
       max_manifest_version_(0),
-      component_extensions_auto_granted_(true) {}
+      component_extensions_auto_granted_(true),
+      internal_(false) {}

[lazyboy, 2016/07/15 22:53:40]

This is exposing "internal" field to all features from APIFeatures. Not sure if
that's a good idea, couldn't we make Parse virtual and override it in
APIFeatures? That way only APIFeatures::Parse would look into "internal" field.

On the flip side, SimpleFeature::IsInternal exists, so maybe a non-issue.

[Devlin, 2016/07/18 17:51:58]

It is, but there's nothing to say that only api features can be internal (other
features could easily be, too).  And since we already have the IsInternal()
function exposed, I don't see any harm.  We could restrict it to only api
features, but it'll be a little messy right now (once we have these compiled,
it'd probably be a bit cleaner).

[lazyboy, 2016/07/18 17:57:43]

Acknowledged.

 
 SimpleFeature::~SimpleFeature() {}
 
-std::string SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
+void SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
   static base::LazyInstance<SimpleFeature::Mappings> mappings =
       LAZY_INSTANCE_INITIALIZER;
 
@@ -358,6 +359,8 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
       channel_.reset(new version_info::Channel(version_info::Channel::UNKNOWN));
       ParseEnum<version_info::Channel>(value, channel_.get(),
                                        mappings.Get().channels);
+    } else if (key == "internal") {
+      value->GetAsBoolean(&internal_);
     }
   }
 
@@ -369,13 +372,18 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
   // and "matches" google.com/*. Then a sub-feature "foo.bar" might override
   // "matches" to be chromium.org/*. That sub-feature doesn't need to specify
   // "web_page" context because it's inherited, but we don't know that here.
+}
 
+bool SimpleFeature::Validate(std::string* error) {
+  DCHECK(error);
   // All features must be channel-restricted, either directly or through
   // dependents.
-  if (!channel_ && dependencies_.empty())
-    return name() + ": Must supply a value for channel or dependencies.";
+  if (!channel_ && dependencies_.empty()) {
+    *error = name() + ": Must supply a value for channel or dependencies.";
+    return false;
+  }
 
-  return std::string();
+  return true;
 }
 
 Feature::Availability SimpleFeature::IsAvailableToManifest(
@@ -583,7 +591,7 @@ Feature::Availability SimpleFeature::CreateAvailability(
 }
 
 bool SimpleFeature::IsInternal() const {
-  return false;
+  return internal_;
 }
 
 bool SimpleFeature::IsIdInBlacklist(const std::string& extension_id) const {