Implement API features for the Extension API feature system

chrome/common/extensions/api/extension_api.cc

Index: chrome/common/extensions/api/extension_api.cc
diff --git a/chrome/common/extensions/api/extension_api.cc b/chrome/common/extensions/api/extension_api.cc
index c2a9cce97931c24a04b16ad3b7f1e8a911e57e13..ee0a5f808e2796611bd60126bf1f5eb7bec44309 100644
--- a/chrome/common/extensions/api/extension_api.cc
+++ b/chrome/common/extensions/api/extension_api.cc
@@ -235,8 +235,7 @@ void ExtensionAPI::SplitDependencyName(const std::string& full_name,
 }
 
 bool ExtensionAPI::UsesFeatureSystem(const std::string& full_name) {
-  std::string api_name = GetAPINameFromFullName(full_name, NULL);
-  return features_.find(api_name) != features_.end();
+  return TryGetFeatureDependency(full_name) != NULL;
 }
 
 void ExtensionAPI::LoadSchema(const std::string& name,
@@ -332,10 +331,6 @@ void ExtensionAPI::LoadSchema(const std::string& name,
 }
 
 ExtensionAPI::ExtensionAPI() {
-  RegisterDependencyProvider("api", this);
-
-  // TODO(aa): Can remove this when all JSON files are converted.
-  RegisterDependencyProvider("", this);
 }
 
 ExtensionAPI::~ExtensionAPI() {
@@ -343,6 +338,8 @@ ExtensionAPI::~ExtensionAPI() {
 
 void ExtensionAPI::InitDefaultConfiguration() {
   RegisterDependencyProvider(
+      "api", BaseFeatureProvider::GetApiFeatures());
+  RegisterDependencyProvider(
       "manifest", BaseFeatureProvider::GetManifestFeatures());
   RegisterDependencyProvider(
       "permission", BaseFeatureProvider::GetPermissionFeatures());
@@ -422,27 +419,35 @@ Feature::Availability ExtensionAPI::IsAvailable(const std::string& full_name,
                                                 const Extension* extension,
                                                 Feature::Context context,
                                                 const GURL& url) {
-  std::set<std::string> dependency_names;
-  dependency_names.insert(full_name);
-  ResolveDependencies(&dependency_names);
+  std::string feature_type;
+  std::string feature_name;
+  SplitDependencyName(full_name, &feature_type, &feature_name);
 
+  std::string child_name;
+  std::string api_name = GetAPINameFromFullName(feature_name, &child_name);

[not at google - send to devlin, 2013/03/26 00:34:48]

nit: eyes want a newline after api_name.

[cduvall, 2013/03/26 19:24:05]

Done.

   // Check APIs not using the feature system first.
-  if (!UsesFeatureSystem(full_name)) {
-    return IsNonFeatureAPIAvailable(full_name, context, extension, url) ?
+  if (!UsesFeatureSystem(api_name)) {
+    // Make sure schema is loaded.
+    GetSchema(api_name);
+    return IsNonFeatureAPIAvailable(api_name, context, extension, url) ?

[not at google - send to devlin, 2013/03/26 00:34:48]

I think the GetSchema thing should go in IsNonFeatureAPIAvailable. It's an
implementation detail of that method.

[cduvall, 2013/03/26 19:24:05]

Done.

         Feature::CreateAvailability(Feature::IS_AVAILABLE, "") :
         Feature::CreateAvailability(Feature::INVALID_CONTEXT,
                                     kUnavailableMessage);
   }
 
-  for (std::set<std::string>::iterator iter = dependency_names.begin();
-       iter != dependency_names.end(); ++iter) {
-    Feature* feature = GetFeatureDependency(*iter);
-    CHECK(feature) << *iter;
-
-    Feature::Availability availability =
-        feature->IsAvailableToContext(extension, context, url);
-    if (!availability.is_available())
-      return availability;
+  Feature* feature = GetFeatureDependency(full_name);
+  CHECK(feature) << full_name;

[not at google - send to devlin, 2013/03/26 00:34:48]

yeah, here we're CHECKing anyway.

[cduvall, 2013/03/26 19:24:05]

Done.

+  Feature::Availability availability =
+      feature->IsAvailableToContext(extension, context, url);
+  if (!availability.is_available())
+    return availability;
+
+  for (std::set<std::string>::iterator iter = feature->dependencies().begin();
+       iter != feature->dependencies().end(); ++iter) {
+    Feature::Availability dependency_availability =
+        IsAvailable(*iter, extension, context, url);
+    if (!dependency_availability.is_available())
+      return dependency_availability;
   }
 
   return Feature::CreateAvailability(Feature::IS_AVAILABLE, "");
@@ -453,31 +458,30 @@ bool ExtensionAPI::IsPrivileged(const std::string& full_name) {
   std::string api_name = GetAPINameFromFullName(full_name, &child_name);
 
   // First try to use the feature system.
-  Feature* feature(GetFeature(full_name));
-  if (feature) {
+  if (UsesFeatureSystem(full_name)) {
     // An API is 'privileged' if it or any of its dependencies can only be run
     // in a blessed context.

[not at google - send to devlin, 2013/03/26 00:34:48]

I don't think this checking dependencies really makes sense anymore, since we
(well, you) fixed the type dependency problem properly. So this can be 1 line,
yay.

return feature->GetContexts()->count(Feature::BLESSED_EXTENSION_CONTEXT) > 0;

[cduvall, 2013/03/26 19:24:05]

Taking out the dependency checks causes ExtensionAPI.IsPrivilegedFeatures to
fail. For example, using the fake api_features.json for the test, if you check
IsPrivileged("test4.bar") should it return true, even though its dependency
"test4.foo" isn't privileged?

[not at google - send to devlin, 2013/03/26 21:31:21]

I can't find this. Are you referring to privileged_api_features.json? That
doesn't have a test4.bar. The other file with a test4.bar doesn't look valid
since it doesn't have a context specified.

Anyway, possibly answering your question -

Thinking about this some more, checking dependencies introduces ambiguity. What
does it mean if a feature specifies [content_scripts] but a dependency only
specifies [blessed]?  Vice versa? The feature specification doesn't really
attach a hierarchy to these contexts.

I think we should just enforce that all API dependencies include contexts, then
IsPrivileged doesn't get involved with dependencies. GetAvailability obviously
does, though.

[cduvall, 2013/03/26 22:08:48]

Ok, I removed dependency checking from IsPrivileged(), and tested to make sure
dependencies aren't checked.

-    std::set<std::string> resolved_dependencies;
-    resolved_dependencies.insert(full_name);
-    ResolveDependencies(&resolved_dependencies);
-    for (std::set<std::string>::iterator iter = resolved_dependencies.begin();
-         iter != resolved_dependencies.end(); ++iter) {
-      Feature* dependency = GetFeatureDependency(*iter);
-      for (std::set<Feature::Context>::iterator context =
-               dependency->GetContexts()->begin();
-           context != dependency->GetContexts()->end(); ++context) {
-        if (*context != Feature::BLESSED_EXTENSION_CONTEXT)
-          return false;
-      }
+    Feature* feature = GetFeatureDependency(full_name);

[not at google - send to devlin, 2013/03/26 00:34:48]

and here we could assign Feature up on line 460 and then check it for null.

[cduvall, 2013/03/26 19:24:05]

Done.

+    std::set<std::string> dependencies = feature->dependencies();
+    for (std::set<Feature::Context>::iterator context =
+             feature->GetContexts()->begin();
+         context != feature->GetContexts()->end(); ++context) {
+      if (*context != Feature::BLESSED_EXTENSION_CONTEXT)
+        return false;
+    }
+    for (std::set<std::string>::iterator iter = dependencies.begin();
+         iter != dependencies.end(); ++iter) {
+      if (!IsPrivileged(*iter))
+        return false;
     }
     return true;
   }
 
+  const DictionaryValue* schema = GetSchema(api_name);

[not at google - send to devlin, 2013/03/26 00:34:48]

nice catch.

// get now to populate |completely_unprivileged_apis_|s.

[cduvall, 2013/03/26 19:24:05]

Done.

   // If this API hasn't been converted yet, fall back to the old system.
   if (completely_unprivileged_apis_.count(api_name))
     return false;
 
-  const DictionaryValue* schema = GetSchema(api_name);
   if (partially_unprivileged_apis_.count(api_name))
     return IsChildNamePrivileged(schema, child_name);
 
@@ -597,48 +601,29 @@ std::set<std::string> ExtensionAPI::GetAllAPINames() {
   return result;
 }
 
-Feature* ExtensionAPI::GetFeature(const std::string& full_name) {
-  // Ensure it's loaded.
-  GetSchema(full_name);
-
-  std::string child_name;
-  std::string api_namespace = GetAPINameFromFullName(full_name, &child_name);
-
-  APIFeatureMap::iterator feature_map = features_.find(api_namespace);
-  if (feature_map == features_.end())
-    return NULL;
-
-  Feature* result = NULL;
-  FeatureMap::iterator child_feature = feature_map->second->find(child_name);
-  if (child_feature != feature_map->second->end()) {
-    result = child_feature->second.get();
-  } else {
-    FeatureMap::iterator parent_feature = feature_map->second->find("");
-    CHECK(parent_feature != feature_map->second->end());
-    result = parent_feature->second.get();
-  }
-
-  if (result->GetContexts()->empty()) {
-    LOG(ERROR) << "API feature '" << full_name
-               << "' must specify at least one context.";
-    return NULL;
-  }
-
-  return result;
+Feature* ExtensionAPI::GetFeatureDependency(const std::string& full_name) {
+  Feature* feature = TryGetFeatureDependency(full_name);
+  CHECK(feature) << full_name;
+  return feature;
 }
 
-Feature* ExtensionAPI::GetFeatureDependency(const std::string& full_name) {
+Feature* ExtensionAPI::TryGetFeatureDependency(const std::string& full_name) {

[not at google - send to devlin, 2013/03/26 00:34:48]

I think this is my favourite method. Let's delete UsesFeatureSystem and
GetFeatureDependency and rename this GetFeatureDependency.

[cduvall, 2013/03/26 19:24:05]

Done.

   std::string feature_type;
   std::string feature_name;
   SplitDependencyName(full_name, &feature_type, &feature_name);
 
   FeatureProviderMap::iterator provider =
       dependency_providers_.find(feature_type);
-  CHECK(provider != dependency_providers_.end()) << full_name;
+  if (provider == dependency_providers_.end())
+    return NULL;
 
   Feature* feature = provider->second->GetFeature(feature_name);
-  CHECK(feature) << full_name;
-
+  // Try getting the feature for the parent API, if this was a child.

[not at google - send to devlin, 2013/03/26 00:34:48]

In the general case I think we need to step all the way up until there is no
more api name.

[cduvall, 2013/03/26 19:24:05]

GetAPINameFromFullName() guarantees to either return a real API name or an empty
string. There doesn't seem to be any point of stepping all the way up, unless
I'm misunderstanding you.

[not at google - send to devlin, 2013/03/26 21:31:21]

Oh ok, never mind.

+  if (!feature) {
+    std::string child_name;
+    feature = provider->second->GetFeature(
+        GetAPINameFromFullName(feature_name, &child_name));
+  }
   return feature;
 }
 
@@ -678,47 +663,6 @@ bool ExtensionAPI::IsAPIAllowed(const std::string& name,
       extension->optional_permission_set()->HasAnyAccessToAPI(name);
 }
 
-void ExtensionAPI::ResolveDependencies(std::set<std::string>* out) {
-  std::set<std::string> missing_dependencies;
-  for (std::set<std::string>::iterator i = out->begin(); i != out->end(); ++i)
-    GetMissingDependencies(*i, *out, &missing_dependencies);
-
-  while (missing_dependencies.size()) {
-    std::string next = *missing_dependencies.begin();
-    missing_dependencies.erase(next);
-    out->insert(next);
-    GetMissingDependencies(next, *out, &missing_dependencies);
-  }
-}
-
-void ExtensionAPI::GetMissingDependencies(
-    const std::string& api_name,
-    const std::set<std::string>& excluding,
-    std::set<std::string>* out) {
-  std::string feature_type;
-  std::string feature_name;
-  SplitDependencyName(api_name, &feature_type, &feature_name);
-
-  // Only API features can have dependencies for now.
-  if (feature_type != "api")
-    return;
-
-  const DictionaryValue* schema = GetSchema(feature_name);
-  CHECK(schema) << "Schema for " << feature_name << " not found";
-
-  const ListValue* dependencies = NULL;
-  if (!schema->GetList("dependencies", &dependencies))
-    return;
-
-  for (size_t i = 0; i < dependencies->GetSize(); ++i) {
-    std::string dependency_name;
-    if (dependencies->GetString(i, &dependency_name) &&
-        !excluding.count(dependency_name)) {
-      out->insert(dependency_name);
-    }
-  }
-}
-
 bool ExtensionAPI::IsPrivilegedAPI(const std::string& name) {
   return completely_unprivileged_apis_.count(name) ||
       partially_unprivileged_apis_.count(name);