chrome.exe --package-extension

chrome.exe --package-extension generates .crx from extension directory

R=aa
BUG=12114
Committed: http://src.chromium.org/viewvc/17927

[rafaelw, 2009-06-07 02:18:37]

please review pending unittests

[Aaron Boodman, 2009-06-07 02:38:28]

Logic seems good. Just factoring/style nits.

http://codereview.chromium.org/118328/diff/36/38
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/118328/diff/36/38#newcode687
Line 687: // TODO(aa): Is it OK to just return here?
I think so.

http://codereview.chromium.org/118328/diff/36/39
File chrome/browser/extensions/extension_creator.cc (right):

http://codereview.chromium.org/118328/diff/36/39#newcode28
Line 28: const char kExtensionFileMagic[] = "Cr24";
These should be exposed from ExtensionsService to avoid copying them into this
file.

http://codereview.chromium.org/118328/diff/36/39#newcode58
Line 58: std::string(kKeyBeginHeaderMarker).length());
If you instead declare the markers as char[] instead of char*, you can do
sizeof(kMarkerConstant) to avoid the temporary string.

http://codereview.chromium.org/118328/diff/36/39#newcode85
Line 85: bool ExtensionCreator::FormatPEMForFileOutput(const std::string input,
I can see why you wanted an rsa_util. I think this is fine for  now since base64
is in net, but can you create a bug to move base64 into base and to refactor the
PEM handling code into base/crypto?

http://codereview.chromium.org/118328/diff/36/39#newcode116
Line 116: bool ExtensionCreator::CreateExtension(const CommandLine&
command_line) {
I think it would be cleaner to nix the static methods and make this class not
know why the extension is being created. 

So its input parameters should just be the input directory, the output file,
etc. The output parameters would just be the error if any.

It should be up to the caller (browser_init in this case, but maybe UI in the
future) to decide how to get any input and what to do with any output.

http://codereview.chromium.org/118328/diff/36/39#newcode118
Line 118: ExtensionErrorReporter::GetInstance();
I'd like to print errors (and success) to the console. I don't actually know how
to do this. printf() doesn't work. I was going to look at how LOG works, since
with the right flags, that does print to the console.

http://codereview.chromium.org/118328/diff/36/39#newcode148
Line 148: if (!file_util::DirectoryExists(extension_dir_)) {
This function is pretty long. Consider breaking up into steps.

http://codereview.chromium.org/118328/diff/36/39#newcode167
Line 167: if (!manifest.get()) {
JSONFileValueSerializer can return a detailed error message that you could print
out.

http://codereview.chromium.org/118328/diff/36/39#newcode197
Line 197: key_pair.reset(base::RSAPrivateKey::Create(1024));
Technically it can, but I think it would be pretty rare.

http://codereview.chromium.org/118328/diff/36/39#newcode223
Line 223: error_message_ = "Private key exists next to input directory. Try
using "
Good idea :)

http://codereview.chromium.org/118328/diff/36/40
File chrome/browser/extensions/extension_creator.h (right):

http://codereview.chromium.org/118328/diff/36/40#newcode11
Line 11: class ExtensionCreator {
Need class comments.

http://codereview.chromium.org/118328/diff/36/42
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/118328/diff/36/42#newcode397
Line 397: const wchar_t kPackageExtension[]              = L"package-extension";
How about --pack-extension instead to go better with a future
--unpack-extension?

[rafaelw, 2009-06-08 00:44:03]

check it

http://codereview.chromium.org/118328/diff/36/38
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/118328/diff/36/38#newcode687
Line 687: // TODO(aa): Is it OK to just return here?
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> I think so.

Done.

http://codereview.chromium.org/118328/diff/36/39
File chrome/browser/extensions/extension_creator.cc (right):

http://codereview.chromium.org/118328/diff/36/39#newcode28
Line 28: const char kExtensionFileMagic[] = "Cr24";
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> These should be exposed from ExtensionsService to avoid copying them into this
> file.

Done.

http://codereview.chromium.org/118328/diff/36/39#newcode58
Line 58: std::string(kKeyBeginHeaderMarker).length());
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> If you instead declare the markers as char[] instead of char*, you can do
> sizeof(kMarkerConstant) to avoid the temporary string.

Cool. done.

http://codereview.chromium.org/118328/diff/36/39#newcode85
Line 85: bool ExtensionCreator::FormatPEMForFileOutput(const std::string input,
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> I can see why you wanted an rsa_util. I think this is fine for  now since
base64
> is in net, but can you create a bug to move base64 into base and to refactor
the
> PEM handling code into base/crypto?

Done.

http://codereview.chromium.org/118328/diff/36/39#newcode116
Line 116: bool ExtensionCreator::CreateExtension(const CommandLine&
command_line) {
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> I think it would be cleaner to nix the static methods and make this class not
> know why the extension is being created. 
> 
> So its input parameters should just be the input directory, the output file,
> etc. The output parameters would just be the error if any.
> 
> It should be up to the caller (browser_init in this case, but maybe UI in the
> future) to decide how to get any input and what to do with any output.

Done.

http://codereview.chromium.org/118328/diff/36/39#newcode118
Line 118: ExtensionErrorReporter::GetInstance();
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> I'd like to print errors (and success) to the console. I don't actually know
how
> to do this. printf() doesn't work. I was going to look at how LOG works, since
> with the right flags, that does print to the console.

This looks tricky. Right now errors cause a message box modal dialog.

http://codereview.chromium.org/118328/diff/36/39#newcode148
Line 148: if (!file_util::DirectoryExists(extension_dir_)) {
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> This function is pretty long. Consider breaking up into steps.

Done.

http://codereview.chromium.org/118328/diff/36/39#newcode167
Line 167: if (!manifest.get()) {
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> JSONFileValueSerializer can return a detailed error message that you could
print
> out.

Done.

http://codereview.chromium.org/118328/diff/36/39#newcode197
Line 197: key_pair.reset(base::RSAPrivateKey::Create(1024));
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> Technically it can, but I think it would be pretty rare.

Done.

http://codereview.chromium.org/118328/diff/36/39#newcode223
Line 223: error_message_ = "Private key exists next to input directory. Try
using "
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> Good idea :)

Sweet.

http://codereview.chromium.org/118328/diff/36/40
File chrome/browser/extensions/extension_creator.h (right):

http://codereview.chromium.org/118328/diff/36/40#newcode11
Line 11: class ExtensionCreator {
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> Need class comments.

Done.

http://codereview.chromium.org/118328/diff/36/42
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/118328/diff/36/42#newcode397
Line 397: const wchar_t kPackageExtension[]              = L"package-extension";
On 2009/06/07 02:38:28, Aaron Boodman wrote:
> How about --pack-extension instead to go better with a future
> --unpack-extension?

Done.

[rafaelw, 2009-06-08 15:28:58]

Had to move PEM functions to extension.cc/h because in the signature that's
coming next, extension needs in (and it's in common).

Also, fixed a small bug in browser_init (correctly named output privkey ".pem").

[Aaron Boodman, 2009-06-08 18:31:36]

http://codereview.chromium.org/118328/diff/1041/67
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/118328/diff/1041/67#newcode687
Line 687: ExtensionErrorReporter *error_reporter =
I'd rather not use ExtensionErrorReporter here. It's not really meant to handle
this case of being run from the command line, so I expect porters will break it.

Instead, why not move the error handling you have here for the paths being valid
into ExtensionCreator. That way, this could will just fetch values from the
command line and not doing anything with them, EEC will do all the error
handling, but not any error reporting. Then this code can just have an #if
defined(OS_WINDOWS) and put up a message box with a NOTIMPLEMENTED() for the
other platforms.

http://codereview.chromium.org/118328/diff/1041/68
File chrome/browser/extensions/extension_creator.cc (right):

http://codereview.chromium.org/118328/diff/1041/68#newcode3
Line 3: // found in the LICENSE file.
I like the new split apart methods.

http://codereview.chromium.org/118328/diff/1041/68#newcode94
Line 94: error_message_ = "Yikes! Failed to generate random RSA private key.";
:)

http://codereview.chromium.org/118328/diff/1041/68#newcode121
Line 121: "--package-extension-key";
--pack-extension-key ?

http://codereview.chromium.org/118328/diff/1041/68#newcode184
Line 184: error_message_ = "Error while signing extension.";
I think some of these are DCHECK-level failures. If base64encode fails, we have
bigger problems :). For this one (and a few others I'll mention in other files,
I think it is fine to just DCHECK and continue.

http://codereview.chromium.org/118328/diff/1041/69
File chrome/browser/extensions/extension_creator.h (right):

http://codereview.chromium.org/118328/diff/1041/69#newcode16
Line 16: // private key that is either provided in |private_key_path| or is
internall
typo: internall

http://codereview.chromium.org/118328/diff/1041/69#newcode17
Line 17: // generated randomly (and optionally written to
|output_private_key_path|.
Nice comment.

http://codereview.chromium.org/118328/diff/1041/69#newcode22
Line 22: bool Run(const FilePath& extension_dir,
May want a short comment here explaining the params.

http://codereview.chromium.org/118328/diff/1041/69#newcode31
Line 31: // Verify's input directory's existance, and reads manifest.
Verify's -> Verifies (also in each of the private methods below).

http://codereview.chromium.org/118328/diff/1041/69#newcode35
Line 35: base::RSAPrivateKey* InputKey(const FilePath& private_key_path);
Method names should be verbs, so... ReadInputKey() ?

http://codereview.chromium.org/118328/diff/1041/69#newcode41
Line 41: // Create's temporary zip file and generates a signature from it.
Typo from -> for

http://codereview.chromium.org/118328/diff/1041/69#newcode47
Line 47: // Inserts generate keys (signature, public_key) into manifest.
Typo: generate -> generated

http://codereview.chromium.org/118328/diff/1041/69#newcode54
Line 54: const FilePath& zip_path);
indent

http://codereview.chromium.org/118328/diff/1041/76
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/118328/diff/1041/76#newcode528
Line 528: std::string* output) {
Weird indent. Also, it looks like maybe this method is not necessary since all
it does is delegate to base64? Maybe that could become part of a method like
FormatPrivateKeyForPemFile()

http://codereview.chromium.org/118328/diff/1041/76#newcode537
Line 537: std::string* output,
Weird indent.

[rafaelw, 2009-06-08 19:28:06]

http://codereview.chromium.org/118328/diff/1041/67
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/118328/diff/1041/67#newcode687
Line 687: ExtensionErrorReporter *error_reporter =
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> I'd rather not use ExtensionErrorReporter here. It's not really meant to
handle
> this case of being run from the command line, so I expect porters will break
it.
> 
> Instead, why not move the error handling you have here for the paths being
valid
> into ExtensionCreator. That way, this could will just fetch values from the
> command line and not doing anything with them, EEC will do all the error
> handling, but not any error reporting. Then this code can just have an #if
> defined(OS_WINDOWS) and put up a message box with a NOTIMPLEMENTED() for the
> other platforms.

Done.

http://codereview.chromium.org/118328/diff/1041/68
File chrome/browser/extensions/extension_creator.cc (right):

http://codereview.chromium.org/118328/diff/1041/68#newcode121
Line 121: "--package-extension-key";
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> --pack-extension-key ?

Done.

http://codereview.chromium.org/118328/diff/1041/68#newcode184
Line 184: error_message_ = "Error while signing extension.";
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> I think some of these are DCHECK-level failures. If base64encode fails, we
have
> bigger problems :). For this one (and a few others I'll mention in other
files,
> I think it is fine to just DCHECK and continue.

Done.

http://codereview.chromium.org/118328/diff/1041/69
File chrome/browser/extensions/extension_creator.h (right):

http://codereview.chromium.org/118328/diff/1041/69#newcode16
Line 16: // private key that is either provided in |private_key_path| or is
internall
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> typo: internall

Done.

http://codereview.chromium.org/118328/diff/1041/69#newcode22
Line 22: bool Run(const FilePath& extension_dir,
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> May want a short comment here explaining the params.

Done.

http://codereview.chromium.org/118328/diff/1041/69#newcode35
Line 35: base::RSAPrivateKey* InputKey(const FilePath& private_key_path);
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> Method names should be verbs, so... ReadInputKey() ?

Done.

http://codereview.chromium.org/118328/diff/1041/69#newcode41
Line 41: // Create's temporary zip file and generates a signature from it.
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> Typo from -> for

Done.

http://codereview.chromium.org/118328/diff/1041/69#newcode47
Line 47: // Inserts generate keys (signature, public_key) into manifest.
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> Typo: generate -> generated

Done.

http://codereview.chromium.org/118328/diff/1041/69#newcode54
Line 54: const FilePath& zip_path);
On 2009/06/08 18:31:36, Aaron Boodman wrote:
> indent

Done.

[Aaron Boodman, 2009-06-08 19:33:01]

lgtm!

http://codereview.chromium.org/118328/diff/86/1046
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/118328/diff/86/1046#newcode47
Line 47: #include "chrome/common/extensions/extension_error_reporter.h"
This can be removed now.

http://codereview.chromium.org/118328/diff/86/1046#newcode700
Line 700: std::wstring parent_dir = src_dir.value();
This won't work on mac/linux (value() is not a wstring there).

You can use src_dir.DirName() to get the parent instead in a cross-platform way.

[rafaelw, 2009-06-08 19:44:18]

http://codereview.chromium.org/118328/diff/86/1046
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/118328/diff/86/1046#newcode47
Line 47: #include "chrome/common/extensions/extension_error_reporter.h"
On 2009/06/08 19:33:01, Aaron Boodman wrote:
> This can be removed now.

Done.

http://codereview.chromium.org/118328/diff/86/1046#newcode700
Line 700: std::wstring parent_dir = src_dir.value();
On 2009/06/08 19:33:01, Aaron Boodman wrote:
> This won't work on mac/linux (value() is not a wstring there).
> 
> You can use src_dir.DirName() to get the parent instead in a cross-platform
way.

Done.