Adding simple Chrome install testing using multi-install and cleaning up a bit.

chrome/test/mini_installer_test/chrome_mini_installer.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/test/mini_installer_test/chrome_mini_installer.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/process.h"
 #include "base/process_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/threading/platform_thread.h"
 #include "base/win/registry.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/google_update_constants.h"
+#include "chrome/installer/util/helper.h"
+#include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/installation_validation_helper.h"
+#include "chrome/test/base/chrome_process_util.h"
 #include "chrome/test/mini_installer_test/mini_installer_test_constants.h"
 #include "chrome/test/mini_installer_test/mini_installer_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::win::RegKey;
+using installer::InstallationValidator;
 
 namespace {
 
 struct FilePathInfo {
   file_util::FileEnumerator::FindInfo info;
   FilePath path;
 };
 
 bool CompareDate(const FilePathInfo& a,
                  const FilePathInfo& b) {
 #if defined(OS_POSIX)
   return a.info.stat.st_mtime > b.info.stat.st_mtime;
 #elif defined(OS_WIN)
   if (a.info.ftLastWriteTime.dwHighDateTime ==
       b.info.ftLastWriteTime.dwHighDateTime) {
     return a.info.ftLastWriteTime.dwLowDateTime >
            b.info.ftLastWriteTime.dwLowDateTime;
   } else {
     return a.info.ftLastWriteTime.dwHighDateTime >
            b.info.ftLastWriteTime.dwHighDateTime;
   }
 #endif
 }
 
+class ChromeProcessFilter : public base::ProcessFilter {
+ public:
+  explicit ChromeProcessFilter(base::ProcessId pid)
+    :process_id(pid) {}

[kkania, 2011/09/26 20:00:53]

4 space indent here, and 1 space between : and process_id; you have it right
below

[Huyen, 2011/09/27 23:00:24]

Done.

+
+  virtual bool Includes(const base::ProcessEntry& entry) const {
+      return process_id == entry.pid();

[kkania, 2011/09/26 20:00:53]

two space indent here

[Huyen, 2011/09/27 23:00:24]

Done.

+  }
+
+ private:
+  const base::ProcessId process_id;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeProcessFilter);
+};
+
+// Copied from chrome/test/base/chrome_process_util.cc
+class ChildProcessFilter : public base::ProcessFilter {
+ public:
+  explicit ChildProcessFilter(base::ProcessId parent_pid)
+      : parent_pids_(&parent_pid, (&parent_pid) + 1) {}
+
+  explicit ChildProcessFilter(std::vector<base::ProcessId> parent_pids)
+      : parent_pids_(parent_pids.begin(), parent_pids.end()) {}
+
+  virtual bool Includes(const base::ProcessEntry& entry) const {
+    return parent_pids_.find(entry.parent_pid()) != parent_pids_.end();
+  }
+
+ private:
+  const std::set<base::ProcessId> parent_pids_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChildProcessFilter);
+};
+
 // Get list of file |type| matching |pattern| in |root|.
 // The list is sorted in last modified date order.
 // Return true if files/directories are found.
 bool FindMatchingFiles(const FilePath& root,
     const FilePath::StringType& pattern,
     file_util::FileEnumerator::FileType type,
     std::vector<FilePath>* paths) {
   file_util::FileEnumerator files(root, false, type, pattern);
   std::vector<FilePathInfo> matches;
   for (FilePath current = files.Next(); !current.empty();
@@ skipping 22 matching lines @@
   std::vector<FilePath> paths;
   if (FindMatchingFiles(root, pattern, type, &paths)) {
     *path = paths[0];
     return true;
   }
   return false;
 }
 
 }  // namespace
 
-ChromeMiniInstaller::ChromeMiniInstaller(const std::wstring& install_type,
+ChromeMiniInstaller::ChromeMiniInstaller(bool system_install,
                                          bool is_chrome_frame)
     : is_chrome_frame_(is_chrome_frame),
-      install_type_(install_type) {}
+      system_install_(system_install) {}
 
 void ChromeMiniInstaller::SetBuildUnderTest(
     const std::wstring& build) {
   if (!LocateInstallers(build)) {
     LOG(WARNING) << "Could not find one or more installers.";
   }
 }
 
 // Installs Chrome.
 void ChromeMiniInstaller::Install() {
-  FilePath installer_path = MiniInstallerTestUtil::GetFilePath(
-      mini_installer_constants::kChromeMiniInstallerExecutable);
-  InstallMiniInstaller(false, installer_path);
+  InstallMiniInstaller(false, mini_installer_);

[kkania, 2011/09/26 20:00:53]

not sure why this passes mini_installer_ if its accessible by the called func.

[Huyen, 2011/09/27 23:00:24]

Because sometime we use diff installer or other type of installer. We can pass
an installer type instead of the actual installer but that doesn't make this
better.

 }
 
 // This method will get the previous latest full installer from
 // nightly location, install it and over install with specified install_type.
 void ChromeMiniInstaller::OverInstallOnFullInstaller(
     const std::wstring& install_type, bool should_start_ie) {
   ASSERT_TRUE(!full_installer_.empty() && !diff_installer_.empty() &&
               !previous_installer_.empty());
 
   if (should_start_ie)
@@ skipping 37 matching lines @@
 // This method will get the latest full installer from nightly location
 // and installs it.
 void ChromeMiniInstaller::InstallFullInstaller(bool over_install) {
   if (!full_installer_.empty())
     InstallMiniInstaller(over_install, full_installer_);
 }
 
 // Installs the Chrome mini-installer, checks the registry and shortcuts.
 void ChromeMiniInstaller::InstallMiniInstaller(bool over_install,
                                                const FilePath& path) {
-  LOG(INFO) << "Chrome will be installed at "
-            << install_type_ << "level.";
-  LOG(INFO) << "Will proceed with the test only if this path exists: "
-            << path.value();
+  LOG(INFO) << "Install level is: "
+      << system_install_ << " (1 means system, otherwise user)";

[kkania, 2011/09/26 20:00:53]

use the ternary operator here to print out 'system' or 'user'

[Huyen, 2011/09/27 23:00:24]

Done.

+  LaunchInstaller(path, CommandLine::CommandLine(CommandLine::NO_PROGRAM));
 
-  ASSERT_TRUE(file_util::PathExists(path)) << path.value()
-                                           << " does not exist.";
-  LaunchInstaller(path, path.BaseName().value().c_str());
+  std::wstring version;
+  ASSERT_TRUE(GetChromeVersionFromRegistry(&version))
+      << "Install failed: unable to get version.";
+  VerifyInstall(over_install);
+}
+
+void ChromeMiniInstaller::InstallUsingMultiInstall() {
+  ASSERT_TRUE(file_util::PathExists(full_installer_));
+  CommandLine args(CommandLine::NO_PROGRAM);
+  args.AppendArg("--multi-install");

[kkania, 2011/09/26 20:00:53]

use
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util...
instead of duplicating the constant value; maybe same for below

[Huyen, 2011/09/27 23:00:24]

Done.

+  args.AppendArg("--chrome");
+  LaunchInstaller(mini_installer_, args);
+  VerifyMultiInstall();
+}
+
+void ChromeMiniInstaller::VerifyMultiInstall() {
+  InstallationValidator::InstallationType type =
+      installer::ExpectValidInstallation(system_install_);
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
-  ASSERT_TRUE(CheckRegistryKey(dist->GetVersionKey())) << dist->GetVersionKey()
-                                                       << " does not exist.";
-  VerifyInstall(over_install);
+  ASSERT_TRUE(InstallUtil::IsMultiInstall(dist, system_install_));
+  if (is_chrome_frame_) {
+    EXPECT_NE(0,

[kkania, 2011/09/26 20:00:53]

how about just EXPECT_TRUE(type & ....); same below

[Huyen, 2011/09/27 23:00:24]

Done.

+        type & InstallationValidator::ProductBits::CHROME_FRAME_MULTI);
+    VerifyChromeFrameInstall();
+  } else {
+      EXPECT_NE(0, type & InstallationValidator::ProductBits::CHROME_MULTI);
+  }
+  FindChromeShortcut();
+  LaunchChrome(false);
 }
 
 // This method tests the standalone installer by verifying the steps listed at:
 // https://sites.google.com/a/google.com/chrome-pmo/
 // standalone-installers/testing-standalone-installers
 // This method applies appropriate tags to standalone installer and deletes
 // old installer before running the new tagged installer. It also verifies
 // that the installed version is correct.
 void ChromeMiniInstaller::InstallStandaloneInstaller() {
   file_util::Delete(mini_installer_constants::kStandaloneInstaller, true);
@@ skipping 23 matching lines @@
       mini_installer_constants::kChromeApplyTagParameters));
   LOG(INFO) << "Tagging command: " << command.GetCommandLineString();
   return command;
 }
 
 // Installs chromesetup.exe, waits for the install to finish and then
 // checks the registry and shortcuts.
 void ChromeMiniInstaller::InstallMetaInstaller() {
   // Install Google Chrome through meta installer.
   LaunchInstaller(FilePath(mini_installer_constants::kChromeMetaInstallerExe),
-                  mini_installer_constants::kChromeSetupExecutable);
+      CommandLine::CommandLine(CommandLine::NO_PROGRAM));
   ASSERT_TRUE(MiniInstallerTestUtil::VerifyProcessClose(
       mini_installer_constants::kChromeMetaInstallerExecutable));
+
   std::wstring chrome_google_update_state_key(
       google_update::kRegPathClients);
   chrome_google_update_state_key.append(L"\\");
 
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   chrome_google_update_state_key.append(dist->GetAppGuid());
 
   ASSERT_TRUE(CheckRegistryKey(chrome_google_update_state_key));
   ASSERT_TRUE(CheckRegistryKey(dist->GetVersionKey()));
   FindChromeShortcut();
-  LaunchAndCloseChrome(false);
+  LaunchChrome(true);
 }
 
 // If the build type is Google Chrome, then it first installs meta installer
 // and then over installs with mini_installer. It also verifies if Chrome can
 // be launched successfully after overinstall.
 void ChromeMiniInstaller::OverInstall() {
   InstallMetaInstaller();
   std::wstring reg_key_value_returned;
   // gets the registry key value before overinstall.
   GetChromeVersionFromRegistry(&reg_key_value_returned);
@@ skipping 21 matching lines @@
   MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
   if (repair_type == ChromeMiniInstaller::VERSION_FOLDER) {
     DeleteFolder(L"version_folder");
     printf("Deleted folder. Now trying to launch chrome\n");
   } else if (repair_type == ChromeMiniInstaller::REGISTRY) {
     DeletePvRegistryKey();
     printf("Deleted registry. Now trying to launch chrome\n");
   }
   FilePath current_path;
   ASSERT_TRUE(MiniInstallerTestUtil::ChangeCurrentDirectory(&current_path));
-  VerifyChromeLaunch(false);
-  printf("\nInstalling Chrome again to see if it can be repaired\n\n");
+  LaunchChrome(false);
+  LOG(INFO) << "Installing Chrome again to see if it can be repaired.";
   InstallFullInstaller(true);
-  printf("Chrome repair successful.\n");
+  LOG(INFO) << "Chrome repair successful.";
   // Set the current directory back to original path.
   file_util::SetCurrentDirectory(current_path);
 }
 
 // This method first checks if Chrome is running.
 // If yes, will close all the processes.
 // Then will find and spawn uninstall path.
 // Handles uninstall confirm dialog.
 // Waits until setup.exe ends.
 // Checks if registry key exist even after uninstall.
 // Deletes App dir.
 // Closes feedback form.
 void ChromeMiniInstaller::UnInstall() {
-  std::wstring product_name;
-  if (is_chrome_frame_)
-    product_name = mini_installer_constants::kChromeFrameProductName;
-  else
-    product_name = mini_installer_constants::kChromeProductName;
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
-  if (!CheckRegistryKey(dist->GetVersionKey())) {
-    printf("%ls is not installed.\n", product_name.c_str());
-    return;
-  }
-  if (is_chrome_frame_) {
-    MiniInstallerTestUtil::CloseProcesses(
-        mini_installer_constants::kIEProcessName);
+  std::wstring version;
+  std::string result;
+  if (GetChromeVersionFromRegistry(&version)) {

[kkania, 2011/09/26 20:00:53]

This function fails silently in a lot of places. In some cases we want to be
sure that the uninstall was successful. To do that, I think we should make this
func expect chrome is installed and should fail if uninstall is unsuccessful.
Make another that removes chrome if its installed.

[Huyen, 2011/09/27 23:00:24]

This seems to be used by the setup and teardown steps only. In these cases if
Chrome is not install then we just continue, there doesn't seem to be a need to
fail.

+    // Close running products.
+    if (is_chrome_frame_) {
+      MiniInstallerTestUtil::CloseProcesses(
+          mini_installer_constants::kIEProcessName);
+    } else {
+      MiniInstallerTestUtil::CloseProcesses(installer::kNaClExe);
+    }
+    MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
+    // Get uninstall command.
+    CommandLine cmd = InstallUtil::GetChromeUninstallCmd(
+        system_install_, dist->GetType());
+
+    if (cmd.GetCommandLineString().empty()) {
+      LOG(ERROR) << "Unable to get uninstall command.";
+      CleanChromeInstall();
+    }
+    LOG(INFO) << "Uninstall command: " << cmd.GetCommandLineString();
+    ASSERT_TRUE(file_util::PathExists(cmd.GetProgram()));
+
+    cmd.AppendArg("--uninstall");

[kkania, 2011/09/26 20:00:53]

same here

[Huyen, 2011/09/27 23:00:24]

Done.

+    cmd.AppendArg("--force-uninstall");
+    if (is_chrome_frame_)
+      cmd.AppendArg("--chrome-frame");
+    if (system_install_)
+      cmd.AppendArg("--system-level");
+
+    base::ProcessHandle setup_handle;
+    if (!base::LaunchProcess(cmd, base::LaunchOptions(), &setup_handle)) {
+      LOG(ERROR) << "Failed to launch uninstall command.";
+    }
+
+    if (is_chrome_frame_)
+      ASSERT_TRUE(CloseUninstallWindow());
+    ASSERT_TRUE(MiniInstallerTestUtil::VerifyProcessHandleClosed(setup_handle));
+    ASSERT_FALSE(CheckRegistryKeyOnUninstall(dist->GetVersionKey()));
+
+    DeleteUserDataFolder();
+    // Close IE survey window that gets launched on uninstall.
+    if (!is_chrome_frame_) {
+      FindChromeShortcut();
+      MiniInstallerTestUtil::CloseProcesses(
+          mini_installer_constants::kIEExecutable);
+      ASSERT_EQ(0,
+          base::GetProcessCount(mini_installer_constants::kIEExecutable, NULL));
+    }
   } else {
-    MiniInstallerTestUtil::CloseProcesses(installer::kNaClExe);
-  }
-  MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
-  std::wstring uninstall_path = GetUninstallPath();
-  if (uninstall_path.empty()) {
-    printf("\n %ls install is in a weird state. Cleaning the machine...\n",
-            product_name.c_str());
-    CleanChromeInstall();
-    return;
-  }
-  ASSERT_TRUE(file_util::PathExists(FilePath(uninstall_path)));
-  std::wstring uninstall_args(L"\"");
-  uninstall_args.append(uninstall_path);
-  uninstall_args.append(L"\" --uninstall --force-uninstall");
-  if (is_chrome_frame_)
-    uninstall_args.append(L" --chrome-frame");
-  if (install_type_ == mini_installer_constants::kSystemInstall)
-    uninstall_args = uninstall_args + L" --system-level";
-
-  base::ProcessHandle setup_handle;
-  base::LaunchProcess(uninstall_args, base::LaunchOptions(), &setup_handle);
-
-  if (is_chrome_frame_)
-    ASSERT_TRUE(CloseUninstallWindow());
-  ASSERT_TRUE(MiniInstallerTestUtil::VerifyProcessHandleClosed(setup_handle));
-  ASSERT_FALSE(CheckRegistryKeyOnUninstall(dist->GetVersionKey()));
-
-  DeleteUserDataFolder();
-  // Close IE survey window that gets launched on uninstall.
-  if (!is_chrome_frame_) {
-    FindChromeShortcut();
-    MiniInstallerTestUtil::CloseProcesses(
-        mini_installer_constants::kIEExecutable);
-    ASSERT_EQ(0,
-        base::GetProcessCount(mini_installer_constants::kIEExecutable, NULL));
+    LOG(WARNING) << "Chrome is not installed: attempt to uninstall failed.";
   }
 }
 
 void ChromeMiniInstaller::UnInstallChromeFrameWithIERunning() {
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   std::wstring product_name =
       mini_installer_constants::kChromeFrameProductName;
-  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   if (!CheckRegistryKey(dist->GetVersionKey())) {
     printf("%ls is not installed.\n", product_name.c_str());
     return;
   }
 
   MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
-  std::wstring uninstall_path = GetUninstallPath();
 
-  if (uninstall_path.empty()) {
-    printf("\n %ls install is in a weird state. Cleaning the machine...\n",
-            product_name.c_str());
+  CommandLine cmd = InstallUtil::GetChromeUninstallCmd(
+      system_install_, dist->GetType());
+
+  if (cmd.GetCommandLineString().empty()) {
+    LOG(ERROR) << "Unable to get uninstall command.";
     CleanChromeInstall();
-    return;
   }
 
-  ASSERT_TRUE(file_util::PathExists(FilePath(uninstall_path)));
-  std::wstring uninstall_args(L"\"");
-  uninstall_args.append(uninstall_path);
-  uninstall_args.append(L"\" --uninstall --force-uninstall");
-  uninstall_args.append(L" --chrome-frame");
+  ASSERT_TRUE(file_util::PathExists(cmd.GetProgram()));
 
-  if (install_type_ == mini_installer_constants::kSystemInstall)
-    uninstall_args = uninstall_args + L" --system-level";
+  cmd.AppendArg("--uninstall");

[kkania, 2011/09/26 20:00:53]

don't hardcode these

[Huyen, 2011/09/27 23:00:24]

Done.

+  cmd.AppendArg("--force-uninstall");
+  cmd.AppendArg("--chrome-frame");
+
+  if (system_install_)
+    cmd.AppendArg("--system-level");
 
   base::ProcessHandle setup_handle;
-  base::LaunchProcess(uninstall_args, base::LaunchOptions(), &setup_handle);
+  base::LaunchProcess(cmd, base::LaunchOptions(), &setup_handle);
 
   ASSERT_TRUE(CloseUninstallWindow());
   ASSERT_TRUE(MiniInstallerTestUtil::VerifyProcessHandleClosed(setup_handle));
   ASSERT_FALSE(CheckRegistryKeyOnUninstall(dist->GetVersionKey()));
 
   DeleteUserDataFolder();
   MiniInstallerTestUtil::CloseProcesses(
       mini_installer_constants::kIEProcessName);
 }
 
@@ skipping 91 matching lines @@
                    KEY_ALL_ACCESS) == ERROR_SUCCESS) &&
          (timer < 20000)) {
     base::PlatformThread::Sleep(200);
     timer = timer + 200;
   }
   return CheckRegistryKey(key_path);
 }
 
 // Deletes Installer folder from Applications directory.
 void ChromeMiniInstaller::DeleteFolder(const wchar_t* folder_name) {
-  FilePath install_path(GetChromeInstallDirectoryLocation());
+  FilePath install_path;
+  ASSERT_TRUE(GetChromeInstallDirectoryLocation(&install_path));
   std::wstring temp_chrome_dir;
   if (is_chrome_frame_) {
     temp_chrome_dir = mini_installer_constants::kChromeFrameAppDir;
-  } else {
-    temp_chrome_dir = mini_installer_constants::kChromeAppDir;
   }
 
   if (wcscmp(folder_name, L"version_folder") == 0) {
     std::wstring build_number;
     GetChromeVersionFromRegistry(&build_number);
     temp_chrome_dir = temp_chrome_dir + build_number;
     install_path = install_path.Append(temp_chrome_dir);
   } else if (wcscmp(folder_name, temp_chrome_dir.c_str()) == 0) {
     install_path = install_path.Append(folder_name).StripTrailingSeparators();
   }
-  printf("This path will be deleted: %ls\n", install_path.value().c_str());
+  ASSERT_TRUE(file_util::PathExists(install_path))
+      << "Could not find path to delete:" << install_path.value();
+  LOG(INFO) << "This path will be deleted: " << install_path.value();
   ASSERT_TRUE(file_util::Delete(install_path, true));
 }
 
 // Will delete user data profile.
 void ChromeMiniInstaller::DeleteUserDataFolder() {
   FilePath path = GetUserDataDirPath();
   if (file_util::PathExists(path))
     ASSERT_TRUE(file_util::Delete(path, true));
 }
 
@@ skipping 40 matching lines @@
   if (file_util::PathExists(path)) {
     return_val = true;
     uninstall_lnk = path;
     path = path.Append(mini_installer_constants::kChromeLaunchShortcut);
     uninstall_lnk = uninstall_lnk.Append(
         mini_installer_constants::kChromeUninstallShortcut);
     ASSERT_TRUE(file_util::PathExists(path));
     ASSERT_TRUE(file_util::PathExists(uninstall_lnk));
   }
   if (return_val) {
-    printf("Chrome shortcuts found are:\n%ls\n%ls\n\n",
-           path.value().c_str(), uninstall_lnk.value().c_str());
+    LOG(INFO) << "Found Chrome shortcuts:\n"
+              << path.value() << "\n"
+              << uninstall_lnk.value();
   } else {
-    printf("Chrome shortcuts not found\n\n");
+    LOG(INFO) << "No Chrome shortcuts found.";
   }
 }
 
-// This method returns path to either program files
-// or documents and setting based on the install type.
-std::wstring ChromeMiniInstaller::GetChromeInstallDirectoryLocation() {
-  FilePath path;
-  if (install_type_ == mini_installer_constants::kSystemInstall)
-    PathService::Get(base::DIR_PROGRAM_FILES, &path);
-  else
-    PathService::Get(base::DIR_LOCAL_APP_DATA, &path);
-  return path.value();
+bool ChromeMiniInstaller::GetChromeInstallDirectoryLocation(FilePath* path) {
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+  *path = installer::GetChromeInstallPath(system_install_, dist);
+  FilePath parent;
+  if (system_install_) {
+    PathService::Get(base::DIR_PROGRAM_FILES, &parent);
+    return file_util::ContainsPath(parent, *path);
+  } else {
+    PathService::Get(base::DIR_LOCAL_APP_DATA, &parent);
+    return file_util::ContainsPath(parent, *path);
+  }
 }
 
 FilePath ChromeMiniInstaller::GetStartMenuShortcutPath() {
   FilePath path_name;
-  if (install_type_ == mini_installer_constants::kSystemInstall)
+  if (system_install_)
     PathService::Get(base::DIR_COMMON_START_MENU, &path_name);
   else
     PathService::Get(base::DIR_START_MENU, &path_name);
   return path_name;
 }
 
-// Gets the path for uninstall.
-std::wstring ChromeMiniInstaller::GetUninstallPath() {
-  std::wstring path, reg_key_value;
-  if (!GetChromeVersionFromRegistry(&reg_key_value))
-    return std::wstring();
-  path = GetChromeInstallDirectoryLocation();
-  if (is_chrome_frame_) {
-    file_util::AppendToPath(&path,
-        mini_installer_constants::kChromeFrameAppDir);
-  } else {
-    file_util::AppendToPath(&path, mini_installer_constants::kChromeAppDir);
-  }
-  file_util::AppendToPath(&path, reg_key_value);
-  file_util::AppendToPath(&path, installer::kInstallerDir);
-  file_util::AppendToPath(&path,
-      mini_installer_constants::kChromeSetupExecutable);
-  if (!file_util::PathExists(FilePath(path))) {
-    printf("This uninstall path is not correct %ls. Will not proceed further",
-           path.c_str());
-    return L"";
-  }
-  printf("uninstall path is %ls\n", path.c_str());
-  return path;
-}
-
 // Returns Chrome pv registry key value
 bool ChromeMiniInstaller::GetChromeVersionFromRegistry(
     std::wstring* build_key_value) {
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   RegKey key(GetRootRegistryKey(), dist->GetVersionKey().c_str(), KEY_READ);
   LONG result = key.ReadValue(L"pv", build_key_value);
   if (result != ERROR_SUCCESS) {
-    printf("registry read for chrome version failed. error: %d\n", result);
+    LOG(WARNING) << "Registry read for Chrome version error: " << result;
     return false;
   }
-  printf("Build key value is %ls\n\n", build_key_value->c_str());
+  LOG(INFO) << "Build key value is " << build_key_value;
   return true;
 }
 
 // Get HKEY based on install type.
 HKEY ChromeMiniInstaller::GetRootRegistryKey() {
   HKEY type = HKEY_CURRENT_USER;
-  if (install_type_ == mini_installer_constants::kSystemInstall)
+  if (system_install_)
     type = HKEY_LOCAL_MACHINE;
   return type;
 }
 
 // Launches the chrome installer and waits for it to end.
 void ChromeMiniInstaller::LaunchInstaller(const FilePath& path,
-                                          const wchar_t* process_name) {
+    const CommandLine& args) {
   ASSERT_TRUE(file_util::PathExists(path));
-  std::wstring launch_args;
+  CommandLine installer(path);
+  installer.AppendArguments(args, false);
   if (is_chrome_frame_) {
-    launch_args.append(L" --do-not-create-shortcuts");
-    launch_args.append(L" --do-not-launch-chrome");
-    launch_args.append(L" --do-not-register-for-update-launch");
-    launch_args.append(L" --chrome-frame");
+    installer.AppendArg("--do-not-create-shortcuts");

[kkania, 2011/09/26 20:00:53]

same here

[Huyen, 2011/09/27 23:00:24]

Done.

+    installer.AppendArg("--do-not-launch-chrome");
+    installer.AppendArg("--do-not-register-for-update-launch");
+    installer.AppendArg("--chrome-frame");
   }
-  if (install_type_ == mini_installer_constants::kSystemInstall) {
-    launch_args.append(L" --system-level");
+  if (system_install_) {
+    installer.AppendArg("--system-level");
   }
 
+  LOG(INFO) << "Running installer command: "
+      << installer.GetCommandLineString();
   base::ProcessHandle app_handle;
-  base::LaunchProcess(L"\"" + path.value() + L"\"" + launch_args,
-                      base::LaunchOptions(), &app_handle);
+  if (!base::LaunchProcess(installer, base::LaunchOptions(), &app_handle))
+    LOG(ERROR) << "Installer failed.";
 
-  printf("Waiting while this process is running  %ls ....\n", process_name);
-  MiniInstallerTestUtil::VerifyProcessLaunch(process_name, true);
-  ASSERT_TRUE(MiniInstallerTestUtil::VerifyProcessHandleClosed(app_handle));
+  if (!base::WaitForSingleProcess(app_handle, 60 * 1000))
+    LOG(ERROR) << "Installer did not complete.";
+
+  // Kill Chrome instance launched by installer.
+  ChildProcessFilter children(base::GetProcId(app_handle));
+  if (base::GetProcessCount(installer::kChromeExe, &children) > 0) {
+    LOG(INFO) << "Terminating all Chrome instances now.";
+    MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);

[kkania, 2011/09/26 20:00:53]

what's better about this new way?

[Huyen, 2011/09/27 23:00:24]

I thought this wasn't really needed either. Considered killing only Chrome
started by the installer but see no value in that. Good catch, removing this.

+  }
 }
 
-// Gets the path to launch Chrome.
-bool ChromeMiniInstaller::GetChromeLaunchPath(FilePath* launch_path) {
-  std::wstring path;
-  path = GetChromeInstallDirectoryLocation();
-  file_util::AppendToPath(&path, mini_installer_constants::kChromeAppDir);
-  file_util::AppendToPath(&path, installer::kChromeExe);
-  *launch_path = FilePath(path);
-  return file_util::PathExists(*launch_path);
-}
+void ChromeMiniInstaller::LaunchChrome(bool kill) {
+  MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
 
-// Launch Chrome to see if it works after overinstall. Then close it.
-void ChromeMiniInstaller::LaunchAndCloseChrome(bool over_install) {
-  VerifyChromeLaunch(true);
-  if ((install_type_ == mini_installer_constants::kSystemInstall) &&
-      (!over_install)) {
-    MiniInstallerTestUtil::VerifyProcessLaunch(
-        installer::kChromeExe, true);
+  FilePath install_path;
+  ASSERT_TRUE(GetChromeInstallDirectoryLocation(&install_path));
+  install_path = install_path.Append(installer::kChromeExe);
+  CommandLine browser(install_path);
+
+  FilePath exe = browser.GetProgram();
+  LOG(INFO) << "Browser launch command: " << browser.GetCommandLineString();
+  base::ProcessHandle chrome;
+  bool launched = base::LaunchProcess(browser, base::LaunchOptions(), &chrome);
+  if (!launched) {
+    LOG(ERROR) << "Could not launch process: " << exe.value();
   }
-  MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
-}
 
-// This method will get Chrome exe path and launch it.
-void ChromeMiniInstaller::VerifyChromeLaunch(bool expected_status) {
-  FilePath launch_path;
-  GetChromeLaunchPath(&launch_path);
-  LaunchBrowser(launch_path, L"", expected_status);
+  ChromeProcessFilter filter(base::GetProcId(chrome));
+  ASSERT_TRUE(base::GetProcessCount(exe.BaseName().value(), &filter) == 1);
+  LOG(INFO) << "Launched browser.";
+  if (kill) {
+    ASSERT_TRUE(base::KillProcess(chrome, 0, true));
+  }
 }
 
 // Verifies Chrome/Chrome Frame install.
 void ChromeMiniInstaller::VerifyInstall(bool over_install) {
-  VerifyMachineState();
-  if (is_chrome_frame_) {
-    VerifyChromeFrameInstall();
-  } else {
-    if ((install_type_ == mini_installer_constants::kUserInstall) &&
-        (!over_install)) {
-      MiniInstallerTestUtil::VerifyProcessLaunch(
-          installer::kChromeExe, true);
-    }
-    base::PlatformThread::Sleep(800);
-    FindChromeShortcut();
-    LaunchAndCloseChrome(over_install);
-  }
-}
-
-// This method verifies installation of Chrome/Chrome Frame via machine
-// introspection.
-void ChromeMiniInstaller::VerifyMachineState() {
-  using installer::InstallationValidator;
-
   InstallationValidator::InstallationType type =
-      installer::ExpectValidInstallation(
-          install_type_ == mini_installer_constants::kSystemInstall);
+      installer::ExpectValidInstallation(system_install_);
   if (is_chrome_frame_) {
     EXPECT_NE(0,
               type & InstallationValidator::ProductBits::CHROME_FRAME_SINGLE);
   } else {
     EXPECT_NE(0, type & InstallationValidator::ProductBits::CHROME_SINGLE);
   }
+
+  if (is_chrome_frame_) {
+    VerifyChromeFrameInstall();
+  } else if ((!system_install_) &&

[kkania, 2011/09/26 20:00:53]

one line, don't need all these parens

[Huyen, 2011/09/27 23:00:24]

Done.

+      (!over_install)) {
+    MiniInstallerTestUtil::VerifyProcessLaunch(
+        installer::kChromeExe, true);
+  }
+
+  base::PlatformThread::Sleep(800);
+  FindChromeShortcut();
+  LaunchChrome(true);
 }
 
 // This method will verify if ChromeFrame installed successfully. It will
 // launch IE with cf:about:version, then check if
 // chrome.exe process got spawned.
 void ChromeMiniInstaller::VerifyChromeFrameInstall() {
   // Launch IE
   LaunchIE(L"gcf:about:version");
 
   // Check if Chrome process got spawned.
   MiniInstallerTestUtil::VerifyProcessLaunch(installer::kChromeExe, true);
 }
 
 void ChromeMiniInstaller::LaunchIE(const std::wstring& navigate_url) {
   FilePath browser_path;
   PathService::Get(base::DIR_PROGRAM_FILES, &browser_path);
   browser_path = browser_path.Append(mini_installer_constants::kIELocation);
   browser_path = browser_path.Append(mini_installer_constants::kIEProcessName);
 
   CommandLine cmd_line(browser_path);
   cmd_line.AppendArgNative(navigate_url);
 
   base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL);
 }
 
-// This method will launch any requested browser.
-void ChromeMiniInstaller::LaunchBrowser(const FilePath& path,
-                                        const std::wstring& args,
-                                        bool expected_status) {
-  LOG(INFO) << "Browser executable: " << path.value();
-  bool launched = base::LaunchProcess(
-      L"\"" + path.value() + L"\"" + L" " + args,
-      base::LaunchOptions(), NULL);
-  if (!launched) {
-    LOG(ERROR) << "Could not launch process: " << path.BaseName().value();
-  }
-  base::PlatformThread::Sleep(1000);
-  MiniInstallerTestUtil::VerifyProcessLaunch(path.BaseName().value().c_str(),
-                                             expected_status);
-}
-
 // This method compares the registry keys after overinstall.
 bool ChromeMiniInstaller::VerifyOverInstall(
     const std::wstring& value_before_overinstall,
     const std::wstring& value_after_overinstall) {
   int64 reg_key_value_before_overinstall;
   base::StringToInt64(value_before_overinstall,
                       &reg_key_value_before_overinstall);
   int64 reg_key_value_after_overinstall;
   base::StringToInt64(value_after_overinstall,
                       &reg_key_value_after_overinstall);
@@ skipping 19 matching lines @@
   else
     return false;
 }
 
 // Search all the specified |build| directory to find the latest
 // diff and full installers. |build| can be empty.
 bool ChromeMiniInstaller::LocateInstallers(
     const std::wstring& build) {
   FilePath::StringType full_installer_pattern =
       FILE_PATH_LITERAL("*_chrome_installer*");
-  FilePath::StringType diff_installer_pattern = FILE_PATH_LITERAL("*_from_*");
+  FilePath::StringType diff_installer_pattern =
+      FILE_PATH_LITERAL("*_from_*");
+  FilePath::StringType mini_installer_pattern =
+      FILE_PATH_LITERAL("mini_installer.exe");
   FilePath root(mini_installer_constants::kChromeInstallersLocation);
   std::vector<FilePath> paths;
   if (!FindMatchingFiles(root, build,
       file_util::FileEnumerator::DIRECTORIES, &paths)) {
     return false;
   }
 
   // Find full and diff installers;
   std::vector<FilePath>::const_iterator dir;
   for (dir = paths.begin(); dir != paths.end(); ++dir) {
     FilePath windir = dir->Append(
         mini_installer_constants::kWinFolder);
     if (FindNewestMatchingFile(windir, full_installer_pattern,
             file_util::FileEnumerator::FILES, &full_installer_) &&
         FindNewestMatchingFile(windir, diff_installer_pattern,
-            file_util::FileEnumerator::FILES, &diff_installer_)) {
+            file_util::FileEnumerator::FILES, &diff_installer_) &&
+        FindNewestMatchingFile(windir, mini_installer_pattern,
+            file_util::FileEnumerator::FILES, &mini_installer_)) {
       break;
     }
   }
 
-  // Set current build directory.
   if (full_installer_.empty() || diff_installer_.empty())
     return false;
 
   current_build_ =
       full_installer_.DirName().DirName().BaseName().value();
 
   // Find previous full installer.
   std::vector<std::wstring> tokenized_name;
   Tokenize(diff_installer_.BaseName().value(),
       L"_", &tokenized_name);
@@ skipping 25 matching lines @@
       L"%ls%ls_%ls.exe", mini_installer_constants::kUntaggedInstallerPattern,
           tokenizedBuildNumber[2].c_str(), tokenizedBuildNumber[3].c_str());
   standalone_installer = standalone_installer.Append(current_build_)
       .Append(mini_installer_constants::kWinFolder)
       .Append(standalone_installer_filename);
 
   standalone_installer_ = standalone_installer;
 
   return !standalone_installer_.empty();
 }