Add a ExtensionBrowserTest base class

Add an ExtensionBrowserTest base class that allows in-process browser tests of extensions using ExtensionsService directly, rather than TestExtensionLoaded. Use it to re-enable some old browser tests that had been disabled.

[Aaron Boodman, 2009-07-02 08:12:49]

Add an ExtensionBrowserTest base class that allows in-process browser tests of
extensions using ExtensionsService directly, rather than TestExtensionLoaded.
Use it to re-enable some old browser tests that had been disabled.

[Matt Perry, 2009-07-02 19:29:24]

Mostly looks good.  Just a few questions.

http://codereview.chromium.org/150213/diff/37/38
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/150213/diff/37/38#newcode41
Line 41: // This enables DOM automation for tab contentses.
lolzes

http://codereview.chromium.org/150213/diff/37/38#newcode53
Line 53: void LoadExtension(const FilePath& path) {
This mechanism is pretty much what TestExtensionLoader did.  Is there something
that makes this way more reliable?

http://codereview.chromium.org/150213/diff/37/38#newcode58
Line 58: ui_test_utils::RunMessageLoop();
no delayed QuitTask?  if the load fails, we'll hang here.

http://codereview.chromium.org/150213/diff/37/38#newcode105
Line 105: case NotificationType::EXTENSIONS_LOADED:
What if there's an extension load pending when someone calls LoadExtension? 
Then we would catch that first load and Quit here, and the LoadExtension caller
would think his extension is ready too early.

I think something like that used to happen if you ran the tests with an unclean
profile.  Maybe we don't do that now?

http://codereview.chromium.org/150213/diff/37/38#newcode145
Line 145: host->render_view_host(), L"", L"testTabsAPI()", &result);
sweet, much better than "test that alert() was called"

[Aaron Boodman, 2009-07-02 19:39:54]

http://codereview.chromium.org/150213/diff/37/38
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/150213/diff/37/38#newcode53
Line 53: void LoadExtension(const FilePath& path) {
On 2009/07/02 19:29:24, Matt Perry wrote:
> This mechanism is pretty much what TestExtensionLoader did.  Is there
something
> that makes this way more reliable?

Actually, no. I thought that TestExtensionLoaded did something different. I must
have been remembering some earlier version of it. The only thing that's
different about this code is that it does not call Init().

For some reason I was thinking the problem with these tests was memory leaks,
related to some weird way that we were loading extensions with
TestExtensionLoader. Now that I look at the actual bugs, I see we disabled them
because they hung on linux. I wonder if this is fixed now that unpacking works
on linux? Was there some other issue with these tests?

http://codereview.chromium.org/150213/diff/37/38#newcode58
Line 58: ui_test_utils::RunMessageLoop();
On 2009/07/02 19:29:24, Matt Perry wrote:
> no delayed QuitTask?  if the load fails, we'll hang here.

I think that the test runner will kill us if that happens, so it does not seem
like a big deal. I guess I could put a timeout in, but I didn't see the
advantage.

http://codereview.chromium.org/150213/diff/37/38#newcode105
Line 105: case NotificationType::EXTENSIONS_LOADED:
On 2009/07/02 19:29:24, Matt Perry wrote:
> What if there's an extension load pending when someone calls LoadExtension? 
> Then we would catch that first load and Quit here, and the LoadExtension
caller
> would think his extension is ready too early.
> 
> I think something like that used to happen if you ran the tests with an
unclean
> profile.  Maybe we don't do that now?

I was thinking that each test gets a clean profile, but I will double-check.

[Matt Perry, 2009-07-02 19:46:32]

LGTM, though I still suggest a timeout (could be a long one, on the order of
minutes)

http://codereview.chromium.org/150213/diff/37/38
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/150213/diff/37/38#newcode53
Line 53: void LoadExtension(const FilePath& path) {
On 2009/07/02 19:39:54, Aaron Boodman wrote:
> On 2009/07/02 19:29:24, Matt Perry wrote:
> > This mechanism is pretty much what TestExtensionLoader did.  Is there
> something
> > that makes this way more reliable?
> 
> Actually, no. I thought that TestExtensionLoaded did something different. I
must
> have been remembering some earlier version of it. The only thing that's
> different about this code is that it does not call Init().
> 
> For some reason I was thinking the problem with these tests was memory leaks,
> related to some weird way that we were loading extensions with
> TestExtensionLoader. Now that I look at the actual bugs, I see we disabled
them
> because they hung on linux. I wonder if this is fixed now that unpacking works
> on linux? Was there some other issue with these tests?

Actually, I thought it was memory issues as well.  I'm not sure.

I like your refactoring though.  Things are cleaner this way.  Let's run it
through the trybots and check it in, just watch for problems.

http://codereview.chromium.org/150213/diff/37/38#newcode58
Line 58: ui_test_utils::RunMessageLoop();
On 2009/07/02 19:39:54, Aaron Boodman wrote:
> On 2009/07/02 19:29:24, Matt Perry wrote:
> > no delayed QuitTask?  if the load fails, we'll hang here.
> 
> I think that the test runner will kill us if that happens, so it does not seem
> like a big deal. I guess I could put a timeout in, but I didn't see the
> advantage.

Small advantage: if this test fails, it will timeout and move onto other tests. 
without a timeout, the tests stop here and there's no further coverage.