Tests for incognito app install, plus some cleanup.

Tests for incognito app install, plus some cleanup.


BUG=62752
TEST=browser_tests

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

[Tessa MacDuff, 2010-12-03 23:33:41]

I'm playing with extension_install_ui_browsertest.cc to see if I can add a
useful test for this.  But it's taking me longer than I expected so I figured I
should start this review...

[Aaron Boodman, 2010-12-04 00:43:19]

Ah, nice easy fix. LGTM.

[Aaron Boodman, 2010-12-04 00:43:39]

If you can find a good way to test, hooray, but it's not critical for this
change.

[Tessa MacDuff, 2010-12-14 23:02:11]

I've uploaded my browsertest.

The thing I was most concerned about was the changes to
extension_browsertest.h/cc.  I don't want to be adding too much extra code just
for these two little tests so I tried to keep it minimal and easy to
update/extend for future tests.  Let me know what you think.

[Aaron Boodman, 2010-12-14 23:21:44]

This seems pretty good.

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.cc:125:
MockAutoConfirmExtensionInstallUI(Profile* profile) :
ExtensionInstallUI(profile) {}
80 cols

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_ui_browsertest.cc:71:
EXPECT_EQ(num_tabs + 1, browser()->tab_count());
Is it also possible to test that the new tab is the NTP URL? (see
chrome/common/url_constants.h - kChromeUINewTabURL)

[Tessa MacDuff, 2010-12-15 00:38:55]

Do I need to worry about the app-id?  Can it change?

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.cc:125:
MockAutoConfirmExtensionInstallUI(Profile* profile) :
ExtensionInstallUI(profile) {}
On 2010/12/14 23:21:44, Aaron Boodman wrote:
> 80 cols

Done.

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

http://codereview.chromium.org/5543001/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_ui_browsertest.cc:71:
EXPECT_EQ(num_tabs + 1, browser()->tab_count());
On 2010/12/14 23:21:44, Aaron Boodman wrote:
> Is it also possible to test that the new tab is the NTP URL? (see
> chrome/common/url_constants.h - kChromeUINewTabURL)

Done.

[Aaron Boodman, 2010-12-15 00:46:15]

LGTM

On 2010/12/15 00:38:55, Tessa MacDuff wrote:
> Do I need to worry about the app-id?  Can it change?

The app-id is generated when a crx is created and baked into it. It can only
change if you recreate the app.

BTW, don't check in the .pem file. It isn't necessary.

[Tessa MacDuff, 2010-12-15 21:00:24]

Per discussion last night (try failure when CL contains new crx file), I removed
the CRX and added support to ExtensionBrowserTest for packing an extension.

[Aaron Boodman, 2010-12-15 22:29:18]

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.cc:111:
ASSERT_TRUE(file_util::Delete(*crx_path, false));
You should double-check that this returns true in the case where the supplied
file doesn't exist.

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.cc:189: if (path.Extension() ==
FILE_PATH_LITERAL(".crx")) {
Can you fix all the callers so that we don't need the support for installing crx
files anymore?

Sorry :| Doesn't look like there's too many.

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.h:34: void PackExtension(const
FilePath& dir_path, FilePath* crx_path);
This is fine, but you could also return the FilePath for a slightly easier to
use API. An empty/invalid FilePath would mean that packing failed.

[Tessa MacDuff, 2010-12-16 00:08:04]

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.cc:111:
ASSERT_TRUE(file_util::Delete(*crx_path, false));
On 2010/12/15 22:29:18, Aaron Boodman wrote:
> You should double-check that this returns true in the case where the supplied
> file doesn't exist.

Done.

There is even a test that checks this:
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/file_uti...

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.cc:189: if (path.Extension() ==
FILE_PATH_LITERAL(".crx")) {
On 2010/12/15 22:29:18, Aaron Boodman wrote:
> Can you fix all the callers so that we don't need the support for installing
crx
> files anymore?
> 
> Sorry :| Doesn't look like there's too many.

Do you mind if I do it in a separate CL?  I'll leave a TODO as collateral.

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.h:34: void PackExtension(const
FilePath& dir_path, FilePath* crx_path);
On 2010/12/15 22:29:18, Aaron Boodman wrote:
> This is fine, but you could also return the FilePath for a slightly easier to
> use API. An empty/invalid FilePath would mean that packing failed.

Yes, that means I'll need to remove all the ASSERTs so it will be slightly
harder to debug if it fails.  But I agree, impler API seems a better tradeoff.

[Aaron Boodman, 2011-01-14 20:57:39]

Sorry, what ASSERTs do you mean here?

[Aaron Boodman, 2011-01-14 20:58:43]

Picking up ancient change that seems to have fallen through the cracks.

Doing the changes to existing tests is fine for a separate CL.

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.cc:189: if (path.Extension() ==
FILE_PATH_LITERAL(".crx")) {
On 2010/12/16 00:08:04, Tessa MacDuff wrote:
> On 2010/12/15 22:29:18, Aaron Boodman wrote:
> > Can you fix all the callers so that we don't need the support for installing
> crx
> > files anymore?
> > 
> > Sorry :| Doesn't look like there's too many.
> 
> Do you mind if I do it in a separate CL?  I'll leave a TODO as collateral.

No that is fine.

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.h:34: void PackExtension(const
FilePath& dir_path, FilePath* crx_path);
On 2010/12/16 00:08:04, Tessa MacDuff wrote:
> On 2010/12/15 22:29:18, Aaron Boodman wrote:
> > This is fine, but you could also return the FilePath for a slightly easier
to
> > use API. An empty/invalid FilePath would mean that packing failed.
> 
> Yes, that means I'll need to remove all the ASSERTs so it will be slightly
> harder to debug if it fails.  But I agree, impler API seems a better tradeoff.

What asserts do you mean here?

[Tessa MacDuff, 2011-01-20 22:20:24]

Ready for another look.

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_browsertest.h (right):

http://codereview.chromium.org/5543001/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_browsertest.h:34: void PackExtension(const
FilePath& dir_path, FilePath* crx_path);
On 2011/01/14 20:58:43, Aaron Boodman wrote:
> On 2010/12/16 00:08:04, Tessa MacDuff wrote:
> > On 2010/12/15 22:29:18, Aaron Boodman wrote:
> > > This is fine, but you could also return the FilePath for a slightly easier
> to
> > > use API. An empty/invalid FilePath would mean that packing failed.
> > 
> > Yes, that means I'll need to remove all the ASSERTs so it will be slightly
> > harder to debug if it fails.  But I agree, impler API seems a better
tradeoff.
> 
> What asserts do you mean here?

There are a bunch of ASSERT_TRUEs in the body of this function.

They are only allowed by the compiler if the function has no/void return type. 
I changed them to if...ADD_FAILURE()...return FilePath() as discussed.

[Aaron Boodman, 2011-01-20 23:00:17]

lgtm