Allow themes to be installed without any commandline flag, still require flag for Extensions. Expand themes unittests.

BUG=12205,12231
TEST=Without any flags, try installing an extension and a theme. The extension should fail and the theme should succeed. Attempts to install a theme with extension components in the manifest should similarly result in failure.

Review URL: http://codereview.chromium.org/115798

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

6 files changed, 211 insertions, 103 deletions

diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 248df33..93cbaed 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -5,7 +5,6 @@
 #include "chrome/browser/download/download_manager.h"
 
 #include "app/l10n_util.h"
-#include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
@@ -29,7 +28,6 @@
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
-#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/platform_util.h"
 #include "chrome/common/pref_names.h"
@@ -1230,37 +1228,20 @@ void DownloadManager::OpenDownload(const DownloadItem* download,
 }
 
 void DownloadManager::OpenChromeExtension(const FilePath& full_path) {
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableExtensions)) {
-    // Temporary: Ask the user if it's okay to install the extension. This
-    // should be replaced with the actual extension installation UI when it is
-    // avaiable.
 #if defined(OS_WIN)
-    if (win_util::MessageBox(GetActiveWindow(),
-        L"Are you sure you want to install this extension?\n\n"
-        L"This is a temporary message and it will be removed when extensions "
-        L"UI is finalized.",
-        l10n_util::GetString(IDS_PRODUCT_NAME).c_str(), MB_OKCANCEL) == IDOK) {
-      ExtensionsService* extensions_service = profile_->GetExtensionsService();
-      extensions_service->InstallExtension(full_path);
-    }
-#else
-    // TODO(port): Needs CreateChromeWindow.
+  if (win_util::MessageBox(GetActiveWindow(),
+      L"Are you sure you want to install this extension?\n\n"
+      L"This is a temporary message and it will be removed when extensions "
+      L"UI is finalized.",
+      l10n_util::GetString(IDS_PRODUCT_NAME).c_str(), MB_OKCANCEL) == IDOK) {
     ExtensionsService* extensions_service = profile_->GetExtensionsService();
     extensions_service->InstallExtension(full_path);
-#endif
-  } else {
-#if defined(OS_WIN)
-    win_util::MessageBox(GetActiveWindow(),
-        L"Extensions are not enabled. Add --enable-extensions to the "
-        L"command-line to enable extensions.\n\n"
-        L"This is a temporary message and it will be removed when extensions "
-        L"UI is finalized.",
-        l10n_util::GetString(IDS_PRODUCT_NAME).c_str(), MB_OK);
+  }
 #else
-    // TODO(port): Needs CreateChromeWindow
+  // TODO(port): Needs CreateChromeWindow.
+  ExtensionsService* extensions_service = profile_->GetExtensionsService();
+  extensions_service->InstallExtension(full_path);
 #endif
-  }
 }
 
 void DownloadManager::OpenDownloadInShell(const DownloadItem* download,
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 7bb421c..e5d365e 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/extensions_service.h"
 
+#include "app/l10n_util.h"
+#include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/gfx/png_encoder.h"
 #include "base/scoped_handle.h"
@@ -22,6 +24,7 @@
 #include "chrome/browser/extensions/extension_process_manager.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/utility_process_host.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_error_reporter.h"
 #include "chrome/common/extensions/extension_unpacker.h"
@@ -30,10 +33,14 @@
 #include "chrome/common/pref_service.h"
 #include "chrome/common/unzip.h"
 #include "chrome/common/url_constants.h"
+#include "grit/chromium_strings.h"
+#include "grit/generated_resources.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 #if defined(OS_WIN)
+#include "app/win_util.h"
 #include "base/registry.h"
+#include "base/win_util.h"
 #endif
 
 // ExtensionsService
@@ -212,6 +219,10 @@ ExtensionsService::ExtensionsService(Profile* profile,
     : prefs_(profile->GetPrefs()),
       backend_loop_(backend_loop),
       install_directory_(profile->GetPath().AppendASCII(kInstallDirectoryName)),
+      extensions_enabled_(
+          CommandLine::ForCurrentProcess()->
+          HasSwitch(switches::kEnableExtensions)),
+      show_extensions_disabled_notification_(true),
       backend_(new ExtensionsServiceBackend(
           install_directory_, g_browser_process->resource_dispatcher_host(),
           frontend_loop, registry_path)) {
@@ -720,6 +731,22 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
     return;
   }
 
+  if (!frontend_->extensions_enabled() && !extension.IsTheme()) {
+#if defined(OS_WIN)
+    if (frontend_->show_extensions_disabled_notification()) {
+      win_util::MessageBox(GetActiveWindow(),
+          L"Extensions are not enabled. Add --enable-extensions to the "
+          L"command-line to enable extensions.\n\n"
+          L"This is a temporary message and it will be removed when extensions "
+          L"UI is finalized.",
+          l10n_util::GetString(IDS_PRODUCT_NAME).c_str(), MB_OK);
+    }
+#endif
+    ReportExtensionInstallError(extension_path,
+        "Extensions are not enabled.");
+    return;
+  }
+
   // If an expected id was provided, make sure it matches.
   if (!expected_id.empty() && expected_id != extension.id()) {
     ReportExtensionInstallError(extension_path,
@@ -869,7 +896,7 @@ void ExtensionsServiceBackend::CheckForExternalUpdates(
   HKEY reg_root = HKEY_LOCAL_MACHINE;
   RegistryKeyIterator iterator(reg_root, ASCIIToWide(registry_path_).c_str());
   while (iterator.Valid()) {
-    // Fold 
+    // Fold
     std::string id = StringToLowerASCII(WideToASCII(iterator.Name()));
     if (ids_to_ignore.find(id) != ids_to_ignore.end()) {
       LOG(INFO) << "Skipping uninstalled external extension " << id;
diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h
index 105533a..70373ff 100644
--- a/chrome/browser/extensions/extensions_service.h
+++ b/chrome/browser/extensions/extensions_service.h
@@ -9,11 +9,13 @@
 #include <string>
 #include <vector>
 
+#include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/message_loop.h"
 #include "base/ref_counted.h"
 #include "base/task.h"
 #include "base/values.h"
+#include "chrome/common/chrome_switches.h"
 
 class Browser;
 class Extension;
@@ -64,6 +66,16 @@ class ExtensionsService
   // The name of the file that the current active version number is stored in.
   static const char* kCurrentVersionFileName;
 
+  void set_extensions_enabled(bool enabled) { extensions_enabled_ = enabled; }
+  void set_show_extensions_disabled_notification(bool enabled) {
+    show_extensions_disabled_notification_ = enabled;
+  }
+
+  bool extensions_enabled() { return extensions_enabled_; }
+  bool show_extensions_disabled_notification() {
+    return show_extensions_disabled_notification_;
+  }
+
  private:
   // For OnExtensionLoaded, OnExtensionInstalled, and
   // OnExtensionVersionReinstalled.
@@ -74,7 +86,7 @@ class ExtensionsService
 
   // Called by the backend when an extensoin hsa been installed.
   void OnExtensionInstalled(Extension* extension, bool is_update);
-  
+
   // Called by the backend when an extension has been reinstalled.
   void OnExtensionVersionReinstalled(const std::string& id);
 
@@ -94,6 +106,13 @@ class ExtensionsService
   // The full path to the directory where extensions are installed.
   FilePath install_directory_;
 
+  // Whether or not extensions are enabled.
+  bool extensions_enabled_;
+
+  // Whether to notify users when they attempt to install an extension without
+  // the flag being enabled.
+  bool show_extensions_disabled_notification_;
+
   // The backend that will do IO on behalf of this instance.
   scoped_refptr<ExtensionsServiceBackend> backend_;
 
@@ -196,6 +215,10 @@ class ExtensionsServiceBackend
   // Notify the frontend that the extension had already been installed.
   void ReportExtensionVersionReinstalled(const std::string& id);
 
+  // Read the manifest from the front of the extension file.
+  // Caller takes ownership of return value.
+  DictionaryValue* ReadManifest(const FilePath& extension_path);
+
   // Reads the Current Version file from |dir| into |version_string|.
   bool ReadCurrentVersion(const FilePath& dir, std::string* version_string);
 
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 57cd42c..ba39450 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -16,6 +16,7 @@
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/url_pattern.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_error_reporter.h"
 #include "chrome/common/json_value_serializer.h"
 #include "chrome/common/notification_registrar.h"
@@ -68,6 +69,8 @@ class ExtensionsServiceTest
                    NotificationService::AllSources());
     registrar_.Add(this, NotificationType::EXTENSION_INSTALLED,
                    NotificationService::AllSources());
+    registrar_.Add(this, NotificationType::THEME_INSTALLED,
+                   NotificationService::AllSources());
 
     // Create a temporary area in the registry to test external extensions.
     registry_path_ = "Software\\Google\\Chrome\\ExtensionsServiceTest_";
@@ -77,6 +80,8 @@ class ExtensionsServiceTest
     profile_.reset(new TestingProfile());
     service_ = new ExtensionsService(profile_.get(), &loop_, &loop_,
                                      registry_path_);
+    service_->set_extensions_enabled(true);
+    service_->set_show_extensions_disabled_notification(false);
     total_successes_ = 0;
   }
 
@@ -109,17 +114,19 @@ class ExtensionsServiceTest
         break;
 
       case NotificationType::EXTENSION_INSTALLED:
+      case NotificationType::THEME_INSTALLED:
         installed_ = Details<Extension>(details).ptr();
         break;
 
-      // TODO(glen): Add tests for themes.
-      // See: http://code.google.com/p/chromium/issues/detail?id=12231
-
       default:
         DCHECK(false);
     }
   }
 
+  void SetExtensionsEnabled(bool enabled) {
+    service_->set_extensions_enabled(enabled);
+  }
+
   void TestInstallExtension(const FilePath& path,
                             bool should_succeed) {
     ASSERT_TRUE(file_util::PathExists(path));
@@ -134,7 +141,10 @@ class ExtensionsServiceTest
       EXPECT_EQ(0u, errors.size()) << path.value();
       EXPECT_EQ(total_successes_, service_->extensions()->size()) <<
           path.value();
-      EXPECT_TRUE(service_->GetExtensionByID(loaded_[0]->id())) << path.value();
+      if (loaded_.size() > 0) {
+        EXPECT_TRUE(service_->GetExtensionByID(loaded_[0]->id())) << 
+            path.value();
+      }
       for (std::vector<std::string>::iterator err = errors.begin();
         err != errors.end(); ++err) {
         LOG(ERROR) << *err;
@@ -291,7 +301,7 @@ TEST_F(ExtensionsServiceTest, CleanupOnStartup) {
 
   // We should have only gotten two extensions now.
   EXPECT_EQ(2u, loaded_.size());
-  
+
   // And extension1 dir should now be toast.
   dest_path = dest_path.DirName();
   ASSERT_FALSE(file_util::PathExists(dest_path));
@@ -303,14 +313,16 @@ TEST_F(ExtensionsServiceTest, InstallExtension) {
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");
 
-  // A simple extension that should install without error.
+  // Extensions not enabled.
+  SetExtensionsEnabled(false);
   FilePath path = extensions_path.AppendASCII("good.crx");
-  TestInstallExtension(path, true);
-  // TODO(erikkay): verify the contents of the installed extension.
+  TestInstallExtension(path, false);
+  SetExtensionsEnabled(true);
 
-  // An extension with theme images.
-  path = extensions_path.AppendASCII("theme.crx");
+  // A simple extension that should install without error.
+  path = extensions_path.AppendASCII("good.crx");
   TestInstallExtension(path, true);
+  // TODO(erikkay): verify the contents of the installed extension.
 
   // An extension with page actions.
   path = extensions_path.AppendASCII("page_action.crx");
@@ -336,6 +348,26 @@ TEST_F(ExtensionsServiceTest, InstallExtension) {
   // TODO(erikkay): add tests for upgrade cases.
 }
 
+TEST_F(ExtensionsServiceTest, InstallTheme) {
+  FilePath extensions_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
+  extensions_path = extensions_path.AppendASCII("extensions");
+
+  // A theme.
+  FilePath path = extensions_path.AppendASCII("theme.crx");
+  TestInstallExtension(path, true);
+
+  // A theme when extensions are disabled.
+  SetExtensionsEnabled(false);
+  path = extensions_path.AppendASCII("theme2.crx");
+  TestInstallExtension(path, true);
+  SetExtensionsEnabled(true);
+
+  // A theme with extension elements.
+  path = extensions_path.AppendASCII("theme_with_extension.crx");
+  TestInstallExtension(path, false);
+}
+
 // Test that when an extension version is reinstalled, nothing happens.
 TEST_F(ExtensionsServiceTest, Reinstall) {
   FilePath extensions_path;
@@ -494,7 +526,7 @@ TEST_F(ExtensionsServiceTest, ExternalInstall) {
   loop_.RunAllPending();
   ASSERT_EQ(0u, GetErrors().size());
   ASSERT_EQ(1u, loaded_.size());
-  
+
   // Now update the extension with a new version. We should get upgraded.
   source_path = source_path.DirName().AppendASCII("good2.crx");
   ASSERT_TRUE(key.WriteValue(L"path", source_path.ToWStringHack().c_str()));
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 710a7e5..cdf484b 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -45,6 +45,16 @@ const char* Extension::kRunAtDocumentEndValue = "document_end";
 const char* Extension::kPageActionTypeTab = "tab";
 const char* Extension::kPageActionTypePermanent = "permanent";
 
+// A list of all the keys allowed by themes.
+static const wchar_t* kValidThemeKeys[] = {
+  Extension::kDescriptionKey,
+  Extension::kIconPathKey,
+  Extension::kIdKey,
+  Extension::kNameKey,
+  Extension::kThemeKey,
+  Extension::kVersionKey,
+  Extension::kZipHashKey
+};
 
 // Extension-related error messages. Some of these are simple patterns, where a
 // '*' is replaced at runtime with a specific value. This is used instead of
@@ -122,6 +132,8 @@ const char* Extension::kInvalidThemeColorsError =
     "Invalid value for theme colors - colors must be integers";
 const char* Extension::kInvalidThemeTintsError =
     "Invalid value for theme images - tints must be decimal numbers.";
+const char* Extension::kThemesCannotContainExtensionsError =
+    "A theme cannot contain extensions code.";
 
 const size_t Extension::kIdSize = 20;  // SHA1 (160 bits) == 20 bytes
 
@@ -155,62 +167,6 @@ const PageAction* Extension::GetPageAction(std::string id) const {
   return it->second;
 }
 
-// static
-FilePath Extension::GetResourcePath(const FilePath& extension_path,
-                                    const std::string& relative_path) {
-  // Build up a file:// URL and convert that back to a FilePath.  This avoids
-  // URL encoding and path separator issues.
-
-  // Convert the extension's root to a file:// URL.
-  GURL extension_url = net::FilePathToFileURL(extension_path);
-  if (!extension_url.is_valid())
-    return FilePath();
-
-  // Append the requested path.
-  GURL::Replacements replacements;
-  std::string new_path(extension_url.path());
-  new_path += "/";
-  new_path += relative_path;
-  replacements.SetPathStr(new_path);
-  GURL file_url = extension_url.ReplaceComponents(replacements);
-  if (!file_url.is_valid())
-    return FilePath();
-
-  // Convert the result back to a FilePath.
-  FilePath ret_val;
-  if (!net::FileURLToFilePath(file_url, &ret_val))
-    return FilePath();
-
-  // Double-check that the path we ended up with is actually inside the
-  // extension root. We can do this with a simple prefix match because:
-  // a) We control the prefix on both sides, and they should match.
-  // b) GURL normalizes things like "../" and "//" before it gets to us.
-  if (ret_val.value().find(extension_path.value() +
-                           FilePath::kSeparators[0]) != 0)
-    return FilePath();
-
-  return ret_val;
-}
-
-Extension::Extension(const FilePath& path) {
-  DCHECK(path.IsAbsolute());
-  location_ = INVALID;
-
-#if defined(OS_WIN)
-  // Normalize any drive letter to upper-case. We do this for consistency with
-  // net_utils::FilePathToFileURL(), which does the same thing, to make string
-  // comparisons simpler.
-  std::wstring path_str = path.value();
-  if (path_str.size() >= 2 && path_str[0] >= L'a' && path_str[0] <= L'z' &&
-      path_str[1] == ':')
-    path_str[0] += ('A' - 'a');
-
-  path_ = FilePath(path_str);
-#else
-  path_ = path;
-#endif
-}
-
 // Helper method that loads a UserScript object from a dictionary in the
 // content_script list of the manifest.
 bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
@@ -393,6 +349,86 @@ PageAction* Extension::LoadPageActionHelper(
   return result.release();
 }
 
+bool Extension::ContainsNonThemeKeys(const DictionaryValue& source) {
+  // Generate a map of allowable keys
+  static std::map<std::wstring, bool> theme_keys;
+  static bool theme_key_mapped = false;
+  if (!theme_key_mapped) {
+    for (size_t i = 0; i < arraysize(kValidThemeKeys); ++i) {
+      theme_keys[kValidThemeKeys[i]] = true;
+    }
+    theme_key_mapped = true;
+  }
+
+  // Go through all the root level keys and verify that they're in the map
+  // of keys allowable by themes. If they're not, then make a not of it for
+  // later.
+  DictionaryValue::key_iterator iter = source.begin_keys();
+  while (iter != source.end_keys()) {
+    std::wstring key = (*iter);
+    if (theme_keys.find(key) == theme_keys.end())
+      return true;
+    ++iter;
+  }
+  return false;
+}
+
+// static
+FilePath Extension::GetResourcePath(const FilePath& extension_path,
+                                    const std::string& relative_path) {
+  // Build up a file:// URL and convert that back to a FilePath.  This avoids
+  // URL encoding and path separator issues.
+
+  // Convert the extension's root to a file:// URL.
+  GURL extension_url = net::FilePathToFileURL(extension_path);
+  if (!extension_url.is_valid())
+    return FilePath();
+
+  // Append the requested path.
+  GURL::Replacements replacements;
+  std::string new_path(extension_url.path());
+  new_path += "/";
+  new_path += relative_path;
+  replacements.SetPathStr(new_path);
+  GURL file_url = extension_url.ReplaceComponents(replacements);
+  if (!file_url.is_valid())
+    return FilePath();
+
+  // Convert the result back to a FilePath.
+  FilePath ret_val;
+  if (!net::FileURLToFilePath(file_url, &ret_val))
+    return FilePath();
+
+  // Double-check that the path we ended up with is actually inside the
+  // extension root. We can do this with a simple prefix match because:
+  // a) We control the prefix on both sides, and they should match.
+  // b) GURL normalizes things like "../" and "//" before it gets to us.
+  if (ret_val.value().find(extension_path.value() +
+                           FilePath::kSeparators[0]) != 0)
+    return FilePath();
+
+  return ret_val;
+}
+
+Extension::Extension(const FilePath& path) {
+  DCHECK(path.IsAbsolute());
+  location_ = INVALID;
+
+#if defined(OS_WIN)
+  // Normalize any drive letter to upper-case. We do this for consistency with
+  // net_utils::FilePathToFileURL(), which does the same thing, to make string
+  // comparisons simpler.
+  std::wstring path_str = path.value();
+  if (path_str.size() >= 2 && path_str[0] >= L'a' && path_str[0] <= L'z' &&
+      path_str[1] == ':')
+    path_str[0] += ('A' - 'a');
+
+  path_ = FilePath(path_str);
+#else
+  path_ = path;
+#endif
+}
+
 bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
                               std::string* error) {
   // Initialize id.
@@ -466,13 +502,15 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     }
   }
 
-  // Initialize themes. If a theme is included, no other items may be processed
-  // (we currently don't want people bundling themes and extension stuff
-  // together).
-  //
-  // TODO(glen): Error if other items *are* included.
+  // Initialize themes.
   is_theme_ = false;
   if (source.HasKey(kThemeKey)) {
+    // Themes cannot contain extension keys.
+    if (ContainsNonThemeKeys(source)) {
+      *error = kThemesCannotContainExtensionsError;
+      return false;
+    }
+
     DictionaryValue* theme_value;
     if (!source.GetDictionary(kThemeKey, &theme_value)) {
       *error = kInvalidThemeError;
@@ -538,6 +576,7 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
       theme_tints_.reset(
           static_cast<DictionaryValue*>(tints_value->DeepCopy()));
     }
+
     return true;
   }
 
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index a92191c..1256b17f 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -13,6 +13,7 @@
 #include "base/values.h"
 #include "base/version.h"
 #include "chrome/common/extensions/user_script.h"
+#include "chrome/browser/extensions/user_script_master.h"
 #include "chrome/common/extensions/url_pattern.h"
 #include "chrome/common/page_action.h"
 #include "googleurl/src/gurl.h"
@@ -97,12 +98,13 @@ class Extension {
   static const char* kInvalidThemeImagesError;
   static const char* kInvalidThemeColorsError;
   static const char* kInvalidThemeTintsError;
+  static const char* kThemesCannotContainExtensionsError;
   static const char* kMissingFileError;
 
   // The number of bytes in a legal id.
   static const size_t kIdSize;
 
-  Extension() : location_(INVALID) {}
+  Extension() : location_(INVALID), is_theme_(false) {}
   explicit Extension(const FilePath& path);
   virtual ~Extension();
 
@@ -175,6 +177,10 @@ class Extension {
                                    int definition_index,
                                    std::string* error);
 
+  // Figures out if a source contains keys not associated with themes - we
+  // don't want to allow scripts and such to be bundled with themes.
+  bool ContainsNonThemeKeys(const DictionaryValue& source);
+
   // The absolute path to the directory the extension is stored in.
   FilePath path_;