[Extensions] Add a SimpleFeature::Validate function

extensions/common/features/simple_feature.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/common/features/simple_feature.h"
 
 #include <algorithm>
 #include <map>
 #include <utility>
 #include <vector>
@@ skipping 292 matching lines @@
   std::map<std::string, Feature::Context> contexts;
   std::map<std::string, SimpleFeature::Location> locations;
   std::map<std::string, Feature::Platform> platforms;
   std::map<std::string, version_info::Channel> channels;
 };
 
 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;
 
   no_parent_ = false;
   for (base::DictionaryValue::Iterator it(*dictionary);
       !it.IsAtEnd();
       it.Advance()) {
     const std::string& key = it.key();
     const base::Value* value = &it.value();
     if (key == "matches") {
@@ skipping 23 matching lines @@
       dictionary->GetBoolean("noparent", &no_parent_);
     } else if (key == "component_extensions_auto_granted") {
       dictionary->GetBoolean("component_extensions_auto_granted",
                              &component_extensions_auto_granted_);
     } else if (key == "command_line_switch") {
       dictionary->GetString("command_line_switch", &command_line_switch_);
     } else if (key == "channel") {
       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_);
     }
   }
 
   // NOTE: ideally we'd sanity check that "matches" can be specified if and
   // only if there's a "web_page" or "webui" context, but without
   // (Simple)Features being aware of their own heirarchy this is impossible.
   //
   // For example, we might have feature "foo" available to "web_page" context
   // 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(
     const std::string& extension_id,
     Manifest::Type type,
     Manifest::Location location,
     int manifest_version,
     Platform platform) const {
   // Check extension type first to avoid granting platform app permissions
   // to component extensions.
@@ skipping 187 matching lines @@
 
 Feature::Availability SimpleFeature::CreateAvailability(
     AvailabilityResult result,
     version_info::Channel channel) const {
   return Availability(
       result, GetAvailabilityMessage(result, Manifest::TYPE_UNKNOWN, GURL(),
                                      UNSPECIFIED_CONTEXT, channel));
 }
 
 bool SimpleFeature::IsInternal() const {
-  return false;
+  return internal_;
 }
 
 bool SimpleFeature::IsIdInBlacklist(const std::string& extension_id) const {
   return IsIdInList(extension_id, blacklist_);
 }
 
 bool SimpleFeature::IsIdInWhitelist(const std::string& extension_id) const {
   return IsIdInList(extension_id, whitelist_);
 }
 
@@ skipping 56 matching lines @@
 bool SimpleFeature::IsValidExtensionId(const std::string& extension_id) {
   // Belt-and-suspenders philosophy here. We should be pretty confident by this
   // point that we've validated the extension ID format, but in case something
   // slips through, we avoid a class of attack where creative ID manipulation
   // leads to hash collisions.
   // 128 bits / 4 = 32 mpdecimal characters
   return (extension_id.length() == 32);
 }
 
 }  // namespace extensions