Implement ComplexFeature to support permission features with multiple rules.

chrome/common/extensions/features/complex_feature_provider.cc

Index: chrome/common/extensions/features/complex_feature_provider.cc
diff --git a/chrome/common/extensions/features/complex_feature_provider.cc b/chrome/common/extensions/features/complex_feature_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..643c3262df8fcf2942d06aea39af2b60aaa2524e
--- /dev/null
+++ b/chrome/common/extensions/features/complex_feature_provider.cc
@@ -0,0 +1,128 @@
+// Copyright (c) 2012 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 "chrome/common/extensions/features/complex_feature_provider.h"
+
+#include "base/json/json_reader.h"
+#include "base/lazy_instance.h"
+#include "chrome/common/extensions/features/complex_feature.h"
+#include "chrome/common/extensions/features/permission_feature.h"
+#include "chrome/common/extensions/features/simple_feature_provider.h"
+#include "grit/common_resources.h"
+#include "ui/base/resource/resource_bundle.h"
+
+
+namespace extensions {
+
+namespace {
+
+struct Static {
+  Static()
+      : permission_features(
+            LoadProvider("permissions",
+                         IDR_EXTENSION_PERMISSION_FEATURES)) {
+  }
+
+  scoped_ptr<ComplexFeatureProvider> permission_features;
+
+ private:
+  scoped_ptr<ComplexFeatureProvider> LoadProvider(
+      const std::string& debug_string, int resource_id) {
+    std::string manifest_features =
+        ResourceBundle::GetSharedInstance().GetRawDataResource(
+            resource_id).as_string();
+    int error_code = 0;
+    std::string error_message;
+    Value* value = base::JSONReader::ReadAndReturnError(
+        manifest_features, base::JSON_PARSE_RFC,
+        &error_code, &error_message);
+    CHECK(value) << "Could not load features: " << debug_string << " "
+                 << error_message;
+    CHECK(value->IsType(Value::TYPE_DICTIONARY)) << debug_string;
+    scoped_ptr<DictionaryValue> dictionary_value(
+        static_cast<DictionaryValue*>(value));
+    return scoped_ptr<ComplexFeatureProvider>(

[not at google - send to devlin, 2012/12/12 17:42:41]

you can use make_scoped_ptr here and avoid template arguments

[justinlin, 2012/12/14 12:26:26]

Done.

+        new ComplexFeatureProvider(dictionary_value.get()));
+  }
+};
+
+base::LazyInstance<Static> g_static = LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+ComplexFeatureProvider::ComplexFeatureProvider(DictionaryValue* root) {
+  for (DictionaryValue::Iterator iter(*root); iter.HasNext(); iter.Advance()) {
+    if (iter.value().GetType() != Value::TYPE_LIST) {

[not at google - send to devlin, 2012/12/12 17:42:41]

It think it'd be simpler to have a single point for some feature provider, not
try to spread out the load between a SimpleFeatureProvider version of the
permission features and this complexfeatureprovider version. I don't see a
reason why ComplexFeatureProvider shouldn't know how to create SimpleFeatures
too?

[justinlin, 2012/12/14 12:26:26]

Done.

+      continue;
+    }
+
+    linked_ptr<ComplexFeature> feature(new ComplexFeature());

[not at google - send to devlin, 2012/12/12 17:42:41]

delay construction of this and collect simplefeatures in a vector rather than
using AddSimpleFeature - that way ComplexFeature can be created with its
features as a constant.

[justinlin, 2012/12/14 12:26:26]

Done.

+    feature->set_name(iter.key());
+
+    const ListValue* list = static_cast<const ListValue*>(&iter.value());
+    if (list->GetSize() == 0)
+      continue;

[not at google - send to devlin, 2012/12/12 17:42:41]

I'm not sure it makes sense to have an empty list. Perhaps DCHECK this?

[justinlin, 2012/12/14 12:26:26]

This is from "file input" though. Do we want to crash if the
{permission,manifest}_features_.json is malformed? Added a LOG(ERROR) for now.

[not at google - send to devlin, 2012/12/14 19:10:42]

Those files being malformed implies a bug in Chrome, and (D)CHECKing for it
increases our chance for finding it. So yes we should crash.

[justinlin, 2012/12/14 21:07:52]

Done. OK, added CHECK. If users aren't supposed to be able to modify it, then
makes sense.

+
+    // Parse and add all SimpleFeatures from the list.
+    for (ListValue::const_iterator list_iter = list->begin();
+         list_iter != list->end(); ++list_iter) {
+      if ((*list_iter)->GetType() != Value::TYPE_DICTIONARY) {
+        LOG(ERROR) << iter.key() << ": Feature rules must be dictionaries.";
+        continue;
+      }
+
+      scoped_ptr<PermissionFeature> simple_feature(new PermissionFeature());
+      simple_feature->set_name(iter.key());
+      simple_feature->Parse(static_cast<const DictionaryValue*>(*list_iter));
+
+      if (simple_feature->extension_types()->empty()) {
+        LOG(ERROR) << iter.key() << ": Simple features must specify at least "
+                   << "one value for extension_types.";
+        continue;

[not at google - send to devlin, 2012/12/12 17:42:41]

does SimpleFeature not do this validation itself?

[justinlin, 2012/12/14 12:26:26]

Done.

+      }
+
+      if (!simple_feature->contexts()->empty()) {
+        LOG(ERROR) << iter.key() << ": Simple features do not support "
+                                 << "contexts.";
+        continue;
+      }

[not at google - send to devlin, 2012/12/12 17:42:41]

why is that? Btw make this DCHECK so that we notice it during tests.

[justinlin, 2012/12/14 12:26:26]

Hmm.. I'm not sure if I want to attempt to move this check into SimpleFeature in
this CL. There might have been a valid reason (or maybe not). I see
SimpleFeatures created elsewhere, so maybe those instances don't require these
checks.

+
+      feature->AddSimpleFeature(simple_feature.release());
+    }
+
+    features_[iter.key()] = feature;
+  }
+}
+
+ComplexFeatureProvider::~ComplexFeatureProvider() {}
+
+// static
+ComplexFeatureProvider* ComplexFeatureProvider::GetPermissionFeatures() {
+  return g_static.Get().permission_features.get();
+}
+
+std::set<std::string> ComplexFeatureProvider::GetAllFeatureNames() const {
+  std::set<std::string> result;
+  for (FeatureMap::const_iterator iter = features_.begin();
+       iter != features_.end(); ++iter) {
+    result.insert(iter->first);
+  }
+
+  // Append all the SimpleFeatures as well.
+  std::set<std::string> simple_result =
+      SimpleFeatureProvider::GetPermissionFeatures()->GetAllFeatureNames();
+  result.insert(simple_result.begin(), simple_result.end());
+
+  return result;
+}
+
+Feature* ComplexFeatureProvider::GetFeature(const std::string& name) {
+  FeatureMap::iterator iter = features_.find(name);
+  if (iter != features_.end())
+    return iter->second.get();
+  else
+    return SimpleFeatureProvider::GetPermissionFeatures()->GetFeature(name);

[not at google - send to devlin, 2012/12/12 17:42:41]

See comment about about not spreading load

[justinlin, 2012/12/14 12:26:26]

Done.

+}
+
+}  // namespace