Don't cache JSON objects in SimpleFeatureProvider

BUG=120068


Review URL: http://codereview.chromium.org/9958055

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@130866 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 98 insertions, 96 deletions

diff --git a/chrome/common/extensions/api/extension_api.cc b/chrome/common/extensions/api/extension_api.cc
index 8feaad3..6853761 100644
--- a/chrome/common/extensions/api/extension_api.cc
+++ b/chrome/common/extensions/api/extension_api.cc
@@ -406,8 +406,8 @@ bool ExtensionAPI::IsAvailable(const std::string& full_name,
 
   for (std::set<std::string>::iterator iter = dependency_names.begin();
        iter != dependency_names.end(); ++iter) {
-    scoped_ptr<Feature> feature(GetFeatureDependency(full_name));
-    CHECK(feature.get()) << *iter;
+    Feature* feature = GetFeatureDependency(full_name);
+    CHECK(feature) << *iter;
 
     Feature::Availability availability =
         feature->IsAvailableToContext(extension, context);
@@ -423,8 +423,8 @@ bool ExtensionAPI::IsPrivileged(const std::string& full_name) {
   std::string api_name = GetAPINameFromFullName(full_name, &child_name);
 
   // First try to use the feature system.
-  scoped_ptr<Feature> feature(GetFeature(full_name));
-  if (feature.get()) {
+  Feature* feature(GetFeature(full_name));
+  if (feature) {
     // An API is 'privileged' if it or any of its dependencies can only be run
     // in a blessed context.
     std::set<std::string> resolved_dependencies;
@@ -432,7 +432,7 @@ bool ExtensionAPI::IsPrivileged(const std::string& full_name) {
     ResolveDependencies(&resolved_dependencies);
     for (std::set<std::string>::iterator iter = resolved_dependencies.begin();
          iter != resolved_dependencies.end(); ++iter) {
-      scoped_ptr<Feature> dependency(GetFeatureDependency(*iter));
+      Feature* dependency = GetFeatureDependency(*iter);
       for (std::set<Feature::Context>::iterator context =
                dependency->contexts()->begin();
            context != dependency->contexts()->end(); ++context) {
@@ -558,40 +558,37 @@ scoped_ptr<std::set<std::string> > ExtensionAPI::GetAPIsForContext(
   return result.Pass();
 }
 
-scoped_ptr<Feature> ExtensionAPI::GetFeature(const std::string& full_name) {
+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 api_features = features_.find(api_namespace);
-  if (api_features == features_.end())
-    return scoped_ptr<Feature>(NULL);
+  APIFeatureMap::iterator feature_map = features_.find(api_namespace);
+  if (feature_map == features_.end())
+    return NULL;
 
-  scoped_ptr<Feature> result;
-  FeatureMap::iterator child_feature = api_features->second->find(child_name);
-  if (child_feature != api_features->second->end()) {
-    // TODO(aa): Argh, having FeatureProvider return a scoped pointer was a
-    // mistake. See: crbug.com/120068.
-    result.reset(new Feature(*child_feature->second));
+  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 = api_features->second->find("");
-    CHECK(parent_feature != api_features->second->end());
-    result.reset(new Feature(*parent_feature->second));
+    FeatureMap::iterator parent_feature = feature_map->second->find("");
+    CHECK(parent_feature != feature_map->second->end());
+    result = parent_feature->second.get();
   }
 
   if (result->contexts()->empty()) {
-    result.reset();
     LOG(ERROR) << "API feature '" << full_name
                << "' must specify at least one context.";
+    return NULL;
   }
 
-  return result.Pass();
+  return result;
 }
 
-scoped_ptr<Feature> ExtensionAPI::GetFeatureDependency(
-    const std::string& full_name) {
+Feature* ExtensionAPI::GetFeatureDependency(const std::string& full_name) {
   std::string feature_type;
   std::string feature_name;
   SplitDependencyName(full_name, &feature_type, &feature_name);
@@ -600,10 +597,10 @@ scoped_ptr<Feature> ExtensionAPI::GetFeatureDependency(
       dependency_providers_.find(feature_type);
   CHECK(provider != dependency_providers_.end()) << full_name;
 
-  scoped_ptr<Feature> feature(provider->second->GetFeature(feature_name));
-  CHECK(feature.get()) << full_name;
+  Feature* feature = provider->second->GetFeature(feature_name);
+  CHECK(feature) << full_name;
 
-  return feature.Pass();
+  return feature;
 }
 
 std::string ExtensionAPI::GetAPINameFromFullName(const std::string& full_name,
diff --git a/chrome/common/extensions/api/extension_api.h b/chrome/common/extensions/api/extension_api.h
index b2da190..1d489c3 100644
--- a/chrome/common/extensions/api/extension_api.h
+++ b/chrome/common/extensions/api/extension_api.h
@@ -96,7 +96,7 @@ class ExtensionAPI : public FeatureProvider {
   // Gets a Feature object describing the API with the specified |full_name|.
   // This can be either an API namespace (like history, or
   // experimental.bookmarks), or it can be an individual function or event.
-  virtual scoped_ptr<Feature> GetFeature(const std::string& full_name) OVERRIDE;
+  virtual Feature* GetFeature(const std::string& full_name) OVERRIDE;
 
   // Splits a full name from the extension API into its API and child name
   // parts. Some examples:
@@ -134,7 +134,7 @@ class ExtensionAPI : public FeatureProvider {
   void GetAllowedAPIs(const Extension* extension, std::set<std::string>* out);
 
   // Gets a feature from any dependency provider.
-  scoped_ptr<Feature> GetFeatureDependency(const std::string& dependency_name);
+  Feature* GetFeatureDependency(const std::string& dependency_name);
 
   // Adds dependent schemas to |out| as determined by the "dependencies"
   // property.
diff --git a/chrome/common/extensions/api/extension_api_unittest.cc b/chrome/common/extensions/api/extension_api_unittest.cc
index 6448679..c5f5ce4 100644
--- a/chrome/common/extensions/api/extension_api_unittest.cc
+++ b/chrome/common/extensions/api/extension_api_unittest.cc
@@ -26,12 +26,12 @@ class TestFeatureProvider : public FeatureProvider {
       : context_(context) {
   }
 
-  virtual scoped_ptr<Feature> GetFeature(const std::string& name) OVERRIDE {
-    scoped_ptr<Feature> result(new Feature());
+  virtual Feature* GetFeature(const std::string& name) OVERRIDE {
+    Feature* result = new Feature();
     result->set_name(name);
     result->extension_types()->insert(Extension::TYPE_EXTENSION);
     result->contexts()->insert(context_);
-    return result.Pass();
+    return result;
   }
 
  private:
@@ -350,15 +350,15 @@ TEST(ExtensionAPI, GetAPINameFromFullName) {
 TEST(ExtensionAPI, DefaultConfigurationFeatures) {
   scoped_ptr<ExtensionAPI> api(ExtensionAPI::CreateWithDefaultConfiguration());
 
-  scoped_ptr<Feature> bookmarks(api->GetFeature("bookmarks"));
-  scoped_ptr<Feature> bookmarks_create(api->GetFeature("bookmarks.create"));
+  Feature* bookmarks = api->GetFeature("bookmarks");
+  Feature* bookmarks_create = api->GetFeature("bookmarks.create");
 
   struct {
     Feature* feature;
     // TODO(aa): More stuff to test over time.
   } test_data[] = {
-    { bookmarks.get() },
-    { bookmarks_create.get() }
+    { bookmarks },
+    { bookmarks_create }
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_data); ++i) {
@@ -408,8 +408,8 @@ TEST(ExtensionAPI, FeaturesRequireContexts) {
     api.RegisterSchema("test", base::StringPiece(schema_source));
     api.LoadAllSchemas();
 
-    scoped_ptr<Feature> feature(api.GetFeature("test"));
-    EXPECT_EQ(test_data[i].expect_success, feature.get() != NULL) << i;
+    Feature* feature = api.GetFeature("test");
+    EXPECT_EQ(test_data[i].expect_success, feature != NULL) << i;
   }
 }
 
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 3869df1..9e2ac3b 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -3305,13 +3305,13 @@ bool Extension::ParsePermissions(const char* key,
       if (permission) {
         extensions::SimpleFeatureProvider* permission_features =
             extensions::SimpleFeatureProvider::GetPermissionFeatures();
-        scoped_ptr<extensions::Feature> feature(
-            permission_features->GetFeature(permission->name()));
+        extensions::Feature* feature =
+            permission_features->GetFeature(permission->name());
 
         // The feature should exist since we just got an ExtensionAPIPermission
         // for it. The two systems should be updated together whenever a
         // permission is added.
-        CHECK(feature.get());
+        CHECK(feature);
 
         extensions::Feature::Availability availability =
             feature->IsAvailableToManifest(
diff --git a/chrome/common/extensions/feature_provider.h b/chrome/common/extensions/feature_provider.h
index 0db0468..1b2b611 100644
--- a/chrome/common/extensions/feature_provider.h
+++ b/chrome/common/extensions/feature_provider.h
@@ -8,8 +8,6 @@
 
 #include <string>
 
-#include "base/memory/scoped_ptr.h"
-
 namespace extensions {
 
 class Feature;
@@ -21,7 +19,7 @@ class FeatureProvider {
   virtual ~FeatureProvider() {}
 
   // Returns the feature with the specified name.
-  virtual scoped_ptr<Feature> GetFeature(const std::string& name) = 0;
+  virtual Feature* GetFeature(const std::string& name) = 0;
 };
 
 }  // namespace extensions
diff --git a/chrome/common/extensions/manifest.cc b/chrome/common/extensions/manifest.cc
index aa4a495..13f9e03 100644
--- a/chrome/common/extensions/manifest.cc
+++ b/chrome/common/extensions/manifest.cc
@@ -28,9 +28,9 @@ Manifest::~Manifest() {
 bool Manifest::ValidateManifest(string16* error) const {
   for (DictionaryValue::key_iterator key = value_->begin_keys();
        key != value_->end_keys(); ++key) {
-    scoped_ptr<Feature> feature =
+    Feature* feature =
         SimpleFeatureProvider::GetManifestFeatures()->GetFeature(*key);
-    if (!feature.get()) {
+    if (!feature) {
       // When validating the extension manifests, we ignore keys that are not
       // recognized for forward compatibility.
       // TODO(aa): Consider having an error here in the case of strict error
@@ -150,9 +150,9 @@ bool Manifest::CanAccessPath(const std::string& path) const {
 }
 
 bool Manifest::CanAccessKey(const std::string& key) const {
-  scoped_ptr<Feature> feature =
+  Feature* feature =
       SimpleFeatureProvider::GetManifestFeatures()->GetFeature(key);
-  if (!feature.get())
+  if (!feature)
     return false;
 
   return Feature::IS_AVAILABLE == feature->IsAvailableToManifest(
diff --git a/chrome/common/extensions/simple_feature_provider.cc b/chrome/common/extensions/simple_feature_provider.cc
index e5acf36..63741aa 100644
--- a/chrome/common/extensions/simple_feature_provider.cc
+++ b/chrome/common/extensions/simple_feature_provider.cc
@@ -55,7 +55,7 @@ struct Static {
     scoped_ptr<DictionaryValue> dictionary_value(
         static_cast<DictionaryValue*>(value));
     return scoped_ptr<SimpleFeatureProvider>(
-        new SimpleFeatureProvider(dictionary_value.Pass(), factory));
+        new SimpleFeatureProvider(dictionary_value.get(), factory));
   }
 };
 
@@ -63,11 +63,33 @@ base::LazyInstance<Static> g_static = LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
-SimpleFeatureProvider::SimpleFeatureProvider(scoped_ptr<DictionaryValue> root,
+SimpleFeatureProvider::SimpleFeatureProvider(DictionaryValue* root,
                                              FeatureFactory factory)
-    : root_(root.release()),
-      factory_(factory ? factory :
+    : factory_(factory ? factory :
                static_cast<FeatureFactory>(&CreateFeature<Feature>)) {
+  for (DictionaryValue::Iterator iter(*root); iter.HasNext(); iter.Advance()) {
+    if (iter.value().GetType() != Value::TYPE_DICTIONARY) {
+      LOG(ERROR) << iter.key() << ": Feature description must be dictionary.";
+      continue;
+    }
+
+    linked_ptr<Feature> feature((*factory_)());
+    feature->set_name(iter.key());
+    feature->Parse(static_cast<const DictionaryValue*>(&iter.value()));
+
+    if (feature->extension_types()->empty()) {
+      LOG(ERROR) << iter.key() << ": Simple features must specify atleast one "
+                 << "value for extension_types.";
+      continue;
+    }
+
+    if (!feature->contexts()->empty()) {
+      LOG(ERROR) << iter.key() << ": Simple features do not support contexts.";
+      continue;
+    }
+
+    features_[iter.key()] = feature;
+  }
 }
 
 SimpleFeatureProvider::~SimpleFeatureProvider() {
@@ -85,37 +107,19 @@ SimpleFeatureProvider* SimpleFeatureProvider::GetPermissionFeatures() {
 
 std::set<std::string> SimpleFeatureProvider::GetAllFeatureNames() const {
   std::set<std::string> result;
-  for (DictionaryValue::key_iterator iter = root_->begin_keys();
-       iter != root_->end_keys(); ++iter) {
-    result.insert(*iter);
+  for (FeatureMap::const_iterator iter = features_.begin();
+       iter != features_.end(); ++iter) {
+    result.insert(iter->first);
   }
   return result;
 }
 
-scoped_ptr<Feature> SimpleFeatureProvider::GetFeature(const std::string& name) {
-  DictionaryValue* description = NULL;
-  if (!root_->GetDictionary(name, &description)) {
-    LOG(ERROR) << name << ": Definition not found.";
-    return scoped_ptr<Feature>();
-  }
-
-  scoped_ptr<Feature> feature(new Feature());
-  feature.reset((*factory_)());
-  feature->set_name(name);
-  feature->Parse(description);
-
-  if (feature->extension_types()->empty()) {
-    LOG(ERROR) << name << ": Simple features must specify atleast one value "
-               << "for extension_types.";
-    return scoped_ptr<Feature>();
-  }
-
-  if (!feature->contexts()->empty()) {
-    LOG(ERROR) << name << ": Simple features do not support contexts.";
-    return scoped_ptr<Feature>();
-  }
-
-  return feature.Pass();
+Feature* SimpleFeatureProvider::GetFeature(const std::string& name) {
+  FeatureMap::iterator iter = features_.find(name);
+  if (iter != features_.end())
+    return iter->second.get();
+  else
+    return NULL;
 }
 
 }  // namespace
diff --git a/chrome/common/extensions/simple_feature_provider.h b/chrome/common/extensions/simple_feature_provider.h
index 0b9172c..e96c87b 100644
--- a/chrome/common/extensions/simple_feature_provider.h
+++ b/chrome/common/extensions/simple_feature_provider.h
@@ -9,7 +9,7 @@
 #include <set>
 #include <string>
 
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/linked_ptr.h"
 #include "base/values.h"
 #include "chrome/common/extensions/feature.h"
 #include "chrome/common/extensions/feature_provider.h"
@@ -23,8 +23,7 @@ class SimpleFeatureProvider : public FeatureProvider {
 
   // Creates a new SimpleFeatureProvider. Pass null to |factory| to have the
   // provider create plain old Feature instances.
-  SimpleFeatureProvider(scoped_ptr<DictionaryValue> root,
-                        FeatureFactory factory);
+  SimpleFeatureProvider(DictionaryValue* root, FeatureFactory factory);
   virtual ~SimpleFeatureProvider();
 
   // Gets an instance for the _manifest_features.json file that is baked into
@@ -39,11 +38,12 @@ class SimpleFeatureProvider : public FeatureProvider {
   std::set<std::string> GetAllFeatureNames() const;
 
   // Gets the feature |feature_name|, if it exists.
-  virtual scoped_ptr<Feature> GetFeature(
-      const std::string& feature_name) OVERRIDE;
+  virtual Feature* GetFeature(const std::string& feature_name) OVERRIDE;
 
  private:
-  scoped_ptr<DictionaryValue> root_;
+  typedef std::map<std::string, linked_ptr<Feature> > FeatureMap;
+  FeatureMap features_;
+
   FeatureFactory factory_;
 };
 
diff --git a/chrome/common/extensions/simple_feature_provider_unittest.cc b/chrome/common/extensions/simple_feature_provider_unittest.cc
index 401828e..3d9dd72 100644
--- a/chrome/common/extensions/simple_feature_provider_unittest.cc
+++ b/chrome/common/extensions/simple_feature_provider_unittest.cc
@@ -12,8 +12,8 @@ using extensions::SimpleFeatureProvider;
 TEST(SimpleFeatureProvider, ManifestFeatures) {
   SimpleFeatureProvider* provider =
       SimpleFeatureProvider::GetManifestFeatures();
-  scoped_ptr<Feature> feature = provider->GetFeature("description");
-  ASSERT_TRUE(feature.get());
+  Feature* feature = provider->GetFeature("description");
+  ASSERT_TRUE(feature);
   EXPECT_EQ(5u, feature->extension_types()->size());
   EXPECT_EQ(1u, feature->extension_types()->count(Extension::TYPE_EXTENSION));
   EXPECT_EQ(1u,
@@ -38,12 +38,12 @@ TEST(SimpleFeatureProvider, ManifestFeatures) {
       extension.get(), Feature::UNSPECIFIED_CONTEXT));
 
   feature = provider->GetFeature("theme");
-  ASSERT_TRUE(feature.get());
+  ASSERT_TRUE(feature);
   EXPECT_EQ(Feature::INVALID_TYPE, feature->IsAvailableToContext(
       extension.get(), Feature::UNSPECIFIED_CONTEXT));
 
   feature = provider->GetFeature("devtools_page");
-  ASSERT_TRUE(feature.get());
+  ASSERT_TRUE(feature);
   EXPECT_EQ(Feature::NOT_PRESENT, feature->IsAvailableToContext(
       extension.get(), Feature::UNSPECIFIED_CONTEXT));
 }
@@ -51,8 +51,8 @@ TEST(SimpleFeatureProvider, ManifestFeatures) {
 TEST(SimpleFeatureProvider, PermissionFeatures) {
   SimpleFeatureProvider* provider =
       SimpleFeatureProvider::GetPermissionFeatures();
-  scoped_ptr<Feature> feature = provider->GetFeature("browsingData");
-  ASSERT_TRUE(feature.get());
+  Feature* feature = provider->GetFeature("browsingData");
+  ASSERT_TRUE(feature);
   EXPECT_EQ(3u, feature->extension_types()->size());
   EXPECT_EQ(1u, feature->extension_types()->count(Extension::TYPE_EXTENSION));
   EXPECT_EQ(1u,
@@ -77,12 +77,12 @@ TEST(SimpleFeatureProvider, PermissionFeatures) {
       extension.get(), Feature::UNSPECIFIED_CONTEXT));
 
   feature = provider->GetFeature("chromePrivate");
-  ASSERT_TRUE(feature.get());
+  ASSERT_TRUE(feature);
   EXPECT_EQ(Feature::NOT_FOUND_IN_WHITELIST, feature->IsAvailableToContext(
       extension.get(), Feature::UNSPECIFIED_CONTEXT));
 
   feature = provider->GetFeature("clipboardWrite");
-  ASSERT_TRUE(feature.get());
+  ASSERT_TRUE(feature);
   EXPECT_EQ(Feature::NOT_PRESENT, feature->IsAvailableToContext(
       extension.get(), Feature::UNSPECIFIED_CONTEXT));
 }
@@ -102,19 +102,22 @@ TEST(SimpleFeatureProvider, Validation) {
   feature2->Set("contexts", contexts);
   value->Set("feature2", feature2);
 
-  SimpleFeatureProvider provider(value.Pass(), NULL);
+  scoped_ptr<SimpleFeatureProvider> provider(
+      new SimpleFeatureProvider(value.get(), NULL));
 
   // feature1 won't validate because it lacks an extension type.
-  EXPECT_FALSE(provider.GetFeature("feature1").get());
+  EXPECT_FALSE(provider->GetFeature("feature1"));
 
   // If we add one, it works.
   feature1->Set("extension_types", extension_types->DeepCopy());
-  EXPECT_TRUE(provider.GetFeature("feature1").get());
+  provider.reset(new SimpleFeatureProvider(value.get(), NULL));
+  EXPECT_TRUE(provider->GetFeature("feature1"));
 
   // feature2 won't validate because of the presence of "contexts".
-  EXPECT_FALSE(provider.GetFeature("feature2").get());
+  EXPECT_FALSE(provider->GetFeature("feature2"));
 
   // If we remove it, it works.
   feature2->Remove("contexts", NULL);
-  EXPECT_TRUE(provider.GetFeature("feature2").get());
+  provider.reset(new SimpleFeatureProvider(value.get(), NULL));
+  EXPECT_TRUE(provider->GetFeature("feature2"));
 }