Refactor UITestBase/ProxyLauncher.

Moves everything related to launching and terminating the browser from UITestBase into ProxyLauncher.

The primary changes are in ui_test.* and proxy_launcher.*. The changes in the remaining files are mostly just changing namespaces from UITestBase:: to ProxyLauncher::.

BUG=None.
TEST=All tests should pass. No functionality change.

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

[Paweł Hajdan Jr., 2011-01-05 21:47:59]

Drive-by with automation comments. Thank you for the follow-up.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy_uitest.h (right):

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy_uitest.h:111:
launcher_.reset(CreateProxyLauncher());
Why do we have to explicitly reset launcher_ here? It should be sufficient to
override CreateProxyLauncher.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:42: virtual ~ProxyLauncher() {}
nit: Move the dtor's impl to the .cc file.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:50: virtual void
InitializeConnection(const CommandLine& arguments,
This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:66: virtual void SetUp(base::ProcessId
process_id);
nit: SetUp in context of ProxyLauncher is very vague. Could you find a better
name, or (better) merge it with some other method?

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:69: void LaunchBrowserAndServer(const
CommandLine& arguments,
This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:89: void
CloseBrowserAndServer(ShutdownType shutdown_type,
This has too many parameters. Can we store quit_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:97: void LaunchBrowser(const
CommandLine& arguments,
This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:141: bool
CrashAwareSleep(base::ProcessHandle process, int time_out_ms);
nit: time_out_ms -> timeout_ms

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:153: void set_ui_test_name(const
std::string& name) {
nit: set_ui_test_name -> set_test_name (ProxyLauncher is more general)

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:260: static void
RewritePreferencesFile(const FilePath& user_data_dir);
nit: Can we move those private static functions to an anonymous namespace inside
the .cc file?

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:271: scoped_ptr<ScopedTempDir>
temp_profile_dir_;
nit: Can we stack-allocate this?

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:273: std::string ui_test_name_;
nit: ui_test_name_ -> test_name_ + add a comment.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:276: static bool in_process_renderer_;  
  // true if we're in single process mode
nit: dot at the end of comment, start with a capital letter.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:277: static bool no_sandbox_;
nit: comment those two.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:283: static bool
dump_histograms_on_exit_;  // Include histograms in log on exit.
nit: This comment is misaligned. It looks weird, so I'd suggest just putting the
comments above described variables instead of to the right.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:305: virtual void
InitializeConnection(const CommandLine& arguments,
This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:332: virtual void
InitializeConnection(const CommandLine& arguments,
This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

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

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:97: launcher_.reset(CreateProxyLauncher());
I wonder if we should do it in SetUp... that might make it cleaner for the
callers.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:131:
launcher_->InitializeConnection(launch_arguments_, include_testing_id_,
Another idea is to push data to launcher_ in places where launch_arguments_,
include_testing_id_, and all those variables are set or modified.

As the last resort, can we have a struct to group them together, instead of
having so many parameters?

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:188: if (!launch_arguments_.HasSwitch(switch_string))
Do we need this check? It seems it would be simpler to just AppendSwitch twice
above.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:344: ProxyLauncher::ProfileType profile_type_;  // Are
we using a profile with a
nit: Could you move the comments above the member variables instead of to the
right?

[dtu, 2011-01-06 01:41:13]

Nits fixed, thanks! Will wait for more feedback to see how far we want to go on
moving member variables from UITestBase to ProxyLauncher to reduce the lengths
of the parameter lists.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy_uitest.h (right):

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy_uitest.h:111:
launcher_.reset(CreateProxyLauncher());
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> Why do we have to explicitly reset launcher_ here? It should be sufficient to
> override CreateProxyLauncher.

Because the ProxyLauncher is now created in the constructor of UITestBase, and
class overrides that constructor. See comment in ui_test.cc.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:42: virtual ~ProxyLauncher() {}
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: Move the dtor's impl to the .cc file.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:66: virtual void SetUp(base::ProcessId
process_id);
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: SetUp in context of ProxyLauncher is very vague. Could you find a better
> name, or (better) merge it with some other method?

Done. Moved back into UITestBase::SetUp().

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:141: bool
CrashAwareSleep(base::ProcessHandle process, int time_out_ms);
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: time_out_ms -> timeout_ms

Done. All instances in ProxyLauncher and UITest.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:153: void set_ui_test_name(const
std::string& name) {
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: set_ui_test_name -> set_test_name (ProxyLauncher is more general)

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:260: static void
RewritePreferencesFile(const FilePath& user_data_dir);
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: Can we move those private static functions to an anonymous namespace
inside
> the .cc file?

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:271: scoped_ptr<ScopedTempDir>
temp_profile_dir_;
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: Can we stack-allocate this?

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:273: std::string ui_test_name_;
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: ui_test_name_ -> test_name_ + add a comment.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:276: static bool in_process_renderer_;  
  // true if we're in single process mode
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: dot at the end of comment, start with a capital letter.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:277: static bool no_sandbox_;
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: comment those two.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:283: static bool
dump_histograms_on_exit_;  // Include histograms in log on exit.
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: This comment is misaligned. It looks weird, so I'd suggest just putting
the
> comments above described variables instead of to the right.

Done.

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

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:97: launcher_.reset(CreateProxyLauncher());
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> I wonder if we should do it in SetUp... that might make it cleaner for the
> callers.

It was originally in SetUp(). I moved it out of SetUp and into the constructor
because homepage_ has been moved from UITestBase to ProxyLauncher. As a result,
set_homepage() must be called after the ProxyLauncher is created but before
SetUp() is called. If ProxyLauncher is created in SetUp(), that's not possible.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:188: if (!launch_arguments_.HasSwitch(switch_string))
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> Do we need this check? It seems it would be simpler to just AppendSwitch twice
> above.

Yes, I believe this check is optional. Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:344: ProxyLauncher::ProfileType profile_type_;  // Are
we using a profile with a
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> nit: Could you move the comments above the member variables instead of to the
> right?

Done.

[Paweł Hajdan Jr., 2011-01-06 20:37:14]

Okay, so we still need to find a way to reduce the number of parameters in some
methods. A few more comments below (sorry, I missed a few tiny nits before).

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

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:97: launcher_.reset(CreateProxyLauncher());
On 2011/01/06 01:41:13, Dave Tu wrote:
> On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> > I wonder if we should do it in SetUp... that might make it cleaner for the
> > callers.
> 
> It was originally in SetUp(). I moved it out of SetUp and into the constructor
> because homepage_ has been moved from UITestBase to ProxyLauncher. As a
result,
> set_homepage() must be called after the ProxyLauncher is created but before
> SetUp() is called. If ProxyLauncher is created in SetUp(), that's not
possible.

I see. How about removing knowledge about homepage from ProxyLauncher, and
leaving it in UITestBase? I mean just don't proxy the set_homepage call to
launcher_, but instead store the value in UITestBase and add the necessary
switch to UITestBase::SetLaunchSwitches (currently the homepage switch is added
in ProxyLauncher::PrepareTestCommandline,
command_line->AppendSwitchASCII(switches::kHomePage, homepage_); it should be
possible to move that code to ui_test.cc).

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:36: namespace {
nit: Add empty line below.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:90: }
nit: Add empty line below.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:108: : temp_profile_dir_(),
nit: I think there's no need to put an initializer here for temp_profile_dir_.
Similarly, homepage_ initializer should be possible to move to the previous
line.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:515: : ProxyLauncher(),
nit: I think there's no need to call ProxyLauncher() explicitly here.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:564: : ProxyLauncher(),
nit: I think there's no need to call ProxyLauncher() explicitly here.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:25: typedef enum {
nit: Is the typedef needed? Just enum should be sufficient. Sorry I missed it
before.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:153: // UITest::SetUp().
nit: Don't mention UITest::SetUp here. I think it can just be removed.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:264: std::string test_name_;
nit: Add empty line below.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:269: static bool in_process_renderer_;
nit: Similarly here, separate the variables by empty lines.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/5967003/diff/25004/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:315: // Path to the browser executable.
nit: Separate the variables by blank lines.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:343: // Should we supply the testing channel id on the
command line? Default is true.
nit: 80 cols.

[Nirnimesh, 2011-01-06 22:56:44]

I have only minor nits.
LGTM

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:49: // connection with it using the
specified UITestBase.
Fix this comment. Don't need UITestBase here

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:121: // Exits out browser instance.
s/out/our/

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:239: // with an UI test.
s/an/a/

[dtu, 2011-01-07 01:46:16]

Nits fixed. Also moved process_ and process_id_ from UITestBase to
ProxyLauncher.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:36: namespace {
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: Add empty line below.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:90: }
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: Add empty line below.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:108: : temp_profile_dir_(),
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: I think there's no need to put an initializer here for temp_profile_dir_.
> Similarly, homepage_ initializer should be possible to move to the previous
> line.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:515: : ProxyLauncher(),
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: I think there's no need to call ProxyLauncher() explicitly here.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:564: : ProxyLauncher(),
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: I think there's no need to call ProxyLauncher() explicitly here.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:25: typedef enum {
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: Is the typedef needed? Just enum should be sufficient. Sorry I missed it
> before.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:49: // connection with it using the
specified UITestBase.
On 2011/01/06 22:56:45, Nirnimesh wrote:
> Fix this comment. Don't need UITestBase here

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:121: // Exits out browser instance.
On 2011/01/06 22:56:45, Nirnimesh wrote:
> s/out/our/

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:153: // UITest::SetUp().
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: Don't mention UITest::SetUp here. I think it can just be removed.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:239: // with an UI test.
On 2011/01/06 22:56:45, Nirnimesh wrote:
> s/an/a/

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:264: std::string test_name_;
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: Add empty line below.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:269: static bool in_process_renderer_;
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: Similarly here, separate the variables by empty lines.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/5967003/diff/25004/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:315: // Path to the browser executable.
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: Separate the variables by blank lines.

Done.

http://codereview.chromium.org/5967003/diff/25004/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:343: // Should we supply the testing channel id on the
command line? Default is true.
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> nit: 80 cols.

Done.

[John Grabowski, 2011-01-07 03:36:11]

3 issues.  1, need more comments.  2, consider "params" struct (see below).  if
you strongly disagree, that's fine. 3. consider bits instead of vars for flags
to make code simpler.

LGTM

http://codereview.chromium.org/5967003/diff/136001/chrome/browser/session_his...
File chrome/browser/session_history_uitest.cc (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/browser/session_his...
chrome/browser/session_history_uitest.cc:159: GURL home(chrome::kAboutBlankURL);
You added a set_homepage() to replace "homepage_ = ..."
So shouldn't you write a simple getter, and make this line be

    GURL home(homepage());

?

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/aut...
File chrome/test/automation/automation_proxy_uitest.h (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/aut...
chrome/test/automation/automation_proxy_uitest.h:110: ExternalTabUITest() :
UITest(MessageLoop::TYPE_UI) {
In a header, empty methods are usually {}, not {\n}
I don't think that's in the style guide as such but is a clear convention.
So remove this change.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.cc:31: static const char kUITestType[] =
"ui";
prefer anonymous namespace to static vars

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.cc:38: void RewritePreferencesFile(const
FilePath& user_data_dir) {
Comment for any function, especially ones which sound dangerous.
Comment on WHY I'd want to call this.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.cc:70: void UpdateHistoryDates(const
FilePath& user_data_dir) {
Comment on when I'd want to use this

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:21: class ProxyLauncher {
All classes need a comment.  Your comment says when to subclass but does not
give a general description.  E.g.
"Base class for doing blah.  Contains functionality for BLAH."

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:48: virtual void
InitializeConnection(const CommandLine& arguments,
That's a lotta args.
Consider adding a POD struct for "params" whose ctor sets some default values. 
That way if you add "just one more" parem you don't need to change 15 sites.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:99: // concept that we support on the
Mac; AppleEvents are the Mac solution for
Wording is a bit funny here.  AppleEvents don't let you launch another browser
process as such.  Just say "the concept doesn't apply to Mac; you can't have N
browser processes."

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:102: // Launches an another browser
process and waits for it to finish. Returns
"an another" --> "another"

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:133: bool WaitForBrowserProcessToQuit();
Suggest adding timeout on any WaitFor-type method, like this one.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:152: // Get/Set a flag to run the
renderer in process when running the tests.
in-process

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:176: // Get/Set a flag to disable
breakpad handling.
Seems like a number of these could be bits set in a "flags" type field, ored
together.  Might make it easier to add "just one more" flag, and reduce code
size.

[dtu, 2011-01-08 01:54:50]

Nits fixed, comments added.

POD struct added. This reduces the lengths of the parameter lists.

I'm leaving CreateProxyLauncher in the constructor because there are other
dependencies on it. E.g., SessionRestoreUITest does a TearDown/SetUp in several
of its tests and it would cause different behavior if the ProxyLauncher is reset
each time. The "correct" behavior is to only initialize ProxyLauncher once.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:50: virtual void
InitializeConnection(const CommandLine& arguments,
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> This has definitely too many parameters. Can we store launch_time inside
> ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:69: void LaunchBrowserAndServer(const
CommandLine& arguments,
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> This has definitely too many parameters. Can we store launch_time inside
> ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:89: void
CloseBrowserAndServer(ShutdownType shutdown_type,
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> This has too many parameters. Can we store quit_time inside
> ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:97: void LaunchBrowser(const
CommandLine& arguments,
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> This has definitely too many parameters. Can we store launch_time inside
> ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:305: virtual void
InitializeConnection(const CommandLine& arguments,
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> This has definitely too many parameters. Can we store launch_time inside
> ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

Done.

http://codereview.chromium.org/5967003/diff/70001/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.h:332: virtual void
InitializeConnection(const CommandLine& arguments,
On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> This has definitely too many parameters. Can we store launch_time inside
> ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

Done.

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

http://codereview.chromium.org/5967003/diff/70001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:97: launcher_.reset(CreateProxyLauncher());
On 2011/01/06 20:37:14, Paweł Hajdan Jr. wrote:
> On 2011/01/06 01:41:13, Dave Tu wrote:
> > On 2011/01/05 21:47:59, Paweł Hajdan Jr. wrote:
> > > I wonder if we should do it in SetUp... that might make it cleaner for the
> > > callers.
> > 
> > It was originally in SetUp(). I moved it out of SetUp and into the
constructor
> > because homepage_ has been moved from UITestBase to ProxyLauncher. As a
> result,
> > set_homepage() must be called after the ProxyLauncher is created but before
> > SetUp() is called. If ProxyLauncher is created in SetUp(), that's not
> possible.
> 
> I see. How about removing knowledge about homepage from ProxyLauncher, and
> leaving it in UITestBase? I mean just don't proxy the set_homepage call to
> launcher_, but instead store the value in UITestBase and add the necessary
> switch to UITestBase::SetLaunchSwitches (currently the homepage switch is
added
> in ProxyLauncher::PrepareTestCommandline,
> command_line->AppendSwitchASCII(switches::kHomePage, homepage_); it should be
> possible to move that code to ui_test.cc).

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/browser/session_his...
File chrome/browser/session_history_uitest.cc (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/browser/session_his...
chrome/browser/session_history_uitest.cc:159: GURL home(chrome::kAboutBlankURL);
On 2011/01/07 03:36:11, John Grabowski wrote:
> You added a set_homepage() to replace "homepage_ = ..."
> So shouldn't you write a simple getter, and make this line be
> 
>     GURL home(homepage());
> 
> ?

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/aut...
File chrome/test/automation/automation_proxy_uitest.h (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/aut...
chrome/test/automation/automation_proxy_uitest.h:110: ExternalTabUITest() :
UITest(MessageLoop::TYPE_UI) {
On 2011/01/07 03:36:11, John Grabowski wrote:
> In a header, empty methods are usually {}, not {\n}
> I don't think that's in the style guide as such but is a clear convention.
> So remove this change.

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.cc:31: static const char kUITestType[] =
"ui";
On 2011/01/07 03:36:11, John Grabowski wrote:
> prefer anonymous namespace to static vars

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.cc:38: void RewritePreferencesFile(const
FilePath& user_data_dir) {
On 2011/01/07 03:36:11, John Grabowski wrote:
> Comment for any function, especially ones which sound dangerous.
> Comment on WHY I'd want to call this.

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.cc:70: void UpdateHistoryDates(const
FilePath& user_data_dir) {
On 2011/01/07 03:36:11, John Grabowski wrote:
> Comment on when I'd want to use this

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:21: class ProxyLauncher {
On 2011/01/07 03:36:11, John Grabowski wrote:
> All classes need a comment.  Your comment says when to subclass but does not
> give a general description.  E.g.
> "Base class for doing blah.  Contains functionality for BLAH."

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:48: virtual void
InitializeConnection(const CommandLine& arguments,
On 2011/01/07 03:36:11, John Grabowski wrote:
> That's a lotta args.
> Consider adding a POD struct for "params" whose ctor sets some default values.

> That way if you add "just one more" parem you don't need to change 15 sites.

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:99: // concept that we support on the
Mac; AppleEvents are the Mac solution for
On 2011/01/07 03:36:11, John Grabowski wrote:
> Wording is a bit funny here.  AppleEvents don't let you launch another browser
> process as such.  Just say "the concept doesn't apply to Mac; you can't have N
> browser processes."

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:102: // Launches an another browser
process and waits for it to finish. Returns
On 2011/01/07 03:36:11, John Grabowski wrote:
> "an another" --> "another"

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:133: bool WaitForBrowserProcessToQuit();
On 2011/01/07 03:36:11, John Grabowski wrote:
> Suggest adding timeout on any WaitFor-type method, like this one.

Done.

http://codereview.chromium.org/5967003/diff/136001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:152: // Get/Set a flag to run the
renderer in process when running the tests.
On 2011/01/07 03:36:11, John Grabowski wrote:
> in-process

Done.

[Paweł Hajdan Jr., 2011-01-08 06:45:25]

Some more comments. Could you please do a follow-up-to-the-followup?

Let me also repeat my earlier comment: please do not commit a change if there
are unresolved comments by any reviewer, even if you have "LGTM" from someone
else.

http://codereview.chromium.org/5967003/diff/206001/chrome/test/automation/aut...
File chrome/test/automation/automation_proxy_uitest.cc (right):

http://codereview.chromium.org/5967003/diff/206001/chrome/test/automation/aut...
chrome/test/automation/automation_proxy_uitest.cc:890:
launcher_.reset(CreateProxyLauncher());
Again, *why* do we need this?

http://codereview.chromium.org/5967003/diff/206001/chrome/test/automation/pro...
File chrome/test/automation/proxy_launcher.h (right):

http://codereview.chromium.org/5967003/diff/206001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:45: typedef struct {
nit: *Again* why is this typedef needed here?

http://codereview.chromium.org/5967003/diff/206001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:51: FilePath& template_user_data;
Why is this a reference? It seems quite dangerous.

http://codereview.chromium.org/5967003/diff/206001/chrome/test/automation/pro...
chrome/test/automation/proxy_launcher.h:60: CommandLine& arguments;
Why is this a reference? It seems quite dangerous.