Improve error messages when a manifest is not readable or not valid JSON.

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

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

3 files changed, 51 insertions, 4 deletions

diff --git a/chrome/browser/extensions/extension_file_util.cc b/chrome/browser/extensions/extension_file_util.cc
index 32c4819..1b6f474 100644
--- a/chrome/browser/extensions/extension_file_util.cc
+++ b/chrome/browser/extensions/extension_file_util.cc
@@ -101,14 +101,26 @@ Extension* LoadExtension(const FilePath& extension_path,
   FilePath manifest_path =
       extension_path.AppendASCII(Extension::kManifestFilename);
   if (!file_util::PathExists(manifest_path)) {
-    *error = extension_manifest_errors::kInvalidManifest;
+    *error = extension_manifest_errors::kManifestUnreadable;
     return NULL;
   }
 
   JSONFileValueSerializer serializer(manifest_path);
   scoped_ptr<Value> root(serializer.Deserialize(error));
-  if (!root.get())
+  if (!root.get()) {
+    if (error->empty()) {
+      // If |error| is empty, than the file could not be read.
+      // It would be cleaner to have the JSON reader give a specific error
+      // in this case, but other code tests for a file error with
+      // error->empty().  For now, be consistent.
+      *error = extension_manifest_errors::kManifestUnreadable;
+    } else {
+      *error = StringPrintf("%s  %s",
+                            extension_manifest_errors::kManifestParseError,
+                            error->c_str());
+    }
     return NULL;
+  }
 
   if (!root->IsType(Value::TYPE_DICTIONARY)) {
     *error = extension_manifest_errors::kInvalidManifest;
diff --git a/chrome/browser/extensions/extension_file_util_unittest.cc b/chrome/browser/extensions/extension_file_util_unittest.cc
index 7230ecd..c462032 100644
--- a/chrome/browser/extensions/extension_file_util_unittest.cc
+++ b/chrome/browser/extensions/extension_file_util_unittest.cc
@@ -185,6 +185,41 @@ TEST(ExtensionFileUtil, CheckIllegalFilenamesReservedAndIllegal) {
                                                              &error));
 }
 
+TEST(ExtensionFileUtil, LoadExtensionGivesHelpfullErrorOnMissingManifest) {
+  FilePath install_dir;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &install_dir));
+  install_dir = install_dir.AppendASCII("extensions")
+      .AppendASCII("bad")
+      .AppendASCII("Extensions")
+      .AppendASCII("dddddddddddddddddddddddddddddddd")
+      .AppendASCII("1.0");
+
+  std::string error;
+  scoped_ptr<Extension> extension(
+      extension_file_util::LoadExtension(install_dir, false, &error));
+  ASSERT_TRUE(extension == NULL);
+  ASSERT_FALSE(error.empty());
+  ASSERT_STREQ("Manifest file is missing or unreadable.", error.c_str());
+}
+
+TEST(ExtensionFileUtil, LoadExtensionGivesHelpfullErrorOnBadManifest) {
+  FilePath install_dir;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &install_dir));
+  install_dir = install_dir.AppendASCII("extensions")
+      .AppendASCII("bad")
+      .AppendASCII("Extensions")
+      .AppendASCII("eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee")
+      .AppendASCII("1.0");
+
+  std::string error;
+  scoped_ptr<Extension> extension(
+      extension_file_util::LoadExtension(install_dir, false, &error));
+  ASSERT_TRUE(extension == NULL);
+  ASSERT_FALSE(error.empty());
+  ASSERT_STREQ("Manifest is not valid JSON.  "
+               "Line: 2, column: 16, Syntax error.", error.c_str());
+}
+
 // TODO(aa): More tests as motivation allows. Maybe steal some from
 // ExtensionsService? Many of them could probably be tested here without the
 // MessageLoop shenanigans.
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 1eaa7159..f1a1ede 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -611,7 +611,7 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) {
 
   EXPECT_TRUE(MatchPattern(GetErrors()[1],
       std::string("Could not load extension from '*'. ") +
-      extension_manifest_errors::kInvalidManifest)) << GetErrors()[1];
+      extension_manifest_errors::kManifestUnreadable)) << GetErrors()[1];
 
   EXPECT_TRUE(MatchPattern(GetErrors()[2],
       std::string("Could not load extension from '*'. ") +
@@ -619,7 +619,7 @@ TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectoryFail) {
 
   EXPECT_TRUE(MatchPattern(GetErrors()[3],
       std::string("Could not load extension from '*'. ") +
-      extension_manifest_errors::kInvalidManifest)) << GetErrors()[3];
+      extension_manifest_errors::kManifestUnreadable)) << GetErrors()[3];
 };
 
 // Test that partially deleted extensions are cleaned up during startup