Implement ComplexFeature to support permission features with multiple rules.

chrome/common/extensions/features/feature.h

Index: chrome/common/extensions/features/feature.h
diff --git a/chrome/common/extensions/features/feature.h b/chrome/common/extensions/features/feature.h
index 41711835c9972685793281e50dfef9015090cb26..bfb7ccbd36ed88d6d46c86eff52f42d0c767bd61 100644
--- a/chrome/common/extensions/features/feature.h
+++ b/chrome/common/extensions/features/feature.h
@@ -83,9 +83,8 @@ class Feature {
     const std::string message_;
   };
 
-  Feature();
-  Feature(const Feature& other);
-  virtual ~Feature();
+  Feature() {}

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

If the constructor is just this then it can be left out

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

Done.

+  virtual ~Feature() {}

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

IIRC virtual constructors in non-pure-virtual classes shouldn't be inlined, and
there are those CreateAvailability methods. If you can make those pure-virtual
(or move them someplace else) then that'd be cool, but nbd if not. I just don't
think this can be inline-virtual.

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

Done. Right, put it in the cc for now.
I moved the CreateAvailability functions to SimpleFeature, maybe that makes more
sense since they're more like helpers.

The get/set_name are in this class, so it won't be pure virtual anyway.

 
   // Gets the current channel as seen by the Feature system.
   static chrome::VersionInfo::Channel GetCurrentChannel();
@@ -124,33 +123,7 @@ class Feature {
   // Gets the Feature::Location value for the specified Extension::Location.
   static Location ConvertLocation(Extension::Location extension_location);
 
-  std::set<std::string>* whitelist() { return &whitelist_; }
-  std::set<Extension::Type>* extension_types() { return &extension_types_; }
-  std::set<Context>* contexts() { return &contexts_; }
-
-  Location location() const { return location_; }
-  void set_location(Location location) { location_ = location; }
-
-  Platform platform() const { return platform_; }
-  void set_platform(Platform platform) { platform_ = platform; }
-
-  int min_manifest_version() const { return min_manifest_version_; }
-  void set_min_manifest_version(int min_manifest_version) {
-    min_manifest_version_ = min_manifest_version;
-  }
-
-  int max_manifest_version() const { return max_manifest_version_; }
-  void set_max_manifest_version(int max_manifest_version) {
-    max_manifest_version_ = max_manifest_version;
-  }
-
-  // Parses the JSON representation of a feature into the fields of this object.
-  // Unspecified values in the JSON are not modified in the object. This allows
-  // us to implement inheritance by parsing one value after another.
-  void Parse(const DictionaryValue* value);
-
-  // Returns true if the feature contains the same values as another.
-  bool Equals(const Feature& other) const;
+  virtual std::set<Context>* contexts() = 0;

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

Generally speaking you can't guarantee that virtual methods have trivial
implementations, which is what calling this contexts() rather than GetContexts()
implies. One reasonable exception is for when things are virtual for testing.

But stepping back: why does this need to be on the interface for Feature?

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

extension_api.cc needs this method in a couple places (3) where it doesn't look
like it's safe to case to SimpleFeature. Renamed. I think this maybe can be
removed eventually by somebody who fully understands the logic in extension_api.
I didn't want to change it and risk breaking something yet.

 
   // Returns true if the feature is available to be parsed into a new extension
   // manifest.
@@ -161,11 +134,11 @@ class Feature {
     return IsAvailableToManifest(extension_id, type, location, manifest_version,
                                  GetCurrentPlatform());
   }
-  Availability IsAvailableToManifest(const std::string& extension_id,
-                                     Extension::Type type,
-                                     Location location,
-                                     int manifest_version,
-                                     Platform platform) const;
+  virtual Availability IsAvailableToManifest(const std::string& extension_id,
+                                             Extension::Type type,
+                                             Location location,
+                                             int manifest_version,
+                                             Platform platform) const = 0;
 
   // Returns true if the feature is available to be used in the specified
   // extension and context.
@@ -175,31 +148,17 @@ class Feature {
   }
   virtual Availability IsAvailableToContext(const Extension* extension,
                                             Context context,
-                                            Platform platform) const;
+                                            Platform platform) const = 0;
 
  protected:
   Availability CreateAvailability(AvailabilityResult result) const;
   Availability CreateAvailability(AvailabilityResult result,
                                   Extension::Type type) const;
 
- private:
-  std::string GetAvailabilityMessage(
-      AvailabilityResult result, Extension::Type type) const;
+  virtual std::string GetAvailabilityMessage(
+      AvailabilityResult result, Extension::Type type) const = 0;
 
   std::string name_;
-
-  // For clarify and consistency, we handle the default value of each of these
-  // members the same way: it matches everything. It is up to the higher level
-  // code that reads Features out of static data to validate that data and set
-  // sensible defaults.
-  std::set<std::string> whitelist_;
-  std::set<Extension::Type> extension_types_;
-  std::set<Context> contexts_;
-  Location location_;  // we only care about component/not-component now
-  Platform platform_;  // we only care about chromeos/not-chromeos now
-  int min_manifest_version_;
-  int max_manifest_version_;
-  chrome::VersionInfo::Channel channel_;
 };
 
 }  // namespace extensions