Extension package creation cleanup

Unify extension package creation code between command line and GUI methods.

Properly handle extension names with periods or trailing slashes.

Don't DCHECK when creating packages from the command line.

BUG=14720, 19103, 51110
TEST=run 'chrome --pack-extension=has.a.dot/' on an extension with period in its name; make sure it is packed correctly and a message indicating success is printed.

Review URL: http://codereview.chromium.org/3077022
Patch from Ilya Sherman <isherman@google.com>.

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

6 files changed, 227 insertions, 80 deletions

diff --git a/chrome/browser/browser_init.cc b/chrome/browser/browser_init.cc
index 46823ff..4b26d4a 100644
--- a/chrome/browser/browser_init.cc
+++ b/chrome/browser/browser_init.cc
@@ -27,6 +27,7 @@
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/extensions/extension_creator.h"
 #include "chrome/browser/extensions/extensions_service.h"
+#include "chrome/browser/extensions/pack_extension_job.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/net/predictor_api.h"
 #include "chrome/browser/net/url_fixer_upper.h"
@@ -345,20 +346,6 @@ GURL GetWelcomePageURL() {
   return GURL(welcome_url);
 }
 
-void ShowPackExtensionMessage(const std::wstring caption,
-    const std::wstring message) {
-#if defined(OS_WIN)
-  win_util::MessageBox(NULL, message, caption, MB_OK | MB_SETFOREGROUND);
-#else
-  // Just send caption & text to stdout on mac & linux.
-  std::string out_text = WideToASCII(caption);
-  out_text.append("\n\n");
-  out_text.append(WideToASCII(message));
-  out_text.append("\n");
-  printf("%s", out_text.c_str());
-#endif
-}
-
 void UrlsToTabs(const std::vector<GURL>& urls,
                 std::vector<BrowserInit::LaunchWithProfile::Tab>* tabs) {
   for (size_t i = 0; i < urls.size(); ++i) {
@@ -934,6 +921,54 @@ void BrowserInit::LaunchWithProfile::CheckDefaultBrowser(Profile* profile) {
       ChromeThread::FILE, FROM_HERE, new CheckDefaultBrowserTask());
 }
 
+class PackExtensionLogger : public PackExtensionJob::Client {
+ public:
+  PackExtensionLogger() {}
+  virtual void OnPackSuccess(const FilePath& crx_path,
+                             const FilePath& output_private_key_path);
+  virtual void OnPackFailure(const std::string& error_message);
+
+ private:
+  void ShowPackExtensionMessage(const std::wstring& caption,
+                                const std::wstring& message);
+
+  DISALLOW_COPY_AND_ASSIGN(PackExtensionLogger);
+};
+
+void PackExtensionLogger::OnPackSuccess(const FilePath& crx_path,
+                                        const FilePath& output_private_key_path)
+{
+  ShowPackExtensionMessage(L"Extension Packaging Success",
+                           PackExtensionJob::StandardSuccessMessage(
+                               crx_path, output_private_key_path));
+}
+
+void PackExtensionLogger::OnPackFailure(const std::string& error_message) {
+  ShowPackExtensionMessage(L"Extension Packaging Error",
+                           UTF8ToWide(error_message));
+}
+
+void PackExtensionLogger::ShowPackExtensionMessage(const std::wstring& caption,
+                                                   const std::wstring& message)
+{
+#if defined(OS_WIN)
+  win_util::MessageBox(NULL, message, caption, MB_OK | MB_SETFOREGROUND);
+#else
+  // Just send caption & text to stdout on mac & linux.
+  std::string out_text = WideToASCII(caption);
+  out_text.append("\n\n");
+  out_text.append(WideToASCII(message));
+  out_text.append("\n");
+  printf("%s", out_text.c_str());
+#endif
+
+  // We got the notification and processed it; we don't expect any further tasks
+  // to be posted to the current thread, so we should stop blocking and exit.
+  // This call to |Quit()| matches the call to |Run()| in
+  // |ProcessCmdLineImpl()|.
+  MessageLoop::current()->Quit();
+}
+
 bool BrowserInit::ProcessCmdLineImpl(const CommandLine& command_line,
                                      const FilePath& cur_dir,
                                      bool process_startup,
@@ -980,44 +1015,21 @@ bool BrowserInit::ProcessCmdLineImpl(const CommandLine& command_line,
             switches::kPackExtensionKey);
       }
 
-      // Output Paths.
-      FilePath output(src_dir.DirName().Append(src_dir.BaseName().value()));
-      FilePath crx_path(output);
-      crx_path = crx_path.ReplaceExtension(chrome::kExtensionFileExtension);
-      FilePath output_private_key_path;
-      if (private_key_path.empty()) {
-        output_private_key_path = FilePath(output);
-        output_private_key_path =
-            output_private_key_path.ReplaceExtension(FILE_PATH_LITERAL("pem"));
-      }
+      // Launch a job to perform the packing on the file thread.
+      PackExtensionLogger pack_client;
+      scoped_refptr<PackExtensionJob> pack_job =
+          new PackExtensionJob(&pack_client, src_dir, private_key_path);
+      pack_job->Start();
+
+      // The job will post a notification task to the current thread's message
+      // loop when it is finished.  We manually run the loop here so that we
+      // block and catch the notification.  Otherwise, the process would exit;
+      // in particular, this would mean that |pack_client| would be destroyed
+      // and we wouldn't be able to report success or failure back to the user.
+      // This call to |Run()| is matched by a call to |Quit()| in the
+      // |PackExtensionLogger|'s notification handling code.
+      MessageLoop::current()->Run();
 
-      // TODO(port): Creation & running is removed from mac & linux because
-      // ExtensionCreator depends on base/crypto/rsa_private_key and
-      // base/crypto/signature_creator, both of which only have windows
-      // implementations.
-      scoped_ptr<ExtensionCreator> creator(new ExtensionCreator());
-      if (creator->Run(src_dir, crx_path, private_key_path,
-          output_private_key_path)) {
-        std::wstring message;
-        if (private_key_path.value().empty()) {
-          message = StringPrintf(
-              L"Created the following files:\n\n"
-              L"Extension: %ls\n"
-              L"Key File: %ls\n\n"
-              L"Keep your key file in a safe place. You will need it to create "
-              L"new versions of your extension.",
-              crx_path.ToWStringHack().c_str(),
-              output_private_key_path.ToWStringHack().c_str());
-        } else {
-          message = StringPrintf(L"Created the extension:\n\n%ls",
-                                 crx_path.ToWStringHack().c_str());
-        }
-        ShowPackExtensionMessage(L"Extension Packaging Success", message);
-      } else {
-        ShowPackExtensionMessage(L"Extension Packaging Error",
-            UTF8ToWide(creator->error_message()));
-        return false;
-      }
       return false;
     }
   }
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 012e8d6..15e5eef 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -27,6 +27,7 @@
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/extensions/external_extension_provider.h"
 #include "chrome/browser/extensions/external_pref_extension_provider.h"
+#include "chrome/browser/extensions/pack_extension_job.cc"
 #include "chrome/browser/pref_value_store.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -600,16 +601,56 @@ class ExtensionsServiceTest
   NotificationRegistrar registrar_;
 };
 
-FilePath::StringType NormalizeSeperators(FilePath::StringType path) {
+FilePath NormalizeSeparators(const FilePath& path) {
 #if defined(FILE_PATH_USES_WIN_SEPARATORS)
-  FilePath::StringType ret_val;
-  for (size_t i = 0; i < path.length(); i++) {
-    if (FilePath::IsSeparator(path[i]))
-      path[i] = FilePath::kSeparators[0];
-  }
-#endif  // FILE_PATH_USES_WIN_SEPARATORS
+  return path.NormalizeWindowsPathSeparators();
+#else
   return path;
+#endif  // FILE_PATH_USES_WIN_SEPARATORS
+}
+
+// Receives notifications from a PackExtensionJob, indicating either that
+// packing succeeded or that there was some error.
+class PackExtensionTestClient : public PackExtensionJob::Client {
+ public:
+  PackExtensionTestClient(const FilePath& expected_crx_path,
+                          const FilePath& expected_private_key_path);
+  virtual void OnPackSuccess(const FilePath& crx_path,
+                             const FilePath& private_key_path);
+  virtual void OnPackFailure(const std::string& error_message);
+
+ private:
+  const FilePath expected_crx_path_;
+  const FilePath expected_private_key_path_;
+  DISALLOW_COPY_AND_ASSIGN(PackExtensionTestClient);
+};
+
+PackExtensionTestClient::PackExtensionTestClient(
+    const FilePath& expected_crx_path,
+    const FilePath& expected_private_key_path)
+    : expected_crx_path_(expected_crx_path),
+      expected_private_key_path_(expected_private_key_path) {}
+
+// If packing succeeded, we make sure that the package names match our
+// expectations.
+void PackExtensionTestClient::OnPackSuccess(const FilePath& crx_path,
+                                            const FilePath& private_key_path) {
+  // We got the notification and processed it; we don't expect any further tasks
+  // to be posted to the current thread, so we should stop blocking and continue
+  // on with the rest of the test.
+  // This call to |Quit()| matches the call to |Run()| in the
+  // |PackPunctuatedExtension| test.
+  MessageLoop::current()->Quit();
+  EXPECT_EQ(expected_crx_path_.value(), crx_path.value());
+  EXPECT_EQ(expected_private_key_path_.value(), private_key_path.value());
+  ASSERT_TRUE(file_util::PathExists(private_key_path));
+}
+
+// The tests are designed so that we never expect to see a packing error.
+void PackExtensionTestClient::OnPackFailure(const std::string& error_message) {
+  FAIL() << "Packing should not fail.";
 }
+
 // Test loading good extensions from the profile directory.
 TEST_F(ExtensionsServiceTest, LoadAllExtensionsFromDirectorySuccess) {
   // Initialize the test dir with a good Preferences/extensions.
@@ -936,6 +977,80 @@ TEST_F(ExtensionsServiceTest, PackExtension) {
                             FilePath()));
 }
 
+// Test Packaging and installing an extension whose name contains punctuation.
+TEST_F(ExtensionsServiceTest, PackPunctuatedExtension) {
+  InitializeEmptyExtensionsService();
+  FilePath extensions_path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &extensions_path));
+  extensions_path = extensions_path.AppendASCII("extensions");
+  FilePath input_directory = extensions_path
+      .AppendASCII("good")
+      .AppendASCII("Extensions")
+      .AppendASCII(good0)
+      .AppendASCII("1.0.0.0");
+
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  // Extension names containing punctuation, and the expected names for the
+  // packed extensions.
+  const FilePath punctuated_names[] = {
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL("this.extensions.name.has.periods"))),
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL(".thisextensionsnamestartswithaperiod"))),
+    NormalizeSeparators(FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL("thisextensionhasaslashinitsname/")))),
+  };
+  const FilePath expected_crx_names[] = {
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL("this.extensions.name.has.periods.crx"))),
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL(".thisextensionsnamestartswithaperiod.crx"))),
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL("thisextensionhasaslashinitsname.crx"))),
+  };
+  const FilePath expected_private_key_names[] = {
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL("this.extensions.name.has.periods.pem"))),
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL(".thisextensionsnamestartswithaperiod.pem"))),
+    FilePath(FilePath::StringType(
+        FILE_PATH_LITERAL("thisextensionhasaslashinitsname.pem"))),
+  };
+
+  for (size_t i = 0; i < arraysize(punctuated_names); ++i) {
+    SCOPED_TRACE(punctuated_names[i].value().c_str());
+    FilePath output_dir = temp_dir.path().Append(punctuated_names[i]);
+
+    // Copy the extension into the output directory, as PackExtensionJob doesn't
+    // let us choose where to output the packed extension.
+    ASSERT_TRUE(file_util::CopyDirectory(input_directory, output_dir, true));
+
+    FilePath expected_crx_path = temp_dir.path().Append(expected_crx_names[i]);
+    FilePath expected_private_key_path =
+        temp_dir.path().Append(expected_private_key_names[i]);
+    PackExtensionTestClient pack_client(expected_crx_path,
+                                        expected_private_key_path);
+    scoped_refptr<PackExtensionJob> packer(new PackExtensionJob(&pack_client,
+                                                                output_dir,
+                                                                FilePath()));
+    packer->Start();
+
+    // The packer will post a notification task to the current thread's message
+    // loop when it is finished.  We manually run the loop here so that we
+    // block and catch the notification; otherwise, the process would exit.
+    // This call to |Run()| is matched by a call to |Quit()| in the
+    // |PackExtensionTestClient|'s notification handling code.
+    MessageLoop::current()->Run();
+
+    if (HasFatalFailure())
+      return;
+
+    InstallExtension(expected_crx_path, true);
+  }
+}
+
 // Test Packaging and installing an extension using an openssl generated key.
 // The openssl is generated with the following:
 // > openssl genrsa -out privkey.pem 1024
diff --git a/chrome/browser/extensions/extensions_ui.cc b/chrome/browser/extensions/extensions_ui.cc
index a26ab07..3395286 100644
--- a/chrome/browser/extensions/extensions_ui.cc
+++ b/chrome/browser/extensions/extensions_ui.cc
@@ -628,18 +628,8 @@ void ExtensionsDOMHandler::HandlePackMessage(const Value* value) {
 
 void ExtensionsDOMHandler::OnPackSuccess(const FilePath& crx_file,
                                          const FilePath& pem_file) {
-  std::string message;
-  if (!pem_file.empty()) {
-    message = l10n_util::GetStringFUTF8(
-        IDS_EXTENSION_PACK_DIALOG_SUCCESS_BODY_NEW,
-        WideToUTF16(crx_file.ToWStringHack()),
-        WideToUTF16(pem_file.ToWStringHack()));
-  } else {
-    message = l10n_util::GetStringFUTF8(
-        IDS_EXTENSION_PACK_DIALOG_SUCCESS_BODY_UPDATE,
-        WideToUTF16(crx_file.ToWStringHack()));
-  }
-  ShowAlert(message);
+  ShowAlert(WideToUTF8(PackExtensionJob::StandardSuccessMessage(crx_file,
+                                                                pem_file)));
 
   ListValue results;
   dom_ui_->CallJavascriptFunction(L"hidePackDialog", results);
diff --git a/chrome/browser/extensions/extensions_ui.h b/chrome/browser/extensions/extensions_ui.h
index 2bf79e6..391cdc0 100644
--- a/chrome/browser/extensions/extensions_ui.h
+++ b/chrome/browser/extensions/extensions_ui.h
@@ -124,7 +124,7 @@ class ExtensionsDOMHandler
   virtual void OnPackSuccess(const FilePath& crx_file,
                              const FilePath& key_file);
 
-  virtual void OnPackFailure(const std::string& message);
+  virtual void OnPackFailure(const std::string& error);
 
   // ExtensionInstallUI::Delegate implementation, used for receiving
   // notification about uninstall confirmation dialog selections.
diff --git a/chrome/browser/extensions/pack_extension_job.cc b/chrome/browser/extensions/pack_extension_job.cc
index 19e26d8..1896c1e 100644
--- a/chrome/browser/extensions/pack_extension_job.cc
+++ b/chrome/browser/extensions/pack_extension_job.cc
@@ -4,15 +4,19 @@
 
 #include "chrome/browser/extensions/pack_extension_job.h"
 
+#include "app/l10n_util.h"
 #include "base/message_loop.h"
 #include "base/utf_string_conversions.h"
 #include "base/task.h"
 #include "chrome/browser/extensions/extension_creator.h"
+#include "chrome/common/chrome_constants.h"
+#include "grit/generated_resources.h"
 
 PackExtensionJob::PackExtensionJob(Client* client,
                                    const FilePath& root_directory,
                                    const FilePath& key_file)
-    : client_(client), root_directory_(root_directory), key_file_(key_file) {
+    : client_(client), key_file_(key_file) {
+  root_directory_ = root_directory.StripTrailingSeparators();
   CHECK(ChromeThread::GetCurrentThreadIdentifier(&client_thread_id_));
 }
 
@@ -27,10 +31,12 @@ void PackExtensionJob::ClearClient() {
 }
 
 void PackExtensionJob::RunOnFileThread() {
-  crx_file_out_ = root_directory_.ReplaceExtension(FILE_PATH_LITERAL("crx"));
+  crx_file_out_ = FilePath(root_directory_.value() +
+                           chrome::kExtensionFileExtension);
 
   if (key_file_.empty())
-    key_file_out_ = root_directory_.ReplaceExtension(FILE_PATH_LITERAL("pem"));
+    key_file_out_ = FilePath(root_directory_.value() +
+                             chrome::kExtensionKeyFileExtension);
 
   // TODO(aa): Need to internationalize the errors that ExtensionCreator
   // returns. See bug 20734.
@@ -38,22 +44,42 @@ void PackExtensionJob::RunOnFileThread() {
   if (creator.Run(root_directory_, crx_file_out_, key_file_, key_file_out_)) {
     ChromeThread::PostTask(
         client_thread_id_, FROM_HERE,
-        NewRunnableMethod(this, &PackExtensionJob::ReportSuccessOnUIThread));
+        NewRunnableMethod(this,
+                          &PackExtensionJob::ReportSuccessOnClientThread));
   } else {
     ChromeThread::PostTask(
         client_thread_id_, FROM_HERE,
         NewRunnableMethod(
-            this, &PackExtensionJob::ReportFailureOnUIThread,
+            this, &PackExtensionJob::ReportFailureOnClientThread,
             creator.error_message()));
   }
 }
 
-void PackExtensionJob::ReportSuccessOnUIThread() {
+void PackExtensionJob::ReportSuccessOnClientThread() {
   if (client_)
     client_->OnPackSuccess(crx_file_out_, key_file_out_);
 }
 
-void PackExtensionJob::ReportFailureOnUIThread(const std::string& error) {
+void PackExtensionJob::ReportFailureOnClientThread(const std::string& error) {
   if (client_)
     client_->OnPackFailure(error);
 }
+
+// static
+std::wstring PackExtensionJob::StandardSuccessMessage(const FilePath& crx_file,
+                                                      const FilePath& key_file)
+{
+  // TODO(isherman): we should use string16 instead of wstring.
+  // See crbug.com/23581 and crbug.com/24672
+  std::wstring message;
+  if (key_file.empty()) {
+    return l10n_util::GetStringF(
+        IDS_EXTENSION_PACK_DIALOG_SUCCESS_BODY_UPDATE,
+        crx_file.ToWStringHack());
+  } else {
+    return l10n_util::GetStringF(
+        IDS_EXTENSION_PACK_DIALOG_SUCCESS_BODY_NEW,
+        crx_file.ToWStringHack(),
+        key_file.ToWStringHack());
+  }
+}
diff --git a/chrome/browser/extensions/pack_extension_job.h b/chrome/browser/extensions/pack_extension_job.h
index d3fc223..48c0a54 100644
--- a/chrome/browser/extensions/pack_extension_job.h
+++ b/chrome/browser/extensions/pack_extension_job.h
@@ -41,14 +41,18 @@ class PackExtensionJob : public base::RefCountedThreadSafe<PackExtensionJob> {
   // PackExtensionJob from attempting to access it.
   void ClearClient();
 
+  // The standard packing success message.
+  static std::wstring StandardSuccessMessage(const FilePath& crx_file,
+                                             const FilePath& key_file);
+
  private:
   friend class base::RefCountedThreadSafe<PackExtensionJob>;
 
   ~PackExtensionJob() {}
 
   void RunOnFileThread();
-  void ReportSuccessOnUIThread();
-  void ReportFailureOnUIThread(const std::string& error);
+  void ReportSuccessOnClientThread();
+  void ReportFailureOnClientThread(const std::string& error);
 
   ChromeThread::ID client_thread_id_;
   Client* client_;