Refactor Uses of std::set to std::vector in SimpleFeature

Local profiling suggests that converting std::set to std::vector will save
around 10% of the overall startup time, likely due to the reduced allocs
involved with std::vector.

This change also reduces lookups by traversing the incoming dictionary
and iterating on the available values instead of looking through all
values for all features.

Finally, cleaned up and enforced the testing accessors via code and friend
declarations

BUG=460987

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

Cr-Commit-Position: refs/heads/master@{#322609}

base/stl_util.h

@@ -90,6 +90,14 @@ void STLDeleteContainerPairSecondPointers(ForwardIterator begin,
   }
 }
 
+// Counts the number of instances of val in a container.
+template <typename Container, typename T>
+typename std::iterator_traits<
+    typename Container::const_iterator>::difference_type
+STLCount(const Container& container, const T& val) {
+  return std::count(container.begin(), container.end(), val);
+}
+
 // To treat a possibly-empty vector as an array, use these functions.
 // If you know the array will never be empty, you can use &*v.begin()
 // directly, but that is undefined behaviour if |v| is empty.
@@ -195,6 +203,14 @@ bool ContainsKey(const Collection& collection, const Key& key) {
   return collection.find(key) != collection.end();
 }
 
+// Test to see if a collection like a vector contains a particular value.
+// Returns true if the value is in the collection.
+template <typename Collection, typename Value>
+bool ContainsValue(const Collection& collection, const Value& value) {
+  return std::find(collection.begin(), collection.end(), value) !=
+      collection.end();
+}
+
 namespace base {
 
 // Returns true if the container is sorted.

chrome/browser/content_settings/content_settings_internal_extension_provider.cc

@@ -138,10 +138,10 @@ void InternalExtensionProvider::Observe(
           "48CA541313139786F056DBCB504A1025CFF5D2E3",
           "05106136AE7F08A3C181D4648E5438350B1D2B4F"
         };
-        if (extensions::SimpleFeature::IsIdInList(
+        if (extensions::SimpleFeature::IsIdInArray(
                 host->extension()->id(),
-                std::set<std::string>(
-                    kAppWhitelist, kAppWhitelist + arraysize(kAppWhitelist)))) {
+                kAppWhitelist,
+                arraysize(kAppWhitelist))) {
           SetContentSettingForExtensionAndResource(
               host->extension(),
               ChromeContentClient::kRemotingViewerPluginPath,

chrome/browser/extensions/api/tab_capture/tab_capture_api.cc

@@ -91,10 +91,8 @@ bool TabCaptureCaptureFunction::RunSync() {
           APIPermission::kTabCaptureForTab) &&
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
           switches::kWhitelistedExtensionID) != extension_id &&
-      !SimpleFeature::IsIdInList(
-          extension_id,
-          std::set<std::string>(kWhitelist,
-                                kWhitelist + arraysize(kWhitelist)))) {
+      !SimpleFeature::IsIdInArray(
+          extension_id, kWhitelist, arraysize(kWhitelist))) {
     error_ = kGrantError;
     return false;
   }

chrome/browser/extensions/external_component_loader.cc

@@ -41,11 +41,8 @@ bool ExternalComponentLoader::IsModifiable(const Extension* extension) {
         "D57DE394F36DC1C3220E7604C575D29C51A6C495",  // http://crbug.com/319444
         "3F65507A3B39259B38C8173C6FFA3D12DF64CCE9"   // http://crbug.com/371562
     };
-    return SimpleFeature::IsIdInList(
-        extension->id(),
-        std::set<std::string>(
-            kEnhancedExtensions,
-            kEnhancedExtensions + arraysize(kEnhancedExtensions)));
+    return SimpleFeature::IsIdInArray(
+        extension->id(), kEnhancedExtensions, arraysize(kEnhancedExtensions));
   }
   return false;
 }

extensions/browser/api/app_window/app_window_api.cc

@@ -263,10 +263,8 @@ bool AppWindowCreateFunction::RunAsync() {
         "0F585FB1D0FDFBEBCE1FEB5E9DFFB6DA476B8C9B"
       };
       if (AppWindowClient::Get()->IsCurrentChannelOlderThanDev() &&
-          !SimpleFeature::IsIdInList(
-              extension_id(),
-              std::set<std::string>(kWhitelist,
-                                    kWhitelist + arraysize(kWhitelist)))) {
+          !SimpleFeature::IsIdInArray(
+              extension_id(), kWhitelist, arraysize(kWhitelist))) {
         error_ = app_window_constants::kAlphaEnabledWrongChannel;
         return false;
       }

extensions/common/features/base_feature_provider_unittest.cc

@@ -4,9 +4,11 @@
 
 #include "extensions/common/features/base_feature_provider.h"
 
+#include <algorithm>
 #include <set>
 #include <string>
 
+#include "base/stl_util.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/features/feature.h"
 #include "extensions/common/features/simple_feature.h"
@@ -24,14 +26,16 @@ TEST(BaseFeatureProviderTest, ManifestFeatureTypes) {
   const SimpleFeature* feature = static_cast<const SimpleFeature*>(
       FeatureProvider::GetManifestFeature("description"));
   ASSERT_TRUE(feature);
-  const std::set<Manifest::Type>* extension_types = feature->extension_types();
+  const std::vector<Manifest::Type>* extension_types =
+      feature->extension_types();
   EXPECT_EQ(6u, extension_types->size());
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_EXTENSION));
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_LEGACY_PACKAGED_APP));
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_PLATFORM_APP));
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_HOSTED_APP));
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_THEME));
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_SHARED_MODULE));
+  EXPECT_EQ(1, STLCount(*(extension_types), Manifest::TYPE_EXTENSION));
+  EXPECT_EQ(1,
+            STLCount(*(extension_types), Manifest::TYPE_LEGACY_PACKAGED_APP));
+  EXPECT_EQ(1, STLCount(*(extension_types), Manifest::TYPE_PLATFORM_APP));
+  EXPECT_EQ(1, STLCount(*(extension_types), Manifest::TYPE_HOSTED_APP));
+  EXPECT_EQ(1, STLCount(*(extension_types), Manifest::TYPE_THEME));
+  EXPECT_EQ(1, STLCount(*(extension_types), Manifest::TYPE_SHARED_MODULE));
 }
 
 // Tests that real manifest features have the correct availability for an
@@ -79,11 +83,13 @@ TEST(BaseFeatureProviderTest, PermissionFeatureTypes) {
   const SimpleFeature* feature = static_cast<const SimpleFeature*>(
       BaseFeatureProvider::GetPermissionFeature("power"));
   ASSERT_TRUE(feature);
-  const std::set<Manifest::Type>* extension_types = feature->extension_types();
+  const std::vector<Manifest::Type>* extension_types =
+      feature->extension_types();
   EXPECT_EQ(3u, extension_types->size());
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_EXTENSION));
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_LEGACY_PACKAGED_APP));
-  EXPECT_EQ(1u, extension_types->count(Manifest::TYPE_PLATFORM_APP));
+  EXPECT_EQ(1, STLCount(*(extension_types), Manifest::TYPE_EXTENSION));
+  EXPECT_EQ(1,
+            STLCount(*(extension_types), Manifest::TYPE_LEGACY_PACKAGED_APP));
+  EXPECT_EQ(1, STLCount(*(extension_types), Manifest::TYPE_PLATFORM_APP));
 }
 
 // Tests that real permission features have the correct availability for an app.

extensions/common/features/simple_feature.h

@@ -11,6 +11,7 @@
 
 #include "base/callback_forward.h"
 #include "base/gtest_prod_util.h"
+#include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/values.h"
@@ -21,53 +22,16 @@
 
 namespace extensions {
 
+class BaseFeatureProviderTest;
+class ExtensionAPITest;
+class ManifestUnitTest;
+class SimpleFeatureTest;
+
 class SimpleFeature : public Feature {
  public:
   SimpleFeature();
   ~SimpleFeature() override;
 
-  // Similar to Manifest::Location, these are the classes of locations
-  // supported in feature files.
-  //
-  // This is only public for testing. Production code should never access it,
-  // nor should it really have any reason to access the SimpleFeature class
-  // directly, it should be dealing with the Feature interface.
-  enum Location {
-    UNSPECIFIED_LOCATION,
-    COMPONENT_LOCATION,
-    EXTERNAL_COMPONENT_LOCATION,
-    POLICY_LOCATION,
-  };
-
-  // Accessors defined for testing. See comment above about not directly using
-  // SimpleFeature in production code.
-  std::set<std::string>* blacklist() { return &blacklist_; }
-  const std::set<std::string>* blacklist() const { return &blacklist_; }
-  std::set<std::string>* whitelist() { return &whitelist_; }
-  const std::set<std::string>* whitelist() const { return &whitelist_; }
-  std::set<Manifest::Type>* extension_types() { return &extension_types_; }
-  const std::set<Manifest::Type>* extension_types() const {
-    return &extension_types_;
-  }
-  std::set<Context>* contexts() { return &contexts_; }
-  const std::set<Context>* contexts() const { return &contexts_; }
-  Location location() const { return location_; }
-  void set_location(Location location) { location_ = location; }
-  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;
-  }
-  const std::string& command_line_switch() const {
-    return command_line_switch_;
-  }
-  void set_command_line_switch(const std::string& command_line_switch) {
-    command_line_switch_ = command_line_switch;
-  }
-
   // Dependency resolution is a property of Features that is preferrably
   // handled internally to avoid temptation, but FeatureFilters may need
   // to know if there are any at all.
@@ -80,9 +44,7 @@ class SimpleFeature : public Feature {
   // Unspecified values in the JSON are not modified in the object. This allows
   // us to implement inheritance by parsing one value after another. Returns
   // the error found, or an empty string on success.
-  virtual std::string Parse(const base::DictionaryValue* value);
-
-  std::set<Platform>* platforms() { return &platforms_; }
+  virtual std::string Parse(const base::DictionaryValue* dictionary);
 
   Availability IsAvailableToContext(const Extension* extension,
                                     Context context) const {
@@ -120,10 +82,51 @@ class SimpleFeature : public Feature {
 
   bool IsIdInBlacklist(const std::string& extension_id) const override;
   bool IsIdInWhitelist(const std::string& extension_id) const override;
-  static bool IsIdInList(const std::string& extension_id,
-                         const std::set<std::string>& list);
+
+  static bool IsIdInArray(const std::string& extension_id,
+                          const char* const array[],
+                          size_t array_length);
 
  protected:
+  // Similar to Manifest::Location, these are the classes of locations
+  // supported in feature files. Production code should never directly access
+  // these.
+  enum Location {
+    UNSPECIFIED_LOCATION,
+    COMPONENT_LOCATION,
+    EXTERNAL_COMPONENT_LOCATION,
+    POLICY_LOCATION,
+  };
+
+  // Accessors defined for testing.
+  std::vector<std::string>* blacklist() { return &blacklist_; }
+  const std::vector<std::string>* blacklist() const { return &blacklist_; }
+  std::vector<std::string>* whitelist() { return &whitelist_; }
+  const std::vector<std::string>* whitelist() const { return &whitelist_; }
+  std::vector<Manifest::Type>* extension_types() { return &extension_types_; }
+  const std::vector<Manifest::Type>* extension_types() const {
+    return &extension_types_;
+  }
+  std::vector<Context>* contexts() { return &contexts_; }
+  const std::vector<Context>* contexts() const { return &contexts_; }
+  std::vector<Platform>* platforms() { return &platforms_; }
+  Location location() const { return location_; }
+  void set_location(Location location) { location_ = location; }
+  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;
+  }
+  const std::string& command_line_switch() const {
+    return command_line_switch_;
+  }
+  void set_command_line_switch(const std::string& command_line_switch) {
+    command_line_switch_ = command_line_switch;
+  }
+
   // Handy utilities which construct the correct availability message.
   Availability CreateAvailability(AvailabilityResult result) const;
   Availability CreateAvailability(AvailabilityResult result,
@@ -134,23 +137,55 @@ class SimpleFeature : public Feature {
                                   Context context) const;
 
  private:
+  friend class SimpleFeatureTest;
+  FRIEND_TEST_ALL_PREFIXES(BaseFeatureProviderTest, ManifestFeatureTypes);
+  FRIEND_TEST_ALL_PREFIXES(BaseFeatureProviderTest, PermissionFeatureTypes);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionAPITest, DefaultConfigurationFeatures);
+  FRIEND_TEST_ALL_PREFIXES(ManifestUnitTest, Extension);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Blacklist);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, CommandLineSwitch);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Context);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, HashedIdBlacklist);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, HashedIdWhitelist);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Inheritance);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Location);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ManifestVersion);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, PackageType);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParseContexts);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParseLocation);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParseManifestVersion);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParseNull);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParsePackageTypes);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParsePlatforms);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParseWhitelist);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Platform);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Whitelist);
+
+  // Holds String to Enum value mappings.
+  struct Mappings;
+
+  static bool IsIdInList(const std::string& extension_id,
+                         const std::vector<std::string>& list);
+
   bool MatchesManifestLocation(Manifest::Location manifest_location) const;
 
   Availability CheckDependencies(
       const base::Callback<Availability(const Feature*)>& checker) const;
 
+  static bool IsValidExtensionId(const std::string& extension_id);
+
   // For clarity 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> blacklist_;
-  std::set<std::string> whitelist_;
-  std::set<std::string> dependencies_;
-  std::set<Manifest::Type> extension_types_;
-  std::set<Context> contexts_;
+  std::vector<std::string> blacklist_;
+  std::vector<std::string> whitelist_;
+  std::vector<std::string> dependencies_;
+  std::vector<Manifest::Type> extension_types_;
+  std::vector<Context> contexts_;
+  std::vector<Platform> platforms_;
   URLPatternSet matches_;
   Location location_;
-  std::set<Platform> platforms_;
   int min_manifest_version_;
   int max_manifest_version_;
   bool component_extensions_auto_granted_;