Cache --whitelisted-extension-id in SimpleFeature.

During startup, CommandLine::HasSwitch(kWhitelistedExtensionID) is
called ~1000 times by SimpleFeature. It is the most common switch
lookup, the second being kAppsGalleryUpdateUrl at ~300 times.

Caching this avoids allocating and copying the string each time.

BUG=471976

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

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

6 files changed, 98 insertions, 36 deletions

diff --git a/chrome/browser/extensions/permission_message_combinations_unittest.cc b/chrome/browser/extensions/permission_message_combinations_unittest.cc
index 42764bb..d4024e5 100644
--- a/chrome/browser/extensions/permission_message_combinations_unittest.cc
+++ b/chrome/browser/extensions/permission_message_combinations_unittest.cc
@@ -8,6 +8,7 @@
 #include "chrome/browser/extensions/test_extension_environment.h"
 #include "chrome/common/extensions/permissions/chrome_permission_message_provider.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/features/simple_feature.h"
 #include "extensions/common/permissions/permission_message_test_util.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/common/switches.h"
@@ -16,12 +17,15 @@
 
 namespace extensions {
 
+const char kWhitelistedExtensionID[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+
 // Tests that ChromePermissionMessageProvider produces the expected messages for
 // various combinations of app/extension permissions.
 class PermissionMessageCombinationsUnittest : public testing::Test {
  public:
   PermissionMessageCombinationsUnittest()
-      : message_provider_(new ChromePermissionMessageProvider()) {}
+      : message_provider_(new ChromePermissionMessageProvider()),
+        whitelisted_extension_id_(kWhitelistedExtensionID) {}
   ~PermissionMessageCombinationsUnittest() override {}
 
   // Overridden from testing::Test:
@@ -39,10 +43,8 @@ class PermissionMessageCombinationsUnittest : public testing::Test {
     std::replace(json_manifest_with_double_quotes.begin(),
                  json_manifest_with_double_quotes.end(), '\'', '"');
     app_ = env_.MakeExtension(
-        *base::test::ParseJson(json_manifest_with_double_quotes));
-    // Add the app to any whitelists so we can test all permissions.
-    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-        switches::kWhitelistedExtensionID, app_->id());
+        *base::test::ParseJson(json_manifest_with_double_quotes),
+        kWhitelistedExtensionID);
   }
 
   // Checks whether the currently installed app or extension produces the given
@@ -197,6 +199,9 @@ class PermissionMessageCombinationsUnittest : public testing::Test {
   extensions::TestExtensionEnvironment env_;
   scoped_ptr<ChromePermissionMessageProvider> message_provider_;
   scoped_refptr<const Extension> app_;
+  // Whitelist a known extension id so we can test all permissions. This ID
+  // will be used for each test app.
+  extensions::SimpleFeature::ScopedWhitelistForTest whitelisted_extension_id_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionMessageCombinationsUnittest);
 };
diff --git a/chrome/browser/extensions/test_extension_environment.cc b/chrome/browser/extensions/test_extension_environment.cc
index 8b5c30e..f8478fc 100644
--- a/chrome/browser/extensions/test_extension_environment.cc
+++ b/chrome/browser/extensions/test_extension_environment.cc
@@ -27,6 +27,28 @@ namespace extensions {
 
 using content::BrowserThread;
 
+namespace {
+
+scoped_ptr<base::DictionaryValue> MakeExtensionManifest(
+    const base::Value& manifest_extra) {
+  scoped_ptr<base::DictionaryValue> manifest = DictionaryBuilder()
+                                                   .Set("name", "Extension")
+                                                   .Set("version", "1.0")
+                                                   .Set("manifest_version", 2)
+                                                   .Build();
+  const base::DictionaryValue* manifest_extra_dict;
+  if (manifest_extra.GetAsDictionary(&manifest_extra_dict)) {
+    manifest->MergeDictionary(manifest_extra_dict);
+  } else {
+    std::string manifest_json;
+    base::JSONWriter::Write(&manifest_extra, &manifest_json);
+    ADD_FAILURE() << "Expected dictionary; got \"" << manifest_json << "\"";
+  }
+  return manifest;
+}
+
+}  // namespace
+
 TestExtensionEnvironment::TestExtensionEnvironment()
     : profile_(new TestingProfile),
       extension_service_(NULL),
@@ -68,26 +90,25 @@ ExtensionPrefs* TestExtensionEnvironment::GetExtensionPrefs() {
 
 const Extension* TestExtensionEnvironment::MakeExtension(
     const base::Value& manifest_extra) {
-  scoped_ptr<base::DictionaryValue> manifest = DictionaryBuilder()
-      .Set("name", "Extension")
-      .Set("version", "1.0")
-      .Set("manifest_version", 2)
-      .Build();
-  const base::DictionaryValue* manifest_extra_dict;
-  if (manifest_extra.GetAsDictionary(&manifest_extra_dict)) {
-    manifest->MergeDictionary(manifest_extra_dict);
-  } else {
-    std::string manifest_json;
-    base::JSONWriter::Write(&manifest_extra, &manifest_json);
-    ADD_FAILURE() << "Expected dictionary; got \"" << manifest_json << "\"";
-  }
-
+  scoped_ptr<base::DictionaryValue> manifest =
+      MakeExtensionManifest(manifest_extra);
   scoped_refptr<Extension> result =
       ExtensionBuilder().SetManifest(manifest.Pass()).Build();
   GetExtensionService()->AddExtension(result.get());
   return result.get();
 }
 
+const Extension* TestExtensionEnvironment::MakeExtension(
+    const base::Value& manifest_extra,
+    const std::string& id) {
+  scoped_ptr<base::DictionaryValue> manifest =
+      MakeExtensionManifest(manifest_extra);
+  scoped_refptr<Extension> result =
+      ExtensionBuilder().SetManifest(manifest.Pass()).SetID(id).Build();
+  GetExtensionService()->AddExtension(result.get());
+  return result.get();
+}
+
 scoped_ptr<content::WebContents> TestExtensionEnvironment::MakeTab() const {
   scoped_ptr<content::WebContents> contents(
       content::WebContentsTester::CreateTestWebContents(profile(), NULL));
diff --git a/chrome/browser/extensions/test_extension_environment.h b/chrome/browser/extensions/test_extension_environment.h
index 57136588..bea77e1 100644
--- a/chrome/browser/extensions/test_extension_environment.h
+++ b/chrome/browser/extensions/test_extension_environment.h
@@ -62,6 +62,11 @@ class TestExtensionEnvironment {
   // manifest_extra override these defaults.
   const Extension* MakeExtension(const base::Value& manifest_extra);
 
+  // Use a specific extension ID instead of the default generated in
+  // Extension::Create.
+  const Extension* MakeExtension(const base::Value& manifest_extra,
+                                 const std::string& id);
+
   // Returns a test web contents that has a tab id.
   scoped_ptr<content::WebContents> MakeTab() const;
 
diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
index 1e5442c..cdb355b 100644
--- a/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
+++ b/chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc
@@ -8,6 +8,7 @@
 #include "chrome/common/extensions/manifest_handlers/app_isolation_info.h"
 #include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h"
 #include "extensions/common/error_utils.h"
+#include "extensions/common/features/simple_feature.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/csp_info.h"
 #include "extensions/common/manifest_handlers/incognito_info.h"
@@ -81,9 +82,8 @@ TEST_F(PlatformAppsManifestTest, PlatformAppContentSecurityPolicy) {
 
   // Whitelisted ones can (this is the ID corresponding to the base 64 encoded
   // key in the init_platform_app_csp.json manifest.)
-  std::string test_id = "ahplfneplbnjcflhdgkkjeiglkkfeelb";
-  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-      switches::kWhitelistedExtensionID, test_id);
+  extensions::SimpleFeature::ScopedWhitelistForTest whitelist(
+      "ahplfneplbnjcflhdgkkjeiglkkfeelb");
   scoped_refptr<Extension> extension =
       LoadAndExpectSuccess("init_platform_app_csp.json");
   EXPECT_EQ(0U, extension->install_warnings().size())
diff --git a/extensions/common/features/simple_feature.cc b/extensions/common/features/simple_feature.cc
index 34258d4..3322fac 100644
--- a/extensions/common/features/simple_feature.cc
+++ b/extensions/common/features/simple_feature.cc
@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/debug/alias.h"
+#include "base/macros.h"
 #include "base/sha1.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -27,6 +28,10 @@ namespace extensions {
 
 namespace {
 
+// A singleton copy of the --whitelisted-extension-id so that we don't need to
+// copy it from the CommandLine each time.
+std::string* g_whitelisted_extension_id = NULL;
+
 Feature::Availability IsAvailableToManifestForBind(
     const std::string& extension_id,
     Manifest::Type type,
@@ -219,8 +224,33 @@ bool IsCommandLineSwitchEnabled(const std::string& switch_name) {
   return false;
 }
 
+bool IsWhitelistedForTest(const std::string& extension_id) {
+  // TODO(jackhou): Delete the commandline whitelisting mechanism.
+  // Since it is only used it tests, ideally it should not be set via the
+  // commandline. At the moment the commandline is used as a mechanism to pass
+  // the id to the renderer process.
+  if (!g_whitelisted_extension_id) {
+    g_whitelisted_extension_id = new std::string(
+        base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+            switches::kWhitelistedExtensionID));
+  }
+  return !g_whitelisted_extension_id->empty() &&
+         *g_whitelisted_extension_id == extension_id;
+}
+
 }  // namespace
 
+SimpleFeature::ScopedWhitelistForTest::ScopedWhitelistForTest(
+    const std::string& id)
+    : previous_id_(g_whitelisted_extension_id) {
+  g_whitelisted_extension_id = new std::string(id);
+}
+
+SimpleFeature::ScopedWhitelistForTest::~ScopedWhitelistForTest() {
+  delete g_whitelisted_extension_id;
+  g_whitelisted_extension_id = previous_id_;
+}
+
 struct SimpleFeature::Mappings {
   Mappings() {
     extension_types["extension"] = Manifest::TYPE_EXTENSION;
@@ -360,20 +390,9 @@ Feature::Availability SimpleFeature::IsAvailableToManifest(
   if (component_extensions_auto_granted_ && location == Manifest::COMPONENT)
     return CreateAvailability(IS_AVAILABLE, type);
 
-  if (!whitelist_.empty()) {
-    if (!IsIdInWhitelist(extension_id)) {
-      // TODO(aa): This is gross. There should be a better way to test the
-      // whitelist.
-      base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-      if (!command_line->HasSwitch(switches::kWhitelistedExtensionID))
-        return CreateAvailability(NOT_FOUND_IN_WHITELIST, type);
-
-      std::string whitelist_switch_value =
-          base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-              switches::kWhitelistedExtensionID);
-      if (extension_id != whitelist_switch_value)
-        return CreateAvailability(NOT_FOUND_IN_WHITELIST, type);
-    }
+  if (!whitelist_.empty() && !IsIdInWhitelist(extension_id) &&
+      !IsWhitelistedForTest(extension_id)) {
+    return CreateAvailability(NOT_FOUND_IN_WHITELIST, type);
   }
 
   if (!MatchesManifestLocation(location))
diff --git a/extensions/common/features/simple_feature.h b/extensions/common/features/simple_feature.h
index 9d8c539..5ec95cf 100644
--- a/extensions/common/features/simple_feature.h
+++ b/extensions/common/features/simple_feature.h
@@ -29,6 +29,18 @@ class SimpleFeatureTest;
 
 class SimpleFeature : public Feature {
  public:
+  // Used by tests to override the cached --whitelisted-extension-id.
+  class ScopedWhitelistForTest {
+   public:
+    explicit ScopedWhitelistForTest(const std::string& id);
+    ~ScopedWhitelistForTest();
+
+   private:
+    std::string* previous_id_;
+
+    DISALLOW_COPY_AND_ASSIGN(ScopedWhitelistForTest);
+  };
+
   SimpleFeature();
   ~SimpleFeature() override;