Move common code into process_util.cc

Move common code into process_util.cc.
Fix namespace usage.
Change ProcessEntry to have a common interface accross platforms and change ProcessFilter::Includes() to make use of it.
Split NamedProcessIterator in two.

BUG=none
TEST=none

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

[M-A Ruel, 2010-04-23 17:24:14]

http://codereview.chromium.org/1689012/diff/1/7
File base/process_util_mac.mm (right):

http://codereview.chromium.org/1689012/diff/1/7#newcode76
base/process_util_mac.mm:76: num_of_kinfo_proc += 16;
I thought 4 was a bit too concervative, especially in a context of high process
creation rate.

http://codereview.chromium.org/1689012/diff/1/7#newcode145
base/process_util_mac.mm:145: entry_.pid = kinfo.kp_proc.p_pid;
Don't pre-filter anymore.

http://codereview.chromium.org/1689012/diff/1/8
File base/process_util_posix.cc (left):

http://codereview.chromium.org/1689012/diff/1/8#oldcode581
base/process_util_posix.cc:581: ProcessMetrics::ProcessMetrics(ProcessHandle
process)
This code was a big mess, I split it up and moved to the relevant files.

[wtc, 2010-04-23 20:58:35]

http://codereview.chromium.org/1689012/diff/1/3
File base/process.h (right):

http://codereview.chromium.org/1689012/diff/1/3#newcode31
base/process.h:31: #endif  // defined(OS_POSIX)
Nit: this should be defined(OS_WIN) to be consistent with
similar comments you added to other files.

http://codereview.chromium.org/1689012/diff/1/4
File base/process_util.cc (right):

http://codereview.chromium.org/1689012/diff/1/4#newcode5
base/process_util.cc:5: // This file/namespace contains utility functions for
enumerating, ending and
Nit: remove "namespace".  The "base" namespace is more than
process utility functions.

http://codereview.chromium.org/1689012/diff/1/5
File base/process_util.h (right):

http://codereview.chromium.org/1689012/diff/1/5#newcode36
base/process_util.h:36: #include "base/file_descriptor_shuffle.h"
Remove this line.  It's already included on line 33.

http://codereview.chromium.org/1689012/diff/1/5#newcode51
base/process_util.h:51: ProcessId getPid() const { return th32ProcessID; }
Nit: these two methods should be named either
    get_pid
    get_parent_pid (or get_ppid)
or
    GetPid
    GetParentPid
to follow the Style Guide.

Or are you following the Windows style (since PROCESSENTRY32
is a Windows type)?

Nit: it would be nice to provide an accessor for
szExeFile.  "szExeFile" looks out of place in cross-platform
code.

http://codereview.chromium.org/1689012/diff/1/5#newcode340
base/process_util.h:340: // Take a snapshot of all the ProcessEntry found.
Nit: use "Takes" to match the style of the comments for the
previous method.

http://codereview.chromium.org/1689012/diff/1/6
File base/process_util_linux.cc (right):

http://codereview.chromium.org/1689012/diff/1/6#newcode147
base/process_util_linux.cc:147: // pid (name) runstate ppid gid
This line has five arguments but the printf format on the
previous line has only four things.  I guess you should add
%d to the previous line?

http://codereview.chromium.org/1689012/diff/1/9
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/1689012/diff/1/9#newcode40
base/process_util_unittest.cc:40: const wchar_t* const kProcessName =
L"base_unittests.exe";
Nit: you can just say
  const wchar_t kProcessName[] = L"base_unittests.exe";

then you need only one 'const' (because an array's name is
like a constant pointer).

http://codereview.chromium.org/1689012/diff/1/10
File base/process_util_win.cc (right):

http://codereview.chromium.org/1689012/diff/1/10#newcode226
base/process_util_win.cc:226: DLOG(ERROR) << "Unable to open process " <<
process_id << " : " <<
Nit: wrap this line like this:
    DLOG(ERROR) << "Unable to open process " << process_id << " : "
                << GetLastError();

See http://www.chromium.org/developers/coding-style#Code_Formatting

http://codereview.chromium.org/1689012/diff/1/11
File base/shared_memory.h (right):

http://codereview.chromium.org/1689012/diff/1/11#newcode54
base/shared_memory.h:54: ProcessHandle process);
Nit: this argument should be aligned with the first argument
on the previous line.

[M-A Ruel, 2010-04-27 17:45:58]

Every other changes done.

http://codereview.chromium.org/1689012/diff/1/9
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/1689012/diff/1/9#newcode40
base/process_util_unittest.cc:40: const wchar_t* const kProcessName =
L"base_unittests.exe";
On 2010/04/23 20:58:35, wtc wrote:
> Nit: you can just say
>   const wchar_t kProcessName[] = L"base_unittests.exe";
> 
> then you need only one 'const' (because an array's name is
> like a constant pointer).

A char array is not the same as a constant pointer. I tried to do the reverse
work a few times since char array can't be string pooled while const string
pointers can.

[wtc, 2010-04-27 18:30:07]

LGTM.  Please commit the CL after making my suggested changes
below.  Don't need to wait for my review.

I'm terribly sorry I suggested the wrong names for the
accessors last time.

http://codereview.chromium.org/1689012/diff/1/9
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/1689012/diff/1/9#newcode40
base/process_util_unittest.cc:40: const wchar_t* const kProcessName =
L"base_unittests.exe";
On 2010/04/27 17:45:58, Marc-Antoine Ruel wrote:
>
> A char array is not the same as a constant pointer. I tried to do the reverse
> work a few times since char array can't be string pooled while const string
> pointers can.

I see.  This is not obvious at all.  Is this a fundamental
limitation that future compilers won't be able to overcome?

http://codereview.chromium.org/1689012/diff/14001/15004
File base/process_util.h (right):

http://codereview.chromium.org/1689012/diff/14001/15004#newcode50
base/process_util.h:50: ProcessId get_pid() const { return th32ProcessID; }
Nit: I'm sorry I suggested the wrong names last time.
By our Style Guide, accessors don't need the get_ prefix,
so these should be just pid(), parent_pid(), and exe_file():

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

get_exe_file() returns a wchar_t* pointer on Windows but a
char* pointer on other platforms.  Is that intentional?

The ProcessEntry structure doesn't have a 'gid' member on
Windows.  Is that OK?

http://codereview.chromium.org/1689012/diff/14001/15009
File base/process_util_win.cc (right):

http://codereview.chromium.org/1689012/diff/14001/15009#newcode227
base/process_util_win.cc:227: << GetLastError();
Nit: align this << with the first << on the previous line.

http://codereview.chromium.org/1689012/diff/14001/15012
File chrome/browser/process_singleton_win_uitest.cc (right):

http://codereview.chromium.org/1689012/diff/14001/15012#newcode144
chrome/browser/process_singleton_win_uitest.cc:144: ancestor_pids_.end()) {
Nit: this line should be indented by 4 from the "if" (rather
than the opening parenthesis) on the previous line.

But after get_parent_pid() is shortened to parent_pid(), you
probably don't need to wrap this line any more.

[M-A Ruel, 2010-04-29 17:34:24]

FYI,

http://codereview.chromium.org/1689012/diff/1/9
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/1689012/diff/1/9#newcode40
base/process_util_unittest.cc:40: const wchar_t* const kProcessName =
L"base_unittests.exe";
On 2010/04/27 18:30:07, wtc wrote:
> I see.  This is not obvious at all.  Is this a fundamental
> limitation that future compilers won't be able to overcome?

It's the definition of the language, you specifically request for an array
otherwise.

http://codereview.chromium.org/1689012/diff/14001/15004
File base/process_util.h (right):

http://codereview.chromium.org/1689012/diff/14001/15004#newcode50
base/process_util.h:50: ProcessId get_pid() const { return th32ProcessID; }
On 2010/04/27 18:30:07, wtc wrote:
> The ProcessEntry structure doesn't have a 'gid' member on
> Windows.  Is that OK?

This concept doesn't exist on Windows.