Do two TODOs related to --load-extension:

* Stop watching extension dirs for user script changes.
* Stop supporting unversioned extension dirs.

Also a few minor related cleanups.

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

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

4 files changed, 41 insertions, 22 deletions

diff --git a/chrome/browser/browser_init.cc b/chrome/browser/browser_init.cc
index 51541f4..c3f03bd 100644
--- a/chrome/browser/browser_init.cc
+++ b/chrome/browser/browser_init.cc
@@ -488,6 +488,7 @@ bool BrowserInit::ProcessCommandLine(
         command_line.GetSwitchValue(switches::kLoadExtension);
     FilePath path = FilePath::FromWStringHack(path_string);
     profile->GetExtensionsService()->LoadExtension(path);
+    profile->GetUserScriptMaster()->AddWatchedPath(path);
   }
 
   if (command_line.HasSwitch(switches::kInstallExtension)) {
diff --git a/chrome/browser/extensions/extension_unittest.cc b/chrome/browser/extensions/extension_unittest.cc
index 125b939..fbce6ad 100644
--- a/chrome/browser/extensions/extension_unittest.cc
+++ b/chrome/browser/extensions/extension_unittest.cc
@@ -26,7 +26,9 @@ TEST(ExtensionTest, InitFromValueInvalid) {
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_dir));
   FilePath extensions_path = FilePath::FromWStringHack(extensions_dir)
       .AppendASCII("extensions")
+      .AppendASCII("good")
       .AppendASCII("extension1")
+      .AppendASCII("1")
       .AppendASCII(Extension::kManifestFilename);
 
   JSONFileValueSerializer serializer(extensions_path.ToWStringHack());
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 05cb722..2ddcbb1 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -111,12 +111,7 @@ void ExtensionsService::OnExtensionsLoadedFromDirectory(
     }
   }
 
-  // Tell UserScriptMaster to also watch the extensions directory for changes
-  // and then kick off the first scan.
-  // TODO(aa): This should go away when we implement the --extension flag, since
-  // developing scripts in the Extensions directory will no longer be a common
-  // use-case.
-  user_script_master_->AddWatchedPath(install_directory_);
+  // Tell UserScriptMaster to kick off the first scan.
   user_script_master_->StartScan();
 
   NotificationService::current()->Notify(
@@ -179,15 +174,14 @@ bool ExtensionsServiceBackend::LoadExtensionsFromDirectory(
   for (FilePath child_path = enumerator.Next(); !child_path.value().empty();
        child_path = enumerator.Next()) {
     std::string version_str;
-    if (ReadCurrentVersion(child_path, &version_str)) {
-      child_path = child_path.AppendASCII(version_str);
-    } else {
-      // For now, continue to allow fallback to a non-versioned directory
-      // structure.  This is so that we can use this same method to load
-      // from local directories that developers are just hacking in place.
-      // TODO(erikkay): perhaps we should use a different code path for this.
+    if (!ReadCurrentVersion(child_path, &version_str)) {
+      ReportExtensionLoadError(frontend.get(), child_path, StringPrintf(
+          "Could not read '%s' file.", 
+          ExtensionsService::kCurrentVersionFileName));
+      continue;
     }
 
+    child_path = child_path.AppendASCII(version_str);
     Extension* extension = LoadExtension(child_path, frontend);
     if (extension)
       extensions->push_back(extension);
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 49e37a3..75a97a0 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -124,11 +124,12 @@ class ExtensionsServiceTestFrontend
 // make the test a PlatformTest to setup autorelease pools properly on mac
 typedef PlatformTest ExtensionsServiceTest;
 
-// Test loading extensions from the profile directory.
-TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectory) {
+// Test loading good extensions from the profile directory.
+TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
   FilePath extensions_path;
   ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
   extensions_path = extensions_path.AppendASCII("extensions");
+  extensions_path = extensions_path.AppendASCII("good");
 
   scoped_refptr<ExtensionsServiceBackend> backend(new ExtensionsServiceBackend);
   scoped_refptr<ExtensionsServiceTestFrontend> frontend(
@@ -139,9 +140,6 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectory) {
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get())));
   frontend->GetMessageLoop()->RunAllPending();
 
-  // Note: There can be more errors if there are extra directories, like .svn
-  // directories.
-  EXPECT_TRUE(frontend->errors()->size() >= 2u);
   ASSERT_EQ(3u, frontend->extensions()->size());
 
   EXPECT_EQ(std::string("com.google.myextension1"),
@@ -183,6 +181,28 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectory) {
   ASSERT_EQ(0u, frontend->extensions()->at(2)->user_scripts().size());
 };
 
+// Test loading bad extensions from the profile directory.
+TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) {
+  FilePath extensions_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
+  extensions_path = extensions_path.AppendASCII("extensions");
+  extensions_path = extensions_path.AppendASCII("bad");
+
+  scoped_refptr<ExtensionsServiceBackend> backend(new ExtensionsServiceBackend);
+  scoped_refptr<ExtensionsServiceTestFrontend> frontend(
+      new ExtensionsServiceTestFrontend);
+
+  std::vector<Extension*> extensions;
+  EXPECT_TRUE(backend->LoadExtensionsFromDirectory(extensions_path,
+      scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get())));
+  frontend->GetMessageLoop()->RunAllPending();
+
+  // Note: There can be more errors if there are extra directories, like .svn
+  // directories.
+  EXPECT_TRUE(frontend->errors()->size() >= 3u);
+  ASSERT_EQ(0u, frontend->extensions()->size());
+};
+
 // Test installing extensions.
 TEST_F(ExtensionsServiceTest, InstallExtension) {
   FilePath extensions_path;
@@ -231,18 +251,20 @@ TEST_F(ExtensionsServiceTest, LoadExtension) {
   scoped_refptr<ExtensionsServiceTestFrontend> frontend(
       new ExtensionsServiceTestFrontend);
 
-  FilePath ext1 = extensions_path.AppendASCII("extension1");
+  FilePath ext1 = extensions_path.AppendASCII("good").AppendASCII("extension1")
+      .AppendASCII("1");
   EXPECT_TRUE(backend->LoadSingleExtension(ext1,
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get())));
   frontend->GetMessageLoop()->RunAllPending();
-  EXPECT_EQ(frontend->errors()->size(), 0u);
+  EXPECT_EQ(0u, frontend->errors()->size());
   ASSERT_EQ(1u, frontend->extensions()->size());
 
-  FilePath no_manifest = extensions_path.AppendASCII("no_manifest");
+  FilePath no_manifest = extensions_path.AppendASCII("bad")
+      .AppendASCII("no_manifest").AppendASCII("1");
   EXPECT_FALSE(backend->LoadSingleExtension(no_manifest,
       scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get())));
   frontend->GetMessageLoop()->RunAllPending();
-  EXPECT_EQ(frontend->errors()->size(), 1u);
+  EXPECT_EQ(1u, frontend->errors()->size());
   ASSERT_EQ(1u, frontend->extensions()->size());
 }