Extension package creation cleanup

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.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55792

[isherman, 2010-08-05 04:57:30]

This is still a WIP -- notably, it is abusing the threading system and needs
unit tests -- but I'd appreciate some high-level feedback on what I've got so
far.  Especially: any suggestions for how to handle the threads properly would
be much appreciated :)

[Matt Perry, 2010-08-05 22:16:30]

This is a good start, and close to what I had envisioned. We just need to spin
up a blocking message loop to wait for the job to finish.

Do we have a file thread at this point? If not, you might have to manually start
it. Searching for ChromeThread::FILE might give you clues how to do that, or
putting a breakpoint in Thread::Start.

http://codereview.chromium.org/3077022/diff/3001/4001
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3077022/diff/3001/4001#newcode913
chrome/browser/browser_init.cc:913: // TODO inherit from
base::RefCountedThreadSafe<PackExtensionLogger> ?
There's only one guy at a time with a handle to this class (the PackJob), so you
shouldn't need to ref count it.

http://codereview.chromium.org/3077022/diff/3001/4001#newcode969
chrome/browser/browser_init.cc:969: delete this; // TODO this probably isn't
kosher...
it's fine if you can guarantee this class is never stack allocated.

http://codereview.chromium.org/3077022/diff/3001/4001#newcode1020
chrome/browser/browser_init.cc:1020: //      to suggestions.
This is fine. We use this pattern a lot (client class owns itself, deletes
itself when it gets a notification).

http://codereview.chromium.org/3077022/diff/3001/4001#newcode1027
chrome/browser/browser_init.cc:1027: // TODO wait on the job to finish?
I think you can do MessageLoop::current()->Run(). Replace the
CloseALlBrowsersAndExit with a call to MessageLoop::current()->Quit(). Run()
will block until the Quit is called, then returning false should end the
process.

Alternatively, I think you can just return true here to allow the regular
message loop to start. Keep the call to Quit in ShowPackExtensionMessage.

Try both and see which works better.

[Matt Perry, 2010-08-05 23:56:09]

Awesome! LGTM with small comment changes.

http://codereview.chromium.org/3077022/diff/8001/9001
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3077022/diff/8001/9001#newcode968
chrome/browser/browser_init.cc:968: MessageLoop::current()->Quit();
add a note saying that this matches the Run() in ProcessCmdLineImpl

http://codereview.chromium.org/3077022/diff/8001/9001#newcode1024
chrome/browser/browser_init.cc:1024: // failure back to the user.
this comment is not quite accurate. The main point of running the loop is to
block here until Quit is called (because if we didn't block, the process would
exit).

http://codereview.chromium.org/3077022/diff/8001/9001#newcode1025
chrome/browser/browser_init.cc:1025: MessageLoop::current()->Run();
add a note saying that this is matched by the Quit in PackExtensionLogger

[Elliot Glaysher, 2010-08-05 23:58:57]

http://codereview.chromium.org/3077022/diff/8001/9001
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3077022/diff/8001/9001#newcode931
chrome/browser/browser_init.cc:931: //      right thing to do?
nit: Format is "TODO(ldap): "

[Matt Perry, 2010-08-06 00:03:54]

couple style issues I forgot

http://codereview.chromium.org/3077022/diff/8001/9001
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3077022/diff/8001/9001#newcode931
chrome/browser/browser_init.cc:931: //      right thing to do?
On 2010/08/05 23:58:57, Elliot Glaysher wrote:
> nit: Format is "TODO(ldap): "

also, for consistency, don't indent the text in the wrapping lines.

http://codereview.chromium.org/3077022/diff/8001/9004
File chrome/browser/extensions/pack_extension_job.cc (right):

http://codereview.chromium.org/3077022/diff/8001/9004#newcode45
chrome/browser/extensions/pack_extension_job.cc:45: NewRunnableMethod(this,
&PackExtensionJob::ReportSuccessOnClientThread));
80 cols

[isherman, 2010-08-07 03:24:39]

I think the patch should be functionally complete now.  Mostly this revision
just adds unit tests and cleans up the code a bit.

http://codereview.chromium.org/3077022/diff/15001/16002
File chrome/browser/extensions/extensions_service_unittest.cc (left):

http://codereview.chromium.org/3077022/diff/15001/16002#oldcode605
chrome/browser/extensions/extensions_service_unittest.cc:605:
FilePath::StringType ret_val;
This change isn't really related to the rest of what this patch is addressing,
but it's an unused variable.  Should I move that into a quick separate patch,
just to keep the revision logs clearer?

http://codereview.chromium.org/3077022/diff/15001/16002
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/3077022/diff/15001/16002#newcode622
chrome/browser/extensions/extensions_service_unittest.cc:622:
expected_private_key_path_(expected_private_key_path) {}
Not sure what the correct style for these lines is -- did I happen to do it
right?

[Paweł Hajdan Jr., 2010-08-09 16:54:15]

Drive-by with some test comments.

http://codereview.chromium.org/3077022/diff/18001/19002
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/3077022/diff/18001/19002#newcode639
chrome/browser/extensions/extensions_service_unittest.cc:639:
ASSERT_TRUE(false);
The right way to say that is FAIL(). Or ADD_FAILURE if you prefer.

http://codereview.chromium.org/3077022/diff/18001/19002#newcode1019
chrome/browser/extensions/extensions_service_unittest.cc:1019: for (size_t i =
0; i < arraysize(punctuated_names); ++i) {
Could you use SCOPED_TRACE to make the failure messages more useful inside this
loop?

[Matt Perry, 2010-08-09 21:08:29]

LGTM, a few small things

http://codereview.chromium.org/3077022/diff/24001/25001
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3077022/diff/24001/25001#newcode913
chrome/browser/browser_init.cc:913: class PackExtensionLogger : public
PackExtensionJob::Client {
please add a quick comment describing the purpose of this class

http://codereview.chromium.org/3077022/diff/24001/25002
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/3077022/diff/24001/25002#newcode638
chrome/browser/extensions/extensions_service_unittest.cc:638: FAIL() << "Packing
should not fail.";
I've never seen this macro used in a unittest. Does it crash the process? You
probably want to use ASSERT_TRUE(false) here.

http://codereview.chromium.org/3077022/diff/24001/25002#newcode1025
chrome/browser/extensions/extensions_service_unittest.cc:1025: true));
nit: looks like this can fit on one line

http://codereview.chromium.org/3077022/diff/24001/25002#newcode1047
chrome/browser/extensions/extensions_service_unittest.cc:1047:
InstallExtension(expected_crx_path, true);
why install it? you're only testing that packing works.

[isherman, 2010-08-09 22:43:56]

http://codereview.chromium.org/3077022/diff/24001/25002
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/3077022/diff/24001/25002#newcode638
chrome/browser/extensions/extensions_service_unittest.cc:638: FAIL() << "Packing
should not fail.";
On 2010/08/09 21:08:29, Matt Perry wrote:
> I've never seen this macro used in a unittest. Does it crash the process? You
> probably want to use ASSERT_TRUE(false) here.

It doesn't crash the process; it seems to have essentially the same effect as
ASSERT_TRUE(false) + some logging.  phajdan.jr specifically suggested suggested
that FAIL() would be better here -- I had ASSERT_TRUE(false) earlier :)

http://codereview.chromium.org/3077022/diff/24001/25002#newcode1047
chrome/browser/extensions/extensions_service_unittest.cc:1047:
InstallExtension(expected_crx_path, true);
On 2010/08/09 21:08:29, Matt Perry wrote:
> why install it? you're only testing that packing works.

I want to make sure that it unpacks properly as well.  Is there a simpler/more
straightforward way to do that?

[Matt Perry, 2010-08-09 22:47:07]

still lgtm

http://codereview.chromium.org/3077022/diff/24001/25002
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/3077022/diff/24001/25002#newcode638
chrome/browser/extensions/extensions_service_unittest.cc:638: FAIL() << "Packing
should not fail.";
On 2010/08/09 22:43:56, isherman wrote:
> On 2010/08/09 21:08:29, Matt Perry wrote:
> > I've never seen this macro used in a unittest. Does it crash the process?
You
> > probably want to use ASSERT_TRUE(false) here.
> 
> It doesn't crash the process; it seems to have essentially the same effect as
> ASSERT_TRUE(false) + some logging.  phajdan.jr specifically suggested
suggested
> that FAIL() would be better here -- I had ASSERT_TRUE(false) earlier :)

Ah, ignore me then :)

http://codereview.chromium.org/3077022/diff/24001/25002#newcode1047
chrome/browser/extensions/extensions_service_unittest.cc:1047:
InstallExtension(expected_crx_path, true);
On 2010/08/09 22:43:56, isherman wrote:
> On 2010/08/09 21:08:29, Matt Perry wrote:
> > why install it? you're only testing that packing works.
> 
> I want to make sure that it unpacks properly as well.  Is there a simpler/more
> straightforward way to do that?

IMO that is outside of the scope of this test, but I don't feel strongly about
it. Your call.

[isherman, 2010-08-09 22:55:31]

http://codereview.chromium.org/3077022/diff/24001/25002
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/3077022/diff/24001/25002#newcode1047
chrome/browser/extensions/extensions_service_unittest.cc:1047:
InstallExtension(expected_crx_path, true);
On 2010/08/09 22:47:07, Matt Perry wrote:
> On 2010/08/09 22:43:56, isherman wrote:
> > On 2010/08/09 21:08:29, Matt Perry wrote:
> > > why install it? you're only testing that packing works.
> > 
> > I want to make sure that it unpacks properly as well.  Is there a
simpler/more
> > straightforward way to do that?
> 
> IMO that is outside of the scope of this test, but I don't feel strongly about
> it. Your call.

Well, one of the issues this patch addresses -- bug 19103 -- is specifically
about unpacking properly.  I guess I could split that off into a separate test,
but it seems like that would mostly just lead to a lot of replicated code.  If
you think it's ok like this, I think I'd rather just leave it as is.

[Matt Perry, 2010-08-09 23:00:53]

On Mon, Aug 9, 2010 at 3:55 PM, <isherman@google.com> wrote:

>
> http://codereview.chromium.org/3077022/diff/24001/25002
> File chrome/browser/extensions/extensions_service_unittest.cc (right):
>
> http://codereview.chromium.org/3077022/diff/24001/25002#newcode1047
> chrome/browser/extensions/extensions_service_unittest.cc:1047:
> InstallExtension(expected_crx_path, true);
> On 2010/08/09 22:47:07, Matt Perry wrote:
>
>> On 2010/08/09 22:43:56, isherman wrote:
>> > On 2010/08/09 21:08:29, Matt Perry wrote:
>> > > why install it? you're only testing that packing works.
>> >
>> > I want to make sure that it unpacks properly as well.  Is there a
>>
> simpler/more
>
>> > straightforward way to do that?
>>
>
>  IMO that is outside of the scope of this test, but I don't feel
>>
> strongly about
>
>> it. Your call.
>>
>
> Well, one of the issues this patch addresses -- bug 19103 -- is
> specifically about unpacking properly.  I guess I could split that off
> into a separate test, but it seems like that would mostly just lead to a
> lot of replicated code.  If you think it's ok like this, I think I'd
> rather just leave it as is.


Sounds good.

[Paweł Hajdan Jr., 2010-08-09 23:40:58]

Code I commented in the drive-by LGTM.