Hello andreip,
I'd like you to do a code review. Please execute
g4 diff -c 9512681
or point your web browser to
http://mondrian/9512681
to review the following code:
Change 9512681 by stevebl...@steveblock-gears3 on 2008/12/23 12:00:01 *pending*
Adds to ProcessRestarter ability to search for a process by window
class.
R=andreip
[email protected],[email protected],[email protected]
DELTA=100 (83 added, 0 deleted, 17 changed)
OCL=9512681
Affected files ...
...
//depot/googleclient/gears/opensource/gears/installer/common/process_restarter.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/process_restarter.h#1
edit
... //depot/googleclient/gears/opensource/gears/installer/opera/setup.cc#2 edit
100 delta lines: 83 added, 0 deleted, 17 changed
Also consider running:
g4 lint -c 9512681
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [email protected].
Change 9512681 by stevebl...@steveblock-gears3 on 2008/12/23 12:00:01 *pending*
Adds to ProcessRestarter ability to search for a process by window
class.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/installer/common/process_restarter.cc#1
edit
...
//depot/googleclient/gears/opensource/gears/installer/common/process_restarter.h#1
edit
... //depot/googleclient/gears/opensource/gears/installer/opera/setup.cc#2 edit
====
//depot/googleclient/gears/opensource/gears/installer/common/process_restarter.cc#1
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/installer/common/process_restarter.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/installer/common/process_restarter.cc
2008-12-23 13:22:39.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/process_restarter.cc
2008-12-23 13:21:53.000000000 +0000
@@ -37,14 +37,34 @@
//------------------------------------------------------------------------------
ProcessRestarter::ProcessRestarter(const char16* process_name) {
- ProcessRestarter(process_name, NULL);
+ assert(process_name && wcslen(process_name) > 0);
+ Init(process_name, NULL, NULL);
}
ProcessRestarter::ProcessRestarter(const char16* process_name,
- const char16* window_name)
- : recursion_level_(0),
- process_name_(process_name),
- window_name_(window_name) {
+ const char16* window_name) {
+ assert(window_name && wcslen(window_name) > 0);
+ Init(process_name, window_name, NULL);
+}
+
+ProcessRestarter::ProcessRestarter(const char16* process_name,
+ const char16* window_name,
+ const char16* class_name) {
+ assert(class_name && wcslen(class_name) > 0);
+ Init(process_name, window_name, class_name);
+}
+
+void ProcessRestarter::Init(const char16* process_name,
+ const char16* window_name,
+ const char16* class_name) {
+ recursion_level_ = 0;
+ process_name_ = process_name;
+ window_name_ = window_name;
+ class_name_ = class_name;
+
+ assert((process_name_ && wcslen(process_name_) > 0) ||
+ (window_name_ && wcslen(window_name_) > 0) ||
+ (class_name_ && wcslen(class_name_) > 0));
}
// Clean up if we've left the process handle open
@@ -176,13 +196,22 @@
if (SUCCEEDED(FindProcessInstancesUsingSnapshot(is_running))) {
return S_OK;
}
- return FindProcessInstancesUsingFindWindow(is_running);
+ if (SUCCEEDED(FindProcessInstancesUsingFindWindow(is_running))) {
+ return S_OK;
+ }
+ return FindProcessInstancesUsingFindClass(is_running);
}
// See http://msdn2.microsoft.com/en-us/library/aa446560.aspx for details
// on how to enumerate processes on Windows Mobile.
HRESULT ProcessRestarter::FindProcessInstancesUsingSnapshot(bool* found) {
ASSERT(found);
+
+ // We fail fast if process_name_ is NULL or the empty string.
+ if (process_name_ == NULL || wcslen(process_name_) == 0) {
+ return E_FAIL;
+ }
+
// Clear the process_ids_.
process_ids_.clear();
// Create a snapshot of the processes running in the system.
@@ -220,13 +249,43 @@
HRESULT ProcessRestarter::FindProcessInstancesUsingFindWindow(bool* found) {
ASSERT(found);
- // We fail fast if the window_name is NULL or the empty string.
+ // We fail fast if window_name_ is NULL or the empty string.
if (window_name_ == NULL || wcslen(window_name_) == 0) {
return E_FAIL;
}
HRESULT result;
HWND handle = FindWindow(NULL, window_name_);
+ if (handle == NULL) {
+ // We didn't find the window. Is this because there is no such window
+ // or because FindWindow failed for a different reason?
+ result = HRESULT_FROM_WIN32(::GetLastError());
+ if (SUCCEEDED(result)) {
+ // There is no such window.
+ *found = false;
+ }
+ return result;
+ }
+
+ // Found a window, get the PID.
+ uint32 process_id = 0;
+ uint32 thread_id =
+ ::GetWindowThreadProcessId(handle,
reinterpret_cast<DWORD*>(&process_id));
+ process_ids_.push_back(process_id);
+ *found = true;
+ return S_OK;
+}
+
+HRESULT ProcessRestarter::FindProcessInstancesUsingFindClass(bool* found) {
+ ASSERT(found);
+
+ // We fail fast if class_name_ is NULL or the empty string.
+ if (class_name_ == NULL || wcslen(class_name_) == 0) {
+ return E_FAIL;
+ }
+
+ HRESULT result;
+ HWND handle = FindWindow(class_name_, NULL);
if (handle == NULL) {
// We didn't find the window. Is this because there is no such window
// or because FindWindow failed for a different reason?
====
//depot/googleclient/gears/opensource/gears/installer/common/process_restarter.h#1
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/installer/common/process_restarter.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/installer/common/process_restarter.h
2008-12-23 13:22:39.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/common/process_restarter.h
2008-12-23 13:20:50.000000000 +0000
@@ -40,15 +40,25 @@
static const int KILL_METHOD_2_THREAD_MESSAGE = 0x02;
static const int KILL_METHOD_3_TERMINATE_PROCESS = 0x04;
- // Creates the object given the process_name to kill.
+ // Creates the object given the process_name to kill. process_name must not
be
+ // NULL or empty.
explicit ProcessRestarter(const char16* process_name);
- // Creates the object given the process_name to kill.
- // The window_name parameter denotes the name of the main window created
- // by the process. If not NULL or empty, this string will be used as a
- // fallback mechanism for finding the process handle when finding by
- // process name failed due to a win32 API error.
+ // As above, but adds window_name. The window_name parameter denotes the name
+ // of the main window created by the process. window_name must not be NULL or
+ // empty. This string will be used as a fallback mechanism for finding the
+ // process handle when finding by process name failed due to a win32 API
+ // error.
ProcessRestarter(const char16* process_name,
const char16* window_name);
+ // As above, but adds class_name. The class_name parameter denotes the name
+ // of the class of the main window created by the process. class_name must
not
+ // be NULL or empty. This string will be used as a fallback mechanism for
+ // finding the process handle when finding by process name and window name
+ // failed.
+ ProcessRestarter(const char16* process_name,
+ const char16* window_name,
+ const char16* class_name);
+
virtual ~ProcessRestarter();
// Go through process list try to find the required one to kill, trying three
@@ -71,6 +81,11 @@
HRESULT IsProcessRunning(bool* is_running);
private:
+ // Initializes member variables. Must be called from constructors.
+ void Init(const char16* process_name,
+ const char16* window_name,
+ const char16* class_name);
+
// Finds all instances of the process.
// The found parameter is only set if the return value denotes success.
HRESULT FindProcessInstances(bool* found);
@@ -79,9 +94,13 @@
// The found parameter is only set if the return value denotes success.
HRESULT FindProcessInstancesUsingSnapshot(bool* found);
- // Finds all instances of the process using ::FindWindow
+ // Finds all instances of the process using ::FindWindow with the window
name.
// The found parameter is only set if the return value denotes success.
HRESULT FindProcessInstancesUsingFindWindow(bool* found);
+
+ // Finds all instances of the process using ::FindWindow with the window
+ // class. The found parameter is only set if the return value denotes
success.
+ HRESULT FindProcessInstancesUsingFindClass(bool* found);
// Will try to open handle to each instance.
// Leaves process handles open (in member process_handles_).
@@ -133,6 +152,7 @@
// Private member variables:
const char16* process_name_;
const char16* window_name_;
+ const char16* class_name_;
// One process can have several instances
// running. This array will keep handles to all
// instances of the process.
==== //depot/googleclient/gears/opensource/gears/installer/opera/setup.cc#2 -
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/installer/opera/setup.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/installer/opera/setup.cc
2008-12-23 13:22:39.000000000 +0000
+++ googleclient/gears/opensource/gears/installer/opera/setup.cc
2008-12-23 13:12:56.000000000 +0000
@@ -59,7 +59,9 @@
LPCTSTR installation_directory) {
// We only handle the first call.
if (!is_first_call) return kContinue;
- ProcessRestarter running_process(kOperaProcessName, NULL);
+ ProcessRestarter running_process(kOperaProcessName,
+ NULL,
+ L"Opera_MainWndClass");
running_process.KillTheProcess(
1000,
ProcessRestarter::KILL_METHOD_1_WINDOW_MESSAGE |
@@ -78,7 +80,9 @@
WORD failed_registry_keys,
WORD failed_registry_values,
WORD failed_shortcuts) {
- ProcessRestarter running_process(kOperaProcessName, NULL);
+ ProcessRestarter running_process(kOperaProcessName,
+ NULL,
+ L"Opera_MainWndClass");
bool is_running = false;
HRESULT result = running_process.IsProcessRunning(&is_running);
if ((SUCCEEDED(result) && is_running) || FAILED(result)) {
@@ -99,7 +103,7 @@
MessageBox(parent_window, fail_message, title, MB_OK | MB_ICONEXCLAMATION);
} else {
- ProcessRestarter restart_process(kOperaLaunchExe, NULL);
+ ProcessRestarter restart_process(kOperaLaunchExe);
if (FAILED(restart_process.StartTheProcess(kGearsSite))) {
// Unfortunately we failed, so inform the user.
LPCTSTR title = reinterpret_cast<LPCTSTR>(LoadString(
