Refactor UITestBase/ProxyLauncher.

chrome/test/automation/proxy_launcher.h

Index: chrome/test/automation/proxy_launcher.h
diff --git a/chrome/test/automation/proxy_launcher.h b/chrome/test/automation/proxy_launcher.h
index 0f4d04dd8d93f0ff7832eb03a992c20109519a50..c2b390473b488a91317db4e44d333f8557eb09f1 100644
--- a/chrome/test/automation/proxy_launcher.h
+++ b/chrome/test/automation/proxy_launcher.h
@@ -8,6 +8,11 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/command_line.h"
+#include "base/process.h"
+#include "base/scoped_ptr.h"
+#include "base/scoped_temp_dir.h"
+#include "base/time.h"
 
 class AutomationProxy;
 class UITestBase;
@@ -16,7 +21,24 @@ class UITestBase;
 // or to use different channel IDs inside a class that derives from UITest.
 class ProxyLauncher {
  public:
-  ProxyLauncher() {}
+  // Profile theme type choices.
+  typedef enum {
+    DEFAULT_THEME = 0,
+    COMPLEX_THEME = 1,
+    NATIVE_THEME = 2,
+    CUSTOM_FRAME = 3,
+    CUSTOM_FRAME_NATIVE_THEME = 4,
+  } ProfileType;
+
+  // Different ways to quit the browser.
+  typedef enum {
+    WINDOW_CLOSE,
+    USER_QUIT,
+    SESSION_ENDING,
+  } ShutdownType;
+
+  ProxyLauncher();
+
   virtual ~ProxyLauncher() {}

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

nit: Move the dtor's impl to the .cc file.

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

Done.

 
   // Creates an automation proxy.
@@ -25,13 +47,247 @@ class ProxyLauncher {
 
   // Launches the browser if needed and establishes a connection
   // connection with it using the specified UITestBase.
-  virtual void InitializeConnection(UITestBase* ui_test_base) const = 0;
+  virtual void InitializeConnection(const CommandLine& arguments,

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

This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

[dtu, 2011/01/08 01:54:51]

Done.

+                                    bool include_testing_id,
+                                    bool clear_profile,
+                                    FilePath& template_user_data,
+                                    ProfileType profile_type,
+                                    FilePath browser_directory,
+                                    bool show_window,
+                                    bool wait_for_initial_loads,
+                                    base::TimeTicks* launch_time,
+                                    base::ProcessHandle* process,
+                                    base::ProcessId* process_id) = 0;
 
   // Returns the automation proxy's channel with any prefixes prepended,
   // for passing as a command line parameter over to the browser.
   virtual std::string PrefixedChannelID() const = 0;
 
+  virtual void SetUp(base::ProcessId process_id);

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

nit: SetUp in context of ProxyLauncher is very vague. Could you find a better
name, or (better) merge it with some other method?

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

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

+
+  // Launches the browser and IPC testing connection in server mode.
+  void LaunchBrowserAndServer(const CommandLine& arguments,

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

This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

[dtu, 2011/01/08 01:54:51]

Done.

+                              bool include_testing_id,
+                              bool clear_profile,
+                              FilePath& template_user_data,
+                              ProfileType profile_type,
+                              FilePath browser_directory,
+                              bool show_window,
+                              bool wait_for_initial_loads,
+                              base::TimeTicks* launch_time,
+                              base::ProcessHandle* process,
+                              base::ProcessId* process_id);
+
+  // Launches the IPC testing connection in client mode,
+  // which then attempts to connect to a browser.
+  void ConnectToRunningBrowser(bool wait_for_initial_loads);
+
+  // Only for pyauto.
+  void set_command_execution_timeout_ms(int timeout);
+
+  // Closes the browser and IPC testing server.
+  void CloseBrowserAndServer(ShutdownType shutdown_type,

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

This has too many parameters. Can we store quit_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

[dtu, 2011/01/08 01:54:51]

Done.

+                             base::TimeDelta* quit_time,
+                             base::ProcessHandle* process,
+                             base::ProcessId* process_id);
+
+  // Launches the browser with the given command line.
+  // TODO(phajdan.jr): Make LaunchBrowser private. Tests should use
+  // LaunchAnotherBrowserBlockUntilClosed.
+  void LaunchBrowser(const CommandLine& arguments,

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

This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

[dtu, 2011/01/08 01:54:51]

Done.

+                     bool include_testing_id,
+                     bool clear_profile,
+                     FilePath& template_user_data,
+                     ProfileType profile_type,
+                     FilePath browser_directory,
+                     bool show_window,
+                     base::TimeTicks* launch_time,
+                     base::ProcessHandle* process,
+                     base::ProcessId* process_id);
+
+#if !defined(OS_MACOSX)
+  // This function is deliberately not defined on the Mac because re-using an
+  // existing browser process when launching from the command line isn't a
+  // concept that we support on the Mac; AppleEvents are the Mac solution for
+  // the same need. Any test based on this function doesn't apply to the Mac.
+
+  // Launches an another browser process and waits for it to finish. Returns
+  // true on success.
+  bool LaunchAnotherBrowserBlockUntilClosed(const CommandLine& cmdline,
+                                            bool include_testing_id,
+                                            FilePath browser_directory,
+                                            bool show_window,
+                                            base::TimeTicks* launch_time);
+#endif
+
+  // Exits out browser instance.
+  void QuitBrowser(ShutdownType shutdown_type,
+                   base::TimeDelta* quit_time,
+                   base::ProcessHandle* process,
+                   base::ProcessId* process_id);
+
+  // Terminates the browser, simulates end of session.
+  void TerminateBrowser(base::TimeDelta* quit_time,
+                        base::ProcessHandle* process,
+                        base::ProcessId* process_id);
+
+  // Returns true when the browser process is running, independent if any
+  // renderer process exists or not. It will returns false if an user closed the
+  // window or if the browser process died by itself.
+  bool IsBrowserRunning(base::ProcessHandle process);
+
+  // Returns true when time_out_ms milliseconds have elapsed.
+  // Returns false if the browser process died while waiting.
+  bool CrashAwareSleep(base::ProcessHandle process, int time_out_ms);

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

nit: time_out_ms -> timeout_ms

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

Done. All instances in ProxyLauncher and UITest.

+
+  // Wait for the browser process to shut down on its own (i.e. as a result of
+  // some action that your test has taken).
+  bool WaitForBrowserProcessToQuit(base::ProcessHandle process);
+
+  AutomationProxy* automation() const;
+
+  // Return the user data directory being used by the browser instance in
+  // UITest::SetUp().
+  FilePath user_data_dir() const;
+
+  void set_ui_test_name(const std::string& name) {

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

nit: set_ui_test_name -> set_test_name (ProxyLauncher is more general)

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

Done.

+    ui_test_name_ = name;
+  }
+
+  // Sets homepage_. Should be called before launching browser to have
+  // any effect.
+  void set_homepage(const std::string& homepage) {
+    homepage_ = homepage;
+  }
+
+  // Get/Set a flag to run the renderer in process when running the
+  // tests.
+  static bool in_process_renderer() { return in_process_renderer_; }
+  static void set_in_process_renderer(bool value) {
+    in_process_renderer_ = value;
+  }
+
+  // Get/Set a flag to run the renderer outside the sandbox when running the
+  // tests
+  static bool no_sandbox() { return no_sandbox_; }
+  static void set_no_sandbox(bool value) {
+    no_sandbox_ = value;
+  }
+
+  // Get/Set a flag to run with DCHECKs enabled in release.
+  static bool enable_dcheck() { return enable_dcheck_; }
+  static void set_enable_dcheck(bool value) {
+    enable_dcheck_ = value;
+  }
+
+  // Get/Set a flag to dump the process memory without crashing on DCHECKs.
+  static bool silent_dump_on_dcheck() { return silent_dump_on_dcheck_; }
+  static void set_silent_dump_on_dcheck(bool value) {
+    silent_dump_on_dcheck_ = value;
+  }
+
+  // Get/Set a flag to disable breakpad handling.
+  static bool disable_breakpad() { return disable_breakpad_; }
+  static void set_disable_breakpad(bool value) {
+    disable_breakpad_ = value;
+  }
+
+  // Get/Set a flag to run the plugin processes inside the sandbox when running
+  // the tests
+  static bool safe_plugins() { return safe_plugins_; }
+  static void set_safe_plugins(bool value) {
+    safe_plugins_ = value;
+  }
+
+  static bool show_error_dialogs() { return show_error_dialogs_; }
+  static void set_show_error_dialogs(bool value) {
+    show_error_dialogs_ = value;
+  }
+
+  static bool full_memory_dump() { return full_memory_dump_; }
+  static void set_full_memory_dump(bool value) {
+    full_memory_dump_ = value;
+  }
+
+  static bool dump_histograms_on_exit() { return dump_histograms_on_exit_; }
+  static void set_dump_histograms_on_exit(bool value) {
+    dump_histograms_on_exit_ = value;
+  }
+
+  static const std::string& js_flags() { return js_flags_; }
+  static void set_js_flags(const std::string& value) {
+    js_flags_ = value;
+  }
+
+  static const std::string& log_level() { return log_level_; }
+  static void set_log_level(const std::string& value) {
+    log_level_ = value;
+  }
+
+ protected:
+  virtual bool ShouldFilterInet() {
+    return true;
+  }
+
  private:
+  void WaitForBrowserLaunch(bool wait_for_initial_loads);
+
+  // Prepare command line that will be used to launch the child browser process
+  // with an UI test.
+  void PrepareTestCommandline(CommandLine* command_line,
+                              bool include_testing_id);
+
+  bool LaunchBrowserHelper(const CommandLine& arguments,
+                           bool include_testing_id,
+                           FilePath browser_directory,
+                           bool wait,
+                           bool show_window,
+                           base::TimeTicks* launch_time,
+                           base::ProcessHandle* process);
+
+  // Check that no processes related to Chrome exist, displaying
+  // the given message if any do.
+  void AssertAppNotRunning(const std::wstring& error_message,
+                           base::ProcessId process_id);
+
+  // Wait a certain amount of time for all the app processes to exit,
+  // forcibly killing them if they haven't exited by then.
+  // It has the side-effect of killing every browser window opened in your
+  // session, even those unrelated in the test.
+  void CleanupAppProcesses(base::ProcessId process_id);
+
+  // Rewrite the preferences file to point to the proper image directory.
+  static void RewritePreferencesFile(const FilePath& user_data_dir);

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

nit: Can we move those private static functions to an anonymous namespace inside
the .cc file?

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

Done.

+
+  // We want to have a current history database when we start the browser so
+  // things like the NTP will have thumbnails.  This method updates the dates
+  // in the history to be more recent.
+  static void UpdateHistoryDates(const FilePath& user_data_dir);
+
+  scoped_ptr<AutomationProxy> automation_proxy_;
+
+  // We use a temporary directory for profile to avoid issues with being
+  // unable to delete some files because they're in use, etc.
+  scoped_ptr<ScopedTempDir> temp_profile_dir_;

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

nit: Can we stack-allocate this?

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

Done.

+
+  std::string ui_test_name_;

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

nit: ui_test_name_ -> test_name_ + add a comment.

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

Done.

+  std::string homepage_;                // Homepage used for testing.
+
+  static bool in_process_renderer_;     // true if we're in single process mode

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

nit: dot at the end of comment, start with a capital letter.

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

Done.

+  static bool no_sandbox_;

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

nit: comment those two.

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

Done.

+  static bool safe_plugins_;
+  static bool full_memory_dump_;        // If true, write full memory dump
+                                        // during crash.
+  static bool show_error_dialogs_;      // If true, a user is paying attention
+                                        // to the test, so show error dialogs.
+  static bool dump_histograms_on_exit_;  // Include histograms in log on exit.

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

nit: This comment is misaligned. It looks weird, so I'd suggest just putting the
comments above described variables instead of to the right.

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

Done.

+  static bool enable_dcheck_;           // Enable dchecks in release mode.
+  static bool silent_dump_on_dcheck_;   // Dump process memory on dcheck without
+                                        // crashing.
+  static bool disable_breakpad_;        // Disable breakpad on the browser.
+  static std::string js_flags_;         // Flags passed to the JS engine.
+  static std::string log_level_;        // Logging level.
+
   DISALLOW_COPY_AND_ASSIGN(ProxyLauncher);
 };
 
@@ -46,7 +302,17 @@ class NamedProxyLauncher : public ProxyLauncher {
   NamedProxyLauncher(bool launch_browser, bool disconnect_on_failure);
 
   virtual AutomationProxy* CreateAutomationProxy(int execution_timeout);
-  virtual void InitializeConnection(UITestBase* ui_test_base) const;
+  virtual void InitializeConnection(const CommandLine& arguments,

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

This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

[dtu, 2011/01/08 01:54:51]

Done.

+                                    bool include_testing_id,
+                                    bool clear_profile,
+                                    FilePath& template_user_data,
+                                    ProfileType profile_type,
+                                    FilePath browser_directory,
+                                    bool show_window,
+                                    bool wait_for_initial_loads,
+                                    base::TimeTicks* launch_time,
+                                    base::ProcessHandle* process,
+                                    base::ProcessId* process_id);
   virtual std::string PrefixedChannelID() const;
 
  protected:
@@ -63,7 +329,17 @@ class AnonymousProxyLauncher : public ProxyLauncher {
  public:
   explicit AnonymousProxyLauncher(bool disconnect_on_failure);
   virtual AutomationProxy* CreateAutomationProxy(int execution_timeout);
-  virtual void InitializeConnection(UITestBase* ui_test_base) const;
+  virtual void InitializeConnection(const CommandLine& arguments,

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

This has definitely too many parameters. Can we store launch_time inside
ProxyLauncher, and expose its value? Similarly for |process| and |process_id|.

[dtu, 2011/01/08 01:54:51]

Done.

+                                    bool include_testing_id,
+                                    bool clear_profile,
+                                    FilePath& template_user_data,
+                                    ProfileType profile_type,
+                                    FilePath browser_directory,
+                                    bool show_window,
+                                    bool wait_for_initial_loads,
+                                    base::TimeTicks* launch_time,
+                                    base::ProcessHandle* process,
+                                    base::ProcessId* process_id);
   virtual std::string PrefixedChannelID() const;
 
  protected:
@@ -75,4 +351,3 @@ class AnonymousProxyLauncher : public ProxyLauncher {
 };
 
 #endif  // CHROME_TEST_AUTOMATION_PROXY_LAUNCHER_H_
-