Make the "id" key for extension manifests optional in

--load-extension mode. This makes this mode more convenient, and
should lead to less copy-and-paste of extension IDs.

Also, remove the "format_version" key. This is basically
duplicated in the code that checks the version number of the crx
file format. And if we ever really need a format version for the
manifest, we can always add it later.

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

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

10 files changed, 88 insertions, 60 deletions

diff --git a/chrome/browser/extensions/extension.cc b/chrome/browser/extensions/extension.cc
index 57fbbbc..d8ab403 100644
--- a/chrome/browser/extensions/extension.cc
+++ b/chrome/browser/extensions/extension.cc
@@ -18,7 +18,6 @@ const char Extension::kManifestFilename[] = "manifest.json";
 const wchar_t* Extension::kContentScriptsKey = L"content_scripts";
 const wchar_t* Extension::kCssKey = L"css";
 const wchar_t* Extension::kDescriptionKey = L"description";
-const wchar_t* Extension::kFormatVersionKey = L"format_version";
 const wchar_t* Extension::kIdKey = L"id";
 const wchar_t* Extension::kJsKey = L"js";
 const wchar_t* Extension::kMatchesKey = L"matches";
@@ -48,8 +47,6 @@ const char* Extension::kInvalidCssListError =
     "Required value 'content_scripts[*].css is invalid.";
 const char* Extension::kInvalidDescriptionError =
     "Invalid value for 'description'.";
-const char* Extension::kInvalidFormatVersionError =
-    "Required value 'format_version' is missing or invalid.";
 const char* Extension::kInvalidIdError =
     "Required value 'id' is missing or invalid.";
 const char* Extension::kInvalidJsError =
@@ -106,6 +103,21 @@ Extension::Extension(const Extension& rhs)
       theme_paths_(rhs.theme_paths_) {
 }
 
+const GURL& Extension::url() {
+  if (!extension_url_.is_valid())
+    extension_url_ = GURL(std::string(chrome::kExtensionScheme) +
+                          chrome::kStandardSchemeSeparator + id_ + "/");
+
+  return extension_url_;
+}
+
+void Extension::set_id(const std::string& id) {
+  id_ = id;
+
+  // Reset url_ so that it gets reinitialized next time.
+  extension_url_ = GURL();
+}
+
 const std::string Extension::VersionString() const {
   return version_->GetString();
 }
@@ -325,36 +337,27 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
 
 bool Extension::InitFromValue(const DictionaryValue& source,
                               std::string* error) {
-  // Check format version.
-  int format_version = 0;
-  if (!source.GetInteger(kFormatVersionKey, &format_version) ||
-      static_cast<uint32>(format_version) != kExpectedFormatVersion) {
-    *error = kInvalidFormatVersionError;
-    return false;
-  }
-
-  // Initialize id.
-  if (!source.GetString(kIdKey, &id_)) {
-    *error = kInvalidIdError;
-    return false;
-  }
+  // Initialize id. The ID is not required here because we don't require IDs for
+  // extensions used with --load-extension.
+  if (source.HasKey(kIdKey)) {
+    if (!source.GetString(kIdKey, &id_)) {
+      *error = kInvalidIdError;
+      return false;
+    }
 
-  // Normalize the string to lowercase, so it can be used as an URL component
-  // (where GURL will lowercase it).
-  StringToLowerASCII(&id_);
+    // Normalize the string to lowercase, so it can be used as an URL component
+    // (where GURL will lowercase it).
+    StringToLowerASCII(&id_);
 
-  // Verify that the id is legal.  The id is a hex string of the SHA-1 hash of
-  // the public key.
-  std::vector<uint8> id_bytes;
-  if (!HexStringToBytes(id_, &id_bytes) || id_bytes.size() != kIdSize) {
-    *error = kInvalidIdError;
-    return false;
+    // Verify that the id is legal.  The id is a hex string of the SHA-1 hash of
+    // the public key.
+    std::vector<uint8> id_bytes;
+    if (!HexStringToBytes(id_, &id_bytes) || id_bytes.size() != kIdSize) {
+      *error = kInvalidIdError;
+      return false;
+    }
   }
 
-  // Initialize URL.
-  extension_url_ = GURL(std::string(chrome::kExtensionScheme) +
-                        chrome::kStandardSchemeSeparator + id_ + "/");
-
   // Initialize version.
   std::string version_str;
   if (!source.GetString(kVersionKey, &version_str)) {
diff --git a/chrome/browser/extensions/extension.h b/chrome/browser/extensions/extension.h
index 565c8b5..5c3d9b8f 100644
--- a/chrome/browser/extensions/extension.h
+++ b/chrome/browser/extensions/extension.h
@@ -25,9 +25,6 @@ class Extension {
   explicit Extension(const FilePath& path);
   explicit Extension(const Extension& path);
 
-  // The format for extension manifests that this code understands.
-  static const unsigned int kExpectedFormatVersion = 1;
-
   // The name of the manifest inside an extension.
   static const char kManifestFilename[];
 
@@ -35,7 +32,6 @@ class Extension {
   static const wchar_t* kContentScriptsKey;
   static const wchar_t* kCssKey;
   static const wchar_t* kDescriptionKey;
-  static const wchar_t* kFormatVersionKey;
   static const wchar_t* kIdKey;
   static const wchar_t* kJsKey;
   static const wchar_t* kMatchesKey;
@@ -58,7 +54,6 @@ class Extension {
   static const char* kInvalidCssError;
   static const char* kInvalidCssListError;
   static const char* kInvalidDescriptionError;
-  static const char* kInvalidFormatVersionError;
   static const char* kInvalidIdError;
   static const char* kInvalidJsError;
   static const char* kInvalidJsListError;
@@ -115,8 +110,9 @@ class Extension {
   FilePath GetThemeResourcePath(const int resource_id);
 
   const FilePath& path() const { return path_; }
-  const GURL& url() const { return extension_url_; }
+  const GURL& url();
   const std::string& id() const { return id_; }
+  void set_id(const std::string& id);
   const Version* version() const { return version_.get(); }
   // String representation of the version number.
   const std::string VersionString() const;
diff --git a/chrome/browser/extensions/extension_unittest.cc b/chrome/browser/extensions/extension_unittest.cc
index b11e2dd..44e6106 100644
--- a/chrome/browser/extensions/extension_unittest.cc
+++ b/chrome/browser/extensions/extension_unittest.cc
@@ -43,26 +43,8 @@ TEST(ExtensionTest, InitFromValueInvalid) {
 
   scoped_ptr<DictionaryValue> input_value;
 
-  // Test missing and invalid format versions
-  input_value.reset(static_cast<DictionaryValue*>(valid_value->DeepCopy()));
-  input_value->Remove(Extension::kFormatVersionKey, NULL);
-  EXPECT_FALSE(extension.InitFromValue(*input_value, &error));
-  EXPECT_EQ(Extension::kInvalidFormatVersionError, error);
-
-  input_value->SetString(Extension::kFormatVersionKey, "foo");
-  EXPECT_FALSE(extension.InitFromValue(*input_value, &error));
-  EXPECT_EQ(Extension::kInvalidFormatVersionError, error);
-
-  input_value->SetInteger(Extension::kFormatVersionKey, 2);
-  EXPECT_FALSE(extension.InitFromValue(*input_value, &error));
-  EXPECT_EQ(Extension::kInvalidFormatVersionError, error);
-
   // Test missing and invalid ids
   input_value.reset(static_cast<DictionaryValue*>(valid_value->DeepCopy()));
-  input_value->Remove(Extension::kIdKey, NULL);
-  EXPECT_FALSE(extension.InitFromValue(*input_value, &error));
-  EXPECT_EQ(Extension::kInvalidIdError, error);
-
   input_value->SetInteger(Extension::kIdKey, 42);
   EXPECT_FALSE(extension.InitFromValue(*input_value, &error));
   EXPECT_EQ(Extension::kInvalidIdError, error);
@@ -222,7 +204,6 @@ TEST(ExtensionTest, InitFromValueValid) {
   DictionaryValue input_value;
 
   // Test minimal extension
-  input_value.SetInteger(Extension::kFormatVersionKey, 1);
   input_value.SetString(Extension::kIdKey,
       "00123456789ABCDEF0123456789ABCDEF0123456");
   input_value.SetString(Extension::kVersionKey, "1.0.0.0");
@@ -246,7 +227,6 @@ TEST(ExtensionTest, GetResourceURLAndPath) {
 #endif
   Extension extension(path);
   DictionaryValue input_value;
-  input_value.SetInteger(Extension::kFormatVersionKey, 1);
   input_value.SetString(Extension::kIdKey,
       "00123456789ABCDEF0123456789ABCDEF0123456");
   input_value.SetString(Extension::kVersionKey, "1.0.0.0");
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index 6b73b65..11eff23 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -63,6 +63,9 @@ const wchar_t kRegistryExtensionVersion[] = L"version";
 // A marker file to indicate that an extension was installed from an external
 // source.
 const char kExternalInstallFile[] = "EXTERNAL_INSTALL";
+
+// The version of the extension package that this code understands.
+const uint32 kExpectedVersion = 1;
 }
 
 ExtensionsService::ExtensionsService(Profile* profile,
@@ -257,6 +260,16 @@ void ExtensionsServiceBackend::LoadSingleExtension(
 
   Extension* extension = LoadExtension(extension_path);
   if (extension) {
+    if (extension->id().empty()) {
+      // Generate an ID
+      static int counter = 0;
+      std::string id = StringPrintf("%x", counter);
+      ++counter;
+
+      // pad the string out to 40 chars with zeroes.
+      id.insert(0, 40 - id.length(), '0');
+      extension->set_id(id);
+    }
     ExtensionList* extensions = new ExtensionList;
     extensions->push_back(extension);
     ReportExtensionsLoaded(extensions);
@@ -382,7 +395,7 @@ DictionaryValue* ExtensionsServiceBackend::ReadManifest(
     ReportExtensionInstallError(extension_path, "bad magic number");
     return NULL;
   }
-  if (header.version != Extension::kExpectedFormatVersion) {
+  if (header.version != kExpectedVersion) {
     ReportExtensionInstallError(extension_path, "bad version number");
     return NULL;
   }
@@ -631,6 +644,12 @@ bool ExtensionsServiceBackend::InstallOrUpdateExtension(
     return false;
   }
 
+  // ID is required for installed extensions.
+  if (extension.id().empty()) {
+    ReportExtensionInstallError(source_file, "Required value 'id' is missing.");
+    return false;
+  }
+
   // If an expected id was provided, make sure it matches.
   if (expected_id.length() && expected_id != extension.id()) {
     ReportExtensionInstallError(source_file,
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 4d4b4f7..abdaef8 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -319,3 +319,33 @@ TEST_F(ExtensionsServiceTest, LoadExtension) {
   EXPECT_EQ(1u, GetErrors().size());
   ASSERT_EQ(1u, frontend->extensions()->size());
 }
+
+TEST_F(ExtensionsServiceTest, GenerateID) {
+  FilePath extensions_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
+  extensions_path = extensions_path.AppendASCII("extensions");
+
+  scoped_refptr<ExtensionsServiceBackend> backend(
+      new ExtensionsServiceBackend(extensions_path));
+  scoped_refptr<ExtensionsServiceTestFrontend> frontend(
+      new ExtensionsServiceTestFrontend);
+
+  FilePath no_id_ext = extensions_path.AppendASCII("no_id");
+  backend->LoadSingleExtension(no_id_ext,
+      scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
+  frontend->GetMessageLoop()->RunAllPending();
+  EXPECT_EQ(0u, GetErrors().size());
+  ASSERT_EQ(1u, frontend->extensions()->size());
+  std::string id1 = frontend->extensions()->at(0)->id();
+  ASSERT_EQ("0000000000000000000000000000000000000000", id1);
+  ASSERT_EQ("chrome-extension://0000000000000000000000000000000000000000/",
+            frontend->extensions()->at(0)->url().spec());
+
+  backend->LoadSingleExtension(no_id_ext,
+      scoped_refptr<ExtensionsServiceFrontendInterface>(frontend.get()));
+  frontend->GetMessageLoop()->RunAllPending();
+  std::string id2 = frontend->extensions()->at(1)->id();
+  ASSERT_EQ("0000000000000000000000000000000000000001", id2);
+  ASSERT_EQ("chrome-extension://0000000000000000000000000000000000000001/",
+            frontend->extensions()->at(1)->url().spec());
+}
diff --git a/chrome/test/data/extensions/bad/missing_content_script/1/manifest.json b/chrome/test/data/extensions/bad/missing_content_script/1/manifest.json
index 55e62a4..9799306 100644
--- a/chrome/test/data/extensions/bad/missing_content_script/1/manifest.json
+++ b/chrome/test/data/extensions/bad/missing_content_script/1/manifest.json
@@ -1,5 +1,4 @@
 {
-  "format_version": 1,
   "id": "00123456789ABCDEF0123456789ABCDEF0123456",
   "version": "1.0.0.0",
   "name": "My extension 1",
diff --git a/chrome/test/data/extensions/good/extension1/1/manifest.json b/chrome/test/data/extensions/good/extension1/1/manifest.json
index cc2104d..f8888c5 100644
--- a/chrome/test/data/extensions/good/extension1/1/manifest.json
+++ b/chrome/test/data/extensions/good/extension1/1/manifest.json
@@ -1,5 +1,4 @@
 {
-  "format_version": 1,
   "id": "00123456789ABCDEF0123456789ABCDEF0123456",
   "version": "1.0.0.0",
   "name": "My extension 1",
diff --git a/chrome/test/data/extensions/good/extension2/2/manifest.json b/chrome/test/data/extensions/good/extension2/2/manifest.json
index d5bb394..5e148f3 100644
--- a/chrome/test/data/extensions/good/extension2/2/manifest.json
+++ b/chrome/test/data/extensions/good/extension2/2/manifest.json
@@ -1,5 +1,4 @@
 {
-  "format_version": 1,
   "id": "10123456789ABCDEF0123456789ABCDEF0123456",
   "version": "1.0.0.0",
   "name": "My extension 2",
diff --git a/chrome/test/data/extensions/good/extension3/1.0/manifest.json b/chrome/test/data/extensions/good/extension3/1.0/manifest.json
index f1390de..7927104 100644
--- a/chrome/test/data/extensions/good/extension3/1.0/manifest.json
+++ b/chrome/test/data/extensions/good/extension3/1.0/manifest.json
@@ -1,5 +1,4 @@
 {
-  "format_version": 1,
   "id": "20123456789ABCDEF0123456789ABCDEF0123456",
   "version": "1.0",
   "name": "My extension 3"
diff --git a/chrome/test/data/extensions/no_id/manifest.json b/chrome/test/data/extensions/no_id/manifest.json
new file mode 100644
index 0000000..5737212
--- /dev/null
+++ b/chrome/test/data/extensions/no_id/manifest.json
@@ -0,0 +1,4 @@
+{
+  "version": "1.0",
+  "name": "My extension"
+}