Remove GetContexts() from the public interface of extensions::Feature. It was

being misused in such a way that makes it impossible to add other trusted
extension context types. It's also a nice cleanup, though requires rewriting
the ExtensionAPI::IsPrivileged function with sightly different semantics.

BUG=391944
R=rockot@chromium.org

Review URL: https://codereview.chromium.org/377753003

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

9 files changed, 36 insertions, 57 deletions

diff --git a/extensions/common/features/api_feature.cc b/extensions/common/features/api_feature.cc
index 4a4ac1c..fc7f707 100644
--- a/extensions/common/features/api_feature.cc
+++ b/extensions/common/features/api_feature.cc
@@ -28,7 +28,7 @@ std::string APIFeature::Parse(const base::DictionaryValue* value) {
   value->GetBoolean("internal", &internal_);
   value->GetBoolean("blocked_in_service_worker", &blocked_in_service_worker_);
 
-  if (GetContexts()->empty())
+  if (contexts()->empty())
     return name() + ": API features must specify at least one context.";
 
   return std::string();
diff --git a/extensions/common/features/complex_feature.cc b/extensions/common/features/complex_feature.cc
index eeb4f77..9440285 100644
--- a/extensions/common/features/complex_feature.cc
+++ b/extensions/common/features/complex_feature.cc
@@ -12,18 +12,14 @@ ComplexFeature::ComplexFeature(scoped_ptr<FeatureList> features) {
   no_parent_ = features_[0]->no_parent();
 
 #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)
-  // Verify GetContexts, IsInternal, & IsBlockedInServiceWorker are consistent
-  // across all features.
-  std::set<Feature::Context>* first_contexts = features_[0]->GetContexts();
+  // Verify IsInternal and IsBlockedInServiceWorker are consistent across all
+  // features.
   bool first_is_internal = features_[0]->IsInternal();
   bool first_blocked_in_service_worker =
       features_[0]->IsBlockedInServiceWorker();
   for (FeatureList::const_iterator it = features_.begin() + 1;
        it != features_.end();
        ++it) {
-    DCHECK(*first_contexts == *(*it)->GetContexts())
-        << "Complex feature must have consistent values of "
-           "contexts across all sub features.";
     DCHECK(first_is_internal == (*it)->IsInternal())
         << "Complex feature must have consistent values of "
            "internal across all sub features.";
@@ -112,16 +108,9 @@ bool ComplexFeature::IsBlockedInServiceWorker() const {
   return features_[0]->IsBlockedInServiceWorker();
 }
 
-std::set<Feature::Context>* ComplexFeature::GetContexts() {
-  // TODO(justinlin): Current use cases for ComplexFeatures are simple (e.g.
-  // allow API in dev channel for everyone but stable channel for a whitelist),
-  // but if they get more complicated, we need to return some meaningful context
-  // set. Either that or remove this method from the Feature interface.
-  return features_[0]->GetContexts();
-}
-
 bool ComplexFeature::IsInternal() const {
-  // TODO(justinlin): Same as the above TODO.
+  // Constructor verifies that composed features are consistent, thus we can
+  // return just the first feature's value.
   return features_[0]->IsInternal();
 }
 
diff --git a/extensions/common/features/complex_feature.h b/extensions/common/features/complex_feature.h
index e71f6da..9204b25 100644
--- a/extensions/common/features/complex_feature.h
+++ b/extensions/common/features/complex_feature.h
@@ -49,8 +49,6 @@ class ComplexFeature : public Feature {
       const GURL& url,
       Context context) const OVERRIDE;
 
-  virtual std::set<Context>* GetContexts() OVERRIDE;
-
   virtual bool IsInternal() const OVERRIDE;
 
  private:
diff --git a/extensions/common/features/feature.h b/extensions/common/features/feature.h
index 534a05f..37387d7 100644
--- a/extensions/common/features/feature.h
+++ b/extensions/common/features/feature.h
@@ -108,8 +108,6 @@ class Feature {
   // Gets the platform the code is currently running on.
   static Platform GetCurrentPlatform();
 
-  virtual std::set<Context>* GetContexts() = 0;
-
   // Tests whether this is an internal API or not.
   virtual bool IsInternal() const = 0;
 
diff --git a/extensions/common/features/manifest_feature.cc b/extensions/common/features/manifest_feature.cc
index 0a4ad9b..f169c6a 100644
--- a/extensions/common/features/manifest_feature.cc
+++ b/extensions/common/features/manifest_feature.cc
@@ -44,7 +44,7 @@ std::string ManifestFeature::Parse(const base::DictionaryValue* value) {
         "value for extension_types.";
   }
 
-  if (!GetContexts()->empty())
+  if (value->HasKey("contexts"))
     return name() + ": Manifest features do not support contexts.";
 
   return std::string();
diff --git a/extensions/common/features/permission_feature.cc b/extensions/common/features/permission_feature.cc
index 82f676bf..529ecd1 100644
--- a/extensions/common/features/permission_feature.cc
+++ b/extensions/common/features/permission_feature.cc
@@ -43,7 +43,7 @@ std::string PermissionFeature::Parse(const base::DictionaryValue* value) {
         "value for extension_types.";
   }
 
-  if (!GetContexts()->empty())
+  if (value->HasKey("contexts"))
     return name() + ": Permission features do not support contexts.";
 
   return std::string();
diff --git a/extensions/common/features/simple_feature.cc b/extensions/common/features/simple_feature.cc
index 762730b1f..155e764 100644
--- a/extensions/common/features/simple_feature.cc
+++ b/extensions/common/features/simple_feature.cc
@@ -287,10 +287,14 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* value) {
   value->GetBoolean("component_extensions_auto_granted",
                     &component_extensions_auto_granted_);
 
-  if (matches_.is_empty() && contexts_.count(WEB_PAGE_CONTEXT) != 0) {
-    return name() + ": Allowing web_page contexts requires supplying a value " +
-        "for matches.";
-  }
+  // NOTE: ideally we'd sanity check that "matches" can be specified if and
+  // only if there's a "web_page" 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.
 
   for (FilterList::iterator filter_iter = filters_.begin();
        filter_iter != filters_.end();
@@ -395,7 +399,7 @@ Feature::Availability SimpleFeature::IsAvailableToContext(
   if (!contexts_.empty() && contexts_.find(context) == contexts_.end())
     return CreateAvailability(INVALID_CONTEXT, context);
 
-  if (!matches_.is_empty() && !matches_.MatchesURL(url))
+  if (context == WEB_PAGE_CONTEXT && !matches_.MatchesURL(url))
     return CreateAvailability(INVALID_URL, url);
 
   for (FilterList::const_iterator filter_iter = filters_.begin();
@@ -502,10 +506,6 @@ Feature::Availability SimpleFeature::CreateAvailability(
                                      context));
 }
 
-std::set<Feature::Context>* SimpleFeature::GetContexts() {
-  return &contexts_;
-}
-
 bool SimpleFeature::IsInternal() const {
   return false;
 }
diff --git a/extensions/common/features/simple_feature.h b/extensions/common/features/simple_feature.h
index 34ae5b5..7df6e19 100644
--- a/extensions/common/features/simple_feature.h
+++ b/extensions/common/features/simple_feature.h
@@ -55,6 +55,7 @@ class SimpleFeature : public Feature {
   std::set<std::string>* blacklist() { return &blacklist_; }
   std::set<std::string>* whitelist() { return &whitelist_; }
   std::set<Manifest::Type>* extension_types() { return &extension_types_; }
+  std::set<Context>* contexts() { return &contexts_; }
 
   // Dependency resolution is a property of Features that is preferrably
   // handled internally to avoid temptation, but FeatureFilters may need
@@ -104,8 +105,6 @@ class SimpleFeature : public Feature {
                                              const GURL& url,
                                              Context context) const OVERRIDE;
 
-  virtual std::set<Context>* GetContexts() OVERRIDE;
-
   virtual bool IsInternal() const OVERRIDE;
   virtual bool IsBlockedInServiceWorker() const OVERRIDE;
 
diff --git a/extensions/common/features/simple_feature_unittest.cc b/extensions/common/features/simple_feature_unittest.cc
index 8ba5dfa..92d2f4d 100644
--- a/extensions/common/features/simple_feature_unittest.cc
+++ b/extensions/common/features/simple_feature_unittest.cc
@@ -289,7 +289,7 @@ TEST_F(ExtensionSimpleFeatureTest, PackageType) {
 TEST_F(ExtensionSimpleFeatureTest, Context) {
   SimpleFeature feature;
   feature.set_name("somefeature");
-  feature.GetContexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
+  feature.contexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
   feature.extension_types()->insert(Manifest::TYPE_LEGACY_PACKAGED_APP);
   feature.platforms()->insert(Feature::CHROMEOS_PLATFORM);
   feature.set_min_manifest_version(21);
@@ -328,9 +328,9 @@ TEST_F(ExtensionSimpleFeatureTest, Context) {
 
   feature.extension_types()->clear();
   feature.extension_types()->insert(Manifest::TYPE_LEGACY_PACKAGED_APP);
-  feature.GetContexts()->clear();
-  feature.GetContexts()->insert(Feature::UNBLESSED_EXTENSION_CONTEXT);
-  feature.GetContexts()->insert(Feature::CONTENT_SCRIPT_CONTEXT);
+  feature.contexts()->clear();
+  feature.contexts()->insert(Feature::UNBLESSED_EXTENSION_CONTEXT);
+  feature.contexts()->insert(Feature::CONTENT_SCRIPT_CONTEXT);
   {
     Feature::Availability availability = feature.IsAvailableToContext(
         extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
@@ -341,7 +341,7 @@ TEST_F(ExtensionSimpleFeatureTest, Context) {
               availability.message());
   }
 
-  feature.GetContexts()->insert(Feature::WEB_PAGE_CONTEXT);
+  feature.contexts()->insert(Feature::WEB_PAGE_CONTEXT);
   {
     Feature::Availability availability = feature.IsAvailableToContext(
         extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
@@ -352,8 +352,8 @@ TEST_F(ExtensionSimpleFeatureTest, Context) {
               availability.message());
   }
 
-  feature.GetContexts()->clear();
-  feature.GetContexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
+  feature.contexts()->clear();
+  feature.contexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
   feature.set_location(SimpleFeature::COMPONENT_LOCATION);
   EXPECT_EQ(Feature::INVALID_LOCATION, feature.IsAvailableToContext(
       extension.get(), Feature::BLESSED_EXTENSION_CONTEXT,
@@ -496,7 +496,7 @@ TEST_F(ExtensionSimpleFeatureTest, ParseNull) {
   feature->Parse(value.get());
   EXPECT_TRUE(feature->whitelist()->empty());
   EXPECT_TRUE(feature->extension_types()->empty());
-  EXPECT_TRUE(feature->GetContexts()->empty());
+  EXPECT_TRUE(feature->contexts()->empty());
   EXPECT_EQ(SimpleFeature::UNSPECIFIED_LOCATION, feature->location());
   EXPECT_TRUE(feature->platforms()->empty());
   EXPECT_EQ(0, feature->min_manifest_version());
@@ -554,22 +554,17 @@ TEST_F(ExtensionSimpleFeatureTest, ParseContexts) {
   value->Set("contexts", contexts);
   scoped_ptr<SimpleFeature> feature(new SimpleFeature());
   feature->Parse(value.get());
-  EXPECT_EQ(5u, feature->GetContexts()->size());
-  EXPECT_TRUE(
-      feature->GetContexts()->count(Feature::BLESSED_EXTENSION_CONTEXT));
-  EXPECT_TRUE(
-      feature->GetContexts()->count(Feature::UNBLESSED_EXTENSION_CONTEXT));
-  EXPECT_TRUE(
-      feature->GetContexts()->count(Feature::CONTENT_SCRIPT_CONTEXT));
-  EXPECT_TRUE(
-      feature->GetContexts()->count(Feature::WEB_PAGE_CONTEXT));
-  EXPECT_TRUE(
-      feature->GetContexts()->count(Feature::BLESSED_WEB_PAGE_CONTEXT));
+  EXPECT_EQ(5u, feature->contexts()->size());
+  EXPECT_TRUE(feature->contexts()->count(Feature::BLESSED_EXTENSION_CONTEXT));
+  EXPECT_TRUE(feature->contexts()->count(Feature::UNBLESSED_EXTENSION_CONTEXT));
+  EXPECT_TRUE(feature->contexts()->count(Feature::CONTENT_SCRIPT_CONTEXT));
+  EXPECT_TRUE(feature->contexts()->count(Feature::WEB_PAGE_CONTEXT));
+  EXPECT_TRUE(feature->contexts()->count(Feature::BLESSED_WEB_PAGE_CONTEXT));
 
   value->SetString("contexts", "all");
   scoped_ptr<SimpleFeature> feature2(new SimpleFeature());
   feature2->Parse(value.get());
-  EXPECT_EQ(*(feature->GetContexts()), *(feature2->GetContexts()));
+  EXPECT_EQ(*(feature->contexts()), *(feature2->contexts()));
 }
 
 TEST_F(ExtensionSimpleFeatureTest, ParseLocation) {
@@ -625,7 +620,7 @@ TEST_F(ExtensionSimpleFeatureTest, Inheritance) {
   SimpleFeature feature;
   feature.whitelist()->insert("foo");
   feature.extension_types()->insert(Manifest::TYPE_THEME);
-  feature.GetContexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
+  feature.contexts()->insert(Feature::BLESSED_EXTENSION_CONTEXT);
   feature.set_location(SimpleFeature::COMPONENT_LOCATION);
   feature.platforms()->insert(Feature::CHROMEOS_PLATFORM);
   feature.set_min_manifest_version(1);
@@ -637,7 +632,7 @@ TEST_F(ExtensionSimpleFeatureTest, Inheritance) {
   feature.Parse(&definition);
   EXPECT_EQ(1u, feature.whitelist()->size());
   EXPECT_EQ(1u, feature.extension_types()->size());
-  EXPECT_EQ(1u, feature.GetContexts()->size());
+  EXPECT_EQ(1u, feature.contexts()->size());
   EXPECT_EQ(1u, feature.whitelist()->count("foo"));
   EXPECT_EQ(SimpleFeature::COMPONENT_LOCATION, feature.location());
   EXPECT_EQ(1u, feature.platforms()->size());
@@ -661,11 +656,11 @@ TEST_F(ExtensionSimpleFeatureTest, Inheritance) {
   feature.Parse(&definition);
   EXPECT_EQ(1u, feature.whitelist()->size());
   EXPECT_EQ(1u, feature.extension_types()->size());
-  EXPECT_EQ(1u, feature.GetContexts()->size());
+  EXPECT_EQ(1u, feature.contexts()->size());
   EXPECT_EQ(1u, feature.whitelist()->count("bar"));
   EXPECT_EQ(1u, feature.extension_types()->count(Manifest::TYPE_EXTENSION));
   EXPECT_EQ(1u,
-            feature.GetContexts()->count(Feature::UNBLESSED_EXTENSION_CONTEXT));
+            feature.contexts()->count(Feature::UNBLESSED_EXTENSION_CONTEXT));
   EXPECT_EQ(2, feature.min_manifest_version());
   EXPECT_EQ(3, feature.max_manifest_version());
 }