Create a "GetWOW64Status()" utility function and make the rest of the codebas...

Create a "GetWOW64Status()" utility function and make the rest of the codebase call it.

BUG=none
TEST=none

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

[Peter Kasting, 2011-03-03 22:22:50]

Ricardo: Sandbox
Brett: Rest

The sandbox changes mean there's now exactly one usage of the Wow64 class, and
it has one public function.  This suggests that someone should do a followup
change to move all this code over to the file where it's called.  I can file a
bug on that if you want.

[brettw, 2011-03-03 22:28:53]

LGTM

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.cc
File base/win/windows_version.cc (right):

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.cc#new...
base/win/windows_version.cc:81: if (is_wow64_process == NULL)
I guess I would have just written !is_wow_64_process but your code is OK if you
prefer that.

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.h
File base/win/windows_version.h (right):

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.h#newc...
base/win/windows_version.h:48: WOW64Status GetWOW64StatusForHandle(HANDLE
handle);
Maybe call this process_handle to make more clear what it's suppose to be a
handle for?

[Peter Kasting, 2011-03-03 22:33:23]

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.cc
File base/win/windows_version.cc (right):

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.cc#new...
base/win/windows_version.cc:81: if (is_wow64_process == NULL)
On 2011/03/03 22:28:53, brettw wrote:
> I guess I would have just written !is_wow_64_process but your code is OK if
you
> prefer that.

Don't care much.  Changed.

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.h
File base/win/windows_version.h (right):

http://codereview.chromium.org/6610029/diff/1/base/win/windows_version.h#newc...
base/win/windows_version.h:48: WOW64Status GetWOW64StatusForHandle(HANDLE
handle);
On 2011/03/03 22:28:53, brettw wrote:
> Maybe call this process_handle to make more clear what it's suppose to be a
> handle for?

I went one better than this and also changed the API name to
GetWOW64StatusForProcess() and the calling code in memory_details_win.cc from
using |handle| to using |process_handle|.

[rvargas (doing something else), 2011-03-04 00:12:18]

LGTM

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.cc
File base/win/windows_version.cc (right):

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.cc#...
base/win/windows_version.cc:80: GetProcAddress(GetModuleHandle(L"kernel32.dll"),
"IsWow64Process"));
Can we save this pointer? GetProcAddress is not that fast, and this may be
called a bunch of times for each process that we want to launch.

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.h
File base/win/windows_version.h (right):

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.h#n...
base/win/windows_version.h:33: enum WOW64Status {
I would prefer if the enum was not a status (as in something that can change),
and instead the name was more "fixed"... this "is running under wow" or not, as
opposed to "disabled" which would always make me double check what it means. But
I don't have a real suggestion for a better name, so if you don't have a better
name that's ok.

[Peter Kasting, 2011-03-04 01:11:48]

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.cc
File base/win/windows_version.cc (right):

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.cc#...
base/win/windows_version.cc:80: GetProcAddress(GetModuleHandle(L"kernel32.dll"),
"IsWow64Process"));
On 2011/03/04 00:12:18, rvargas wrote:
> Can we save this pointer? GetProcAddress is not that fast, and this may be
> called a bunch of times for each process that we want to launch.

But the entire return value is cached in GetWOW64Status().  So this should only
be called once.

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.h
File base/win/windows_version.h (right):

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.h#n...
base/win/windows_version.h:33: enum WOW64Status {
On 2011/03/04 00:12:18, rvargas wrote:
> I would prefer if the enum was not a status (as in something that can change),
> and instead the name was more "fixed"... this "is running under wow" or not,
as
> opposed to "disabled" which would always make me double check what it means.
But
> I don't have a real suggestion for a better name, so if you don't have a
better
> name that's ok.

Hmm.  I see your point.  I can't think of anything better though.  "WOW64State"
is equally bad.

[rvargas (doing something else), 2011-03-04 01:21:49]

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.cc
File base/win/windows_version.cc (right):

http://codereview.chromium.org/6610029/diff/2006/base/win/windows_version.cc#...
base/win/windows_version.cc:80: GetProcAddress(GetModuleHandle(L"kernel32.dll"),
"IsWow64Process"));
On 2011/03/04 01:11:48, Peter Kasting wrote:
> On 2011/03/04 00:12:18, rvargas wrote:
> > Can we save this pointer? GetProcAddress is not that fast, and this may be
> > called a bunch of times for each process that we want to launch.
> 
> But the entire return value is cached in GetWOW64Status().  So this should
only
> be called once.

Even better :)