Fix up GCAPI's LaunchGoogleChrome() method.

chrome/installer/gcapi/gcapi.cc

Index: chrome/installer/gcapi/gcapi.cc
===================================================================
--- chrome/installer/gcapi/gcapi.cc	(revision 112456)
+++ chrome/installer/gcapi/gcapi.cc	(working copy)
@@ -10,8 +10,6 @@
 
 #include "chrome/installer/gcapi/gcapi.h"
 
-#include <atlbase.h>
-#include <atlcom.h>
 #include <sddl.h>
 #define STRSAFE_NO_DEPRECATE
 #include <strsafe.h>
@@ -23,13 +21,26 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
 #include "base/string_number_conversions.h"
 #include "base/time.h"
 #include "base/win/registry.h"
+#include "base/win/scoped_com_initializer.h"
+#include "base/win/scoped_comptr.h"
+#include "base/win/scoped_handle.h"
 #include "chrome/installer/util/google_update_constants.h"
+#include "chrome/installer/util/util_constants.h"
 
 #include "google_update_idl.h"  // NOLINT
 
+using base::win::RegKey;
+using base::win::ScopedCOMInitializer;
+using base::win::ScopedComPtr;
+using base::win::ScopedHandle;
+using base::Time;

[grt (UTC plus 2), 2011/12/05 15:14:22]

what do you think about moving these above RegKey so that the sorting is akin to
the #include order?

[robertshield, 2011/12/05 18:18:00]

I think that's a splendid idea.

+using base::TimeDelta;
+
 namespace {
 
 const wchar_t kChromeRegClientsKey[] =
@@ -339,29 +350,45 @@
 }
 
 BOOL __stdcall LaunchGoogleChrome() {
-  wchar_t launch_cmd[MAX_PATH];
-  size_t size = _countof(launch_cmd);
-  if (!ReadValueFromRegistry(HKEY_LOCAL_MACHINE, kChromeRegClientStateKey,
-                             kChromeRegLastLaunchCmd, launch_cmd, &size)) {
-    size = _countof(launch_cmd);
-    if (!ReadValueFromRegistry(HKEY_LOCAL_MACHINE, kChromeRegClientStateKey,
-                               kChromeRegLaunchCmd, launch_cmd, &size)) {
+  // Check to make sure we have a valid Chrome installation.
+  HKEY install_key = HKEY_LOCAL_MACHINE;
+  if (!IsChromeInstalled(install_key)) {
+    install_key = HKEY_CURRENT_USER;
+    if (!IsChromeInstalled(install_key)) {

[Roger Tawa OOO till Jul 10th, 2011/12/05 16:19:45]

For my own info, why try HKLM before HKCU?

[robertshield, 2011/12/05 18:18:00]

If both exist and the user-level one is launched, it will self-destruct and
delegate to the system-level one. I figured this process might interact badly
with the LaunchGoogleChromeWithDimensions code below.

I could also just never launch the user-level one (like the old code), but Mike
stated that this code could also be used with user-level Chrome installations so
I made this change.

       return false;
     }
   }
 
-  HRESULT hr = ::CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
-  if (hr != S_OK) {
-    if (hr == S_FALSE)
-      ::CoUninitialize();
-    return false;
+  // Now grab the uninstall string from the appropriate ClientState key
+  // and use that as the base for a path to chrome.exe.
+  FilePath chrome_exe_path;
+  RegKey client_state(install_key, kChromeRegClientStateKey, KEY_QUERY_VALUE);
+  if (client_state.Valid()) {
+    std::wstring uninstall_string;
+    if (client_state.ReadValue(installer::kUninstallStringField,
+                               &uninstall_string) == ERROR_SUCCESS) {
+      // The uninstall path contains the path to setup.exe which is two levels
+      // down from chrome.exe. Move up two levels (plus one to drop the file
+      // name) and look for chrome.exe from there.
+      FilePath uninstall_path(uninstall_string);
+      chrome_exe_path = uninstall_path.DirName()
+                                      .DirName()
+                                      .DirName()
+                                      .Append(installer::kChromeExe);
+      if (!file_util::PathExists(chrome_exe_path)) {
+        // By way of mild future proofing, look up one to see if there's a
+        // chrome.exe in the version directory
+        chrome_exe_path =
+            uninstall_path.DirName().DirName().Append(installer::kChromeExe);
+      }

[Roger Tawa OOO till Jul 10th, 2011/12/05 16:19:45]

You don't want to do another path exists check here, instead of relying on the
code below to eventually fail?

[robertshield, 2011/12/05 18:18:00]

Good idea, done.

+    }
   }
 
+  ScopedCOMInitializer com_initializer;
   if (::CoInitializeSecurity(NULL, -1, NULL, NULL,
                              RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
                              RPC_C_IMP_LEVEL_IDENTIFY, NULL,
                              EOAC_DYNAMIC_CLOAKING, NULL) != S_OK) {
-    ::CoUninitialize();
     return false;
   }
 
@@ -369,7 +396,6 @@
   if (IsRunningElevated()) {
     wchar_t* curr_proc_sid;
     if (!GetUserIdForProcess(GetCurrentProcessId(), &curr_proc_sid)) {
-      ::CoUninitialize();
       return false;
     }
 
@@ -377,16 +403,17 @@
     ::GetWindowThreadProcessId(::GetShellWindow(), &pid);
     if (pid <= 0) {
       ::LocalFree(curr_proc_sid);
-      ::CoUninitialize();
       return false;
     }
 
     wchar_t* exp_proc_sid;
     if (GetUserIdForProcess(pid, &exp_proc_sid)) {
       if (_wcsicmp(curr_proc_sid, exp_proc_sid) == 0) {
-        HANDLE process_handle = ::OpenProcess(
-            PROCESS_DUP_HANDLE | PROCESS_QUERY_INFORMATION, TRUE, pid);
-        if (process_handle != NULL) {
+        ScopedHandle process_handle(
+            ::OpenProcess(PROCESS_DUP_HANDLE | PROCESS_QUERY_INFORMATION,
+                          TRUE,
+                          pid));
+        if (process_handle.IsValid()) {
           HANDLE process_token = NULL;
           HANDLE user_token = NULL;
           if (::OpenProcessToken(process_handle, TOKEN_DUPLICATE | TOKEN_QUERY,
@@ -403,7 +430,6 @@
             ::CloseHandle(user_token);
           if (process_token)
             ::CloseHandle(process_token);
-          ::CloseHandle(process_handle);
         }
       }
       ::LocalFree(exp_proc_sid);
@@ -411,30 +437,30 @@
 
     ::LocalFree(curr_proc_sid);
     if (!impersonation_success) {
-      ::CoUninitialize();
       return false;
     }
   }
 
   bool ret = false;
-  CComPtr<IProcessLauncher> ipl;
-  if (!FAILED(ipl.CoCreateInstance(__uuidof(ProcessLauncherClass), NULL,
+  ScopedComPtr<IProcessLauncher> ipl;
+  if (SUCCEEDED(ipl.CreateInstance(__uuidof(ProcessLauncherClass),

[grt (UTC plus 2), 2011/12/05 15:14:22]

what, !FAILED implies SUCCEEDED to you?

[robertshield, 2011/12/05 18:18:00]

#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)
#define FAILED(hr) (((HRESULT)(hr)) < 0)

yes. 

I suppose there could be degrees of failure, DIDNT_REALLY_WORK(), FAILED(),
BLEW_UP_APOCALYPTICALLY(), etc.. but I'm sticking with the glass-half-full
outlook on life.

+                                   NULL,
                                    CLSCTX_LOCAL_SERVER))) {
-    if (!FAILED(ipl->LaunchCmdLine(launch_cmd)))
+    if (SUCCEEDED(ipl->LaunchCmdLine(chrome_exe_path.value().c_str())))
       ret = true;
     ipl.Release();
   }
 
   if (impersonation_success)
     ::RevertToSelf();
-  ::CoUninitialize();
   return ret;
 }
 
 BOOL __stdcall LaunchGoogleChromeWithDimensions(int x,
                                                 int y,
                                                 int width,
-                                                int height) {
+                                                int height,
+                                                bool in_background) {
   if (!LaunchGoogleChrome())
     return false;
 
@@ -445,7 +471,7 @@
   // yet. Wait for it to appear for 10 seconds, but exit if it takes longer
   // than that.
   while (!handle && seconds_elapsed < 10) {
-    handle = FindWindowEx(NULL, handle, L"Chrome_WindowImpl_0", NULL);
+    handle = FindWindowEx(NULL, handle, L"Chrome_WidgetWin_0", NULL);

[grt (UTC plus 2), 2011/12/05 15:14:22]

how about making a kChromeWindowClass for this?

[robertshield, 2011/12/05 18:18:00]

Done.

     if (!handle) {
       Sleep(1000);
       seconds_elapsed++;
@@ -460,20 +486,20 @@
 
   // This loop iterates through all of the top-level Windows named
   // Chrome_WindowImpl_0, and looks for the first one with any children.
-  while (handle && !FindWindowEx(handle, NULL, L"Chrome_WindowImpl_0", NULL)) {
+  while (handle && !FindWindowEx(handle, NULL, L"Chrome_WidgetWin_0", NULL)) {
     // Get the next top-level Chrome window.
-    handle = FindWindowEx(NULL, handle, L"Chrome_WindowImpl_0", NULL);
+    handle = FindWindowEx(NULL, handle, L"Chrome_WidgetWin_0", NULL);
   }
 
+  HWND set_window_hwnd_insert_after = in_background ? HWND_BOTTOM : NULL;
+  DWORD set_window_flags = in_background ? SWP_NOACTIVATE : SWP_NOZORDER;
+
   return (handle &&
-          SetWindowPos(handle, 0, x, y, width, height, SWP_NOZORDER));
+          SetWindowPos(handle, set_window_hwnd_insert_after, x, y,
+                       width, height, set_window_flags));
 }
 
 int __stdcall GoogleChromeDaysSinceLastRun() {
-  using base::win::RegKey;
-  using base::Time;
-  using base::TimeDelta;
-
   int days_since_last_run = std::numeric_limits<int>::max();
 
   struct {