Move launcher_.reset() from UITestBase constructor to SetUp().

Followup to http://codereview.chromium.org/5967003/

UITestBase now creates its ProxyLauncher instance in SetUp() rather than its constructor. LaunchState no longer contains references.

BUG=none
TEST=none

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

[Paweł Hajdan Jr., 2011-01-12 19:56:45]

Thank you, could you just fix the last issue?

http://codereview.chromium.org/5967003/diff/206001/chrome/test/automation/aut...

[dtu, 2011-01-12 20:04:28]

Let me know if any other changes are needed.

For this:
launcher_.reset(CreateProxyLauncher());

I mentioned it in my last comment but I was probably unclear.

This belongs in the constructor instead of SetUp() because:
1. It is part of the initial initialization of UITestBase, and there may be
things in UITestBase and UITest that rely on the ProxyLauncher being initialized
at all times.
2. It should be called only once for each instance of UITestBase. There is at
least one test (SessionRestoreUITest) that calls TearDown() and SetUp() more
than once, and these tests will fail if launcher_.reset() is called in SetUp()
instead.

Because ExternalTabUITest overrides the default constructor for UITestBase() and
does not call the constructor of its parent, it needs to have this line in it to
make sure ProxyLauncher is initialized.

[Paweł Hajdan Jr., 2011-01-12 21:11:28]

On 2011/01/12 20:04:28, Dave Tu wrote:
> Let me know if any other changes are needed.
> 
> For this:
> launcher_.reset(CreateProxyLauncher());
> 
> I mentioned it in my last comment but I was probably unclear.
> 
> This belongs in the constructor instead of SetUp() because:
> 1. It is part of the initial initialization of UITestBase, and there may be
> things in UITestBase and UITest that rely on the ProxyLauncher being
initialized
> at all times.

I just reviewed all occurrences of |launcher_| in ui_test.cc and all of them
should not be called before SetUp is called. Do you have some example we should
be worried about?

> 2. It should be called only once for each instance of UITestBase. There is at
> least one test (SessionRestoreUITest) that calls TearDown() and SetUp() more
> than once, and these tests will fail if launcher_.reset() is called in SetUp()
> instead.

Ouch, yeah, that's broken. How about checking |launcher_| for NULL in
UITestBase::SetUp, and only CreateProxyLauncher if it's NULL?

> Because ExternalTabUITest overrides the default constructor for UITestBase()
and
> does not call the constructor of its parent, it needs to have this line in it
to
> make sure ProxyLauncher is initialized.

I think ExternalTabUITest does call the parent constructor. There is other issue
here, calling a virtual method from ctor/dtor. See
http://www.artima.com/cppsource/nevercall.html for more info.

[dtu, 2011-01-12 21:23:39]

Done.

[Paweł Hajdan Jr., 2011-01-12 21:34:29]

LGTM with comments. Thank you for fixing all the review comments, I know it
wasn't easy.

Please also update the CL description to briefly describe the changes made here
and not just that it's a follow-up.

http://codereview.chromium.org/6111011/diff/6001/chrome/test/automation/autom...
File chrome/test/automation/automation_proxy_uitest.h (right):

http://codereview.chromium.org/6111011/diff/6001/chrome/test/automation/autom...
chrome/test/automation/automation_proxy_uitest.h:112: ExternalTabUITest() :
UITest(MessageLoop::TYPE_UI) {}
nit: Please keep the constructor's body in the .cc file, just remove the call to
CreateProxyLauncher. Decluttering headers helps with binary size and build time,
ask Elliot Glaysher (erg/eglaysher) for more info.

http://codereview.chromium.org/6111011/diff/6001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/6111011/diff/6001/chrome/test/ui/ui_test.cc#ne...
chrome/test/ui/ui_test.cc:117: if (!launcher_.get())
nit: Add a comment above why this NULL-check is needed.

For example: Some tests (SessionRestore...) call SetUp multiple times.
ProxyLauncher should only be created once.
// TODO(phajdan.jr): Fix tests that abuse SetUp and remove this check.