Support a "policy" extension location in extension features files. At the same

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 ebb9cb21fd43de212945780a7b52296ac0182708..92b41b4120832b9da4287a4fb44e47f7415c09da 100644
--- a/extensions/common/features/simple_feature.cc
+++ b/extensions/common/features/simple_feature.cc
@@ -21,31 +21,36 @@ namespace {
 
 struct Mappings {
   Mappings() {
-    extension_types["extension"] = Manifest::TYPE_EXTENSION;
-    extension_types["theme"] = Manifest::TYPE_THEME;
-    extension_types["legacy_packaged_app"] = Manifest::TYPE_LEGACY_PACKAGED_APP;
-    extension_types["hosted_app"] = Manifest::TYPE_HOSTED_APP;
-    extension_types["platform_app"] = Manifest::TYPE_PLATFORM_APP;
-    extension_types["shared_module"] = Manifest::TYPE_SHARED_MODULE;
-
-    contexts["blessed_extension"] = Feature::BLESSED_EXTENSION_CONTEXT;
-    contexts["unblessed_extension"] = Feature::UNBLESSED_EXTENSION_CONTEXT;
-    contexts["content_script"] = Feature::CONTENT_SCRIPT_CONTEXT;
-    contexts["web_page"] = Feature::WEB_PAGE_CONTEXT;
-    contexts["blessed_web_page"] = Feature::BLESSED_WEB_PAGE_CONTEXT;
-
-    locations["component"] = Feature::COMPONENT_LOCATION;
-
-    platforms["chromeos"] = Feature::CHROMEOS_PLATFORM;
-    platforms["linux"] = Feature::LINUX_PLATFORM;
-    platforms["mac"] = Feature::MACOSX_PLATFORM;
-    platforms["win"] = Feature::WIN_PLATFORM;
+    extension_types["extension"].push_back(Manifest::TYPE_EXTENSION);

[not at google - send to devlin, 2014/04/17 21:33:14]

I'm not... super happy about these changes.

[Yoyo Zhou, 2014/04/17 21:53:12]

Can you replace Manifest::Location with something like a LocationClass that
matches multiple Manifest::Locations? We already have a custom notion of
location for Features, so why not replace it with a different custom notion?

[not at google - send to devlin, 2014/04/17 21:59:31]

I found the old way of doing that a bit awkward; exposing a utility method and
calling it from the right places.

but it would be a less extreme change.

[Yoyo Zhou, 2014/04/17 22:00:30]

You don't have to expose it. Just pass in the Manifest::Location to
IsAvailable...().

+    extension_types["theme"].push_back(Manifest::TYPE_THEME);
+    extension_types["legacy_packaged_app"].push_back(
+        Manifest::TYPE_LEGACY_PACKAGED_APP);
+    extension_types["hosted_app"].push_back(Manifest::TYPE_HOSTED_APP);
+    extension_types["platform_app"].push_back(Manifest::TYPE_PLATFORM_APP);
+    extension_types["shared_module"].push_back(Manifest::TYPE_SHARED_MODULE);
+
+    contexts["blessed_extension"].push_back(Feature::BLESSED_EXTENSION_CONTEXT);
+    contexts["unblessed_extension"].push_back(
+        Feature::UNBLESSED_EXTENSION_CONTEXT);
+    contexts["content_script"].push_back(Feature::CONTENT_SCRIPT_CONTEXT);
+    contexts["web_page"].push_back(Feature::WEB_PAGE_CONTEXT);
+    contexts["blessed_web_page"].push_back(Feature::BLESSED_WEB_PAGE_CONTEXT);
+
+    locations["component"].push_back(Manifest::COMPONENT);
+    locations["component"].push_back(Manifest::EXTERNAL_COMPONENT);
+    locations["policy"].push_back(Manifest::EXTERNAL_POLICY_DOWNLOAD);
+    locations["policy"].push_back(Manifest::EXTERNAL_POLICY);
+
+    platforms["chromeos"].push_back(Feature::CHROMEOS_PLATFORM);
+    platforms["linux"].push_back(Feature::LINUX_PLATFORM);
+    platforms["mac"].push_back(Feature::MACOSX_PLATFORM);
+    platforms["win"].push_back(Feature::WIN_PLATFORM);
   }
 
-  std::map<std::string, Manifest::Type> extension_types;
-  std::map<std::string, Feature::Context> contexts;
-  std::map<std::string, Feature::Location> locations;
-  std::map<std::string, Feature::Platform> platforms;
+  std::map<std::string, std::vector<Manifest::Type> > extension_types;
+  std::map<std::string, std::vector<Feature::Context> > contexts;
+  std::map<std::string, std::vector<Manifest::Location> > locations;
+  std::map<std::string, std::vector<Feature::Platform> > platforms;
 };
 
 base::LazyInstance<Mappings> g_mappings = LAZY_INSTANCE_INITIALIZER;
@@ -67,33 +72,29 @@ void ParseSet(const base::DictionaryValue* value,
   }
 }
 
-template<typename T>
-void ParseEnum(const std::string& string_value,
-               T* enum_value,
-               const std::map<std::string, T>& mapping) {
-  typename std::map<std::string, T>::const_iterator iter =
+template <typename T>
+void ParseEnumHelper(const std::string& string_value,
+                     const std::map<std::string, std::vector<T> >& mapping,
+                     std::set<T>* enum_set) {
+  typename std::map<std::string, std::vector<T> >::const_iterator iter =
       mapping.find(string_value);
   CHECK(iter != mapping.end()) << string_value;
-  *enum_value = iter->second;
+  enum_set->insert(iter->second.begin(), iter->second.end());
 }
 
-template<typename T>
-void ParseEnum(const base::DictionaryValue* value,
-               const std::string& property,
-               T* enum_value,
-               const std::map<std::string, T>& mapping) {
-  std::string string_value;
-  if (!value->GetString(property, &string_value))
-    return;
-
-  ParseEnum(string_value, enum_value, mapping);
+template <typename T>
+void ParseEnum(const std::string& string_value,
+               const std::map<std::string, std::vector<T> >& mapping,
+               std::set<T>* enum_set) {
+  enum_set->clear();
+  ParseEnumHelper(string_value, mapping, enum_set);
 }
 
-template<typename T>
+template <typename T>
 void ParseEnumSet(const base::DictionaryValue* value,
                   const std::string& property,
-                  std::set<T>* enum_set,
-                  const std::map<std::string, T>& mapping) {
+                  const std::map<std::string, std::vector<T> >& mapping,
+                  std::set<T>* enum_set) {
   if (!value->HasKey(property))
     return;
 
@@ -102,9 +103,11 @@ void ParseEnumSet(const base::DictionaryValue* value,
   std::string property_string;
   if (value->GetString(property, &property_string)) {
     if (property_string == "all") {
-      for (typename std::map<std::string, T>::const_iterator j =
-               mapping.begin(); j != mapping.end(); ++j) {
-        enum_set->insert(j->second);
+      for (typename std::map<std::string, std::vector<T> >::const_iterator j =
+               mapping.begin();
+           j != mapping.end();
+           ++j) {
+        enum_set->insert(j->second.begin(), j->second.end());
       }
     }
     return;
@@ -114,9 +117,7 @@ void ParseEnumSet(const base::DictionaryValue* value,
   ParseSet(value, property, &string_set);
   for (std::set<std::string>::iterator iter = string_set.begin();
        iter != string_set.end(); ++iter) {
-    T enum_value = static_cast<T>(0);
-    ParseEnum(*iter, &enum_value, mapping);
-    enum_set->insert(enum_value);
+    ParseEnumHelper(*iter, mapping, enum_set);
   }
 }
 
@@ -215,10 +216,8 @@ std::string HashExtensionId(const std::string& extension_id) {
 }  // namespace
 
 SimpleFeature::SimpleFeature()
-    : location_(UNSPECIFIED_LOCATION),
-      min_manifest_version_(0),
-      max_manifest_version_(0),
-      has_parent_(false) {}
+    : min_manifest_version_(0), max_manifest_version_(0), has_parent_(false) {
+}
 
 SimpleFeature::~SimpleFeature() {}
 
@@ -230,14 +229,21 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* value) {
   ParseURLPatterns(value, "matches", &matches_);
   ParseSet(value, "whitelist", &whitelist_);
   ParseSet(value, "dependencies", &dependencies_);
-  ParseEnumSet<Manifest::Type>(value, "extension_types", &extension_types_,
-                                g_mappings.Get().extension_types);
-  ParseEnumSet<Context>(value, "contexts", &contexts_,
-                        g_mappings.Get().contexts);
-  ParseEnum<Location>(value, "location", &location_,
-                      g_mappings.Get().locations);
-  ParseEnumSet<Platform>(value, "platforms", &platforms_,
-                         g_mappings.Get().platforms);
+  ParseEnumSet<Manifest::Type>(value,
+                               "extension_types",
+                               g_mappings.Get().extension_types,
+                               &extension_types_);
+  // Only a single value is supported for "location", but it expands to
+  // multiple possible values.
+  std::string location;
+  if (value->GetStringWithoutPathExpansion("location", &location)) {
+    ParseEnum<Manifest::Location>(
+        location, g_mappings.Get().locations, &locations_);
+  }
+  ParseEnumSet<Context>(
+      value, "contexts", g_mappings.Get().contexts, &contexts_);
+  ParseEnumSet<Platform>(
+      value, "platforms", g_mappings.Get().platforms, &platforms_);
   value->GetInteger("min_manifest_version", &min_manifest_version_);
   value->GetInteger("max_manifest_version", &max_manifest_version_);
 
@@ -264,23 +270,19 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* value) {
 Feature::Availability SimpleFeature::IsAvailableToManifest(
     const std::string& extension_id,
     Manifest::Type type,
-    Location location,
+    Manifest::Location location,
     int manifest_version,
     Platform platform) const {
-  // Check extension type first to avoid granting platform app permissions
-  // to component extensions.
-  // HACK(kalman): user script -> extension. Solve this in a more generic way
-  // when we compile feature files.
-  Manifest::Type type_to_check = (type == Manifest::TYPE_USER_SCRIPT) ?
-      Manifest::TYPE_EXTENSION : type;
   if (!extension_types_.empty() &&
-      extension_types_.find(type_to_check) == extension_types_.end()) {
+      extension_types_.find(type) == extension_types_.end()) {
     return CreateAvailability(INVALID_TYPE, type);
   }
 
   // Component extensions can access any feature.
-  if (location == COMPONENT_LOCATION)
+  if (location == Manifest::COMPONENT ||
+      location == Manifest::EXTERNAL_COMPONENT) {
     return CreateAvailability(IS_AVAILABLE, type);
+  }
 
   if (!whitelist_.empty()) {
     if (!IsIdInWhitelist(extension_id)) {
@@ -298,7 +300,7 @@ Feature::Availability SimpleFeature::IsAvailableToManifest(
     }
   }
 
-  if (location_ != UNSPECIFIED_LOCATION && location_ != location)
+  if (!locations_.empty() && locations_.find(location) == locations_.end())

[Yoyo Zhou, 2014/04/17 21:53:12]

Change this test to be something like location_class_.Contains(location)

[not at google - send to devlin, 2014/04/17 23:18:11]

done sort of. I went back to how the code was before with the Location and move
the util in here. I didn't like the idea of updating that test again this time
with a "LocationClass" that's effectively an implementation detail.

     return CreateAvailability(INVALID_LOCATION, type);
 
   if (!platforms_.empty() &&
@@ -329,12 +331,11 @@ Feature::Availability SimpleFeature::IsAvailableToContext(
     const GURL& url,
     SimpleFeature::Platform platform) const {
   if (extension) {
-    Availability result = IsAvailableToManifest(
-        extension->id(),
-        extension->GetType(),
-        ConvertLocation(extension->location()),
-        extension->manifest_version(),
-        platform);
+    Availability result = IsAvailableToManifest(extension->id(),
+                                                extension->GetType(),
+                                                extension->location(),
+                                                extension->manifest_version(),
+                                                platform);
     if (!result.is_available())
       return result;
   }