Replace chrome_process_filter with chrome_process_util.

Replace chrome_process_filter with chrome_process_util.

- move code only used by tests to chrome/test
- make a better, more portable abstraction

For now, it still only works on Windows. But this is the first step
to porting this part of code.

[Paweł Hajdan Jr., 2009-03-27 15:19:40]

Link to discussion:
http://groups.google.com/group/chromium-dev/browse_thread/thread/9a5cf684832c...

[Paweł Hajdan Jr., 2009-03-30 14:11:27]

ping.

[Evan Martin, 2009-03-30 16:39:45]

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

http://codereview.chromium.org/54003/diff/1/2#newcode37
Line 37: return OpenProcess(PROCESS_DUP_HANDLE |
Does anything else rely on the old behavior (flags) of OpenProcessHandle?  Does
SYNCHRONIZE change our performance characteristics?

http://codereview.chromium.org/54003/diff/1/9
File chrome/test/chrome_process_util.cc (right):

http://codereview.chromium.org/54003/diff/1/9#newcode16
Line 16: const TimeDelta kExitTimeout = TimeDelta::FromMilliseconds(5000);
Could you comment on this?  (E.g. whether this is the total time we'll wait or
the per-process time we'll wait?)

http://codereview.chromium.org/54003/diff/1/9#newcode17
Line 17: const int kExitCode = 1;
Why 1 here?

http://codereview.chromium.org/54003/diff/1/11
File chrome/test/chrome_process_util_win.cc (right):

http://codereview.chromium.org/54003/diff/1/11#newcode21
Line 21: virtual bool Includes(uint32 pid, uint32 parent_pid) const {
Here I see uint32 for pid, elsewhere int, and on Unix I think it's pid_t.  Do we
have a proper typedef for it?

[tommi (sloooow) - chröme, 2009-03-31 17:56:48]

I took a quick look and noticed one thing that might be a problem.

http://codereview.chromium.org/54003/diff/1/9
File chrome/test/chrome_process_util.cc (right):

http://codereview.chromium.org/54003/diff/1/9#newcode21
Line 21: std::vector<base::ProcessHandle> handles;
looks like all of these handles are being leaked?

http://codereview.chromium.org/54003/diff/1/9#newcode25
Line 25: handles.push_back(base::OpenProcessHandle(*it));
error handling?  or is pushing a NULL handle fine?

http://codereview.chromium.org/54003/diff/1/10
File chrome/test/chrome_process_util.h (right):

http://codereview.chromium.org/54003/diff/1/10#newcode12
Line 12: typedef std::vector<int> ChromeProcessList;
I guess this will become std::vector<ProcessId> when the other change gets
checked in?

http://codereview.chromium.org/54003/diff/1/11
File chrome/test/chrome_process_util_win.cc (right):

http://codereview.chromium.org/54003/diff/1/11#newcode41
Line 41: DWORD browser_pid;
initialize this variable in case the function fails.

[Paweł Hajdan Jr., 2009-04-02 17:34:44]

On 2009/03/30 16:39:45, Evan Martin wrote:
> http://codereview.chromium.org/54003/diff/1/2
> File base/process_util_win.cc (right):
> 
> http://codereview.chromium.org/54003/diff/1/2#newcode37
> Line 37: return OpenProcess(PROCESS_DUP_HANDLE |
> Does anything else rely on the old behavior (flags) of OpenProcessHandle? 
Does
> SYNCHRONIZE change our performance characteristics?

So I think that adding more permissions to the handle won't break old callers
(they could already terminate the process, so waiting for it to finish or
looking how much memory it uses doesn't look harmful).

I also think that it won't affect performance (I admin I'm a bit speculative
here). From my understanding of docs this only affects permission checks.

> http://codereview.chromium.org/54003/diff/1/9
> File chrome/test/chrome_process_util.cc (right):
> 
> http://codereview.chromium.org/54003/diff/1/9#newcode16
> Line 16: const TimeDelta kExitTimeout = TimeDelta::FromMilliseconds(5000);
> Could you comment on this?  (E.g. whether this is the total time we'll wait or
> the per-process time we'll wait?)

Done.

> http://codereview.chromium.org/54003/diff/1/9#newcode17
> Line 17: const int kExitCode = 1;
> Why 1 here?

Used a more descriptive constant.

> http://codereview.chromium.org/54003/diff/1/11
> File chrome/test/chrome_process_util_win.cc (right):
> 
> http://codereview.chromium.org/54003/diff/1/11#newcode21
> Line 21: virtual bool Includes(uint32 pid, uint32 parent_pid) const {
> Here I see uint32 for pid, elsewhere int, and on Unix I think it's pid_t.  Do
we
> have a proper typedef for it?

After my recent check-in, yes: ProcessId.

On 2009/03/31 17:56:48, tommi wrote:
> I took a quick look and noticed one thing that might be a problem.
> 
> http://codereview.chromium.org/54003/diff/1/9
> File chrome/test/chrome_process_util.cc (right):
> 
> http://codereview.chromium.org/54003/diff/1/9#newcode21
> Line 21: std::vector<base::ProcessHandle> handles;
> looks like all of these handles are being leaked?

Fixed.

> http://codereview.chromium.org/54003/diff/1/9#newcode25
> Line 25: handles.push_back(base::OpenProcessHandle(*it));
> error handling?  or is pushing a NULL handle fine?

We would have to recognize a valid handle (on POSIX the handle is not a pointer,
so we can't just compare with NULL). I will take a closer look.

> http://codereview.chromium.org/54003/diff/1/10
> File chrome/test/chrome_process_util.h (right):
> 
> http://codereview.chromium.org/54003/diff/1/10#newcode12
> Line 12: typedef std::vector<int> ChromeProcessList;
> I guess this will become std::vector<ProcessId> when the other change gets
> checked in?

Sure.

> http://codereview.chromium.org/54003/diff/1/11
> File chrome/test/chrome_process_util_win.cc (right):
> 
> http://codereview.chromium.org/54003/diff/1/11#newcode41
> Line 41: DWORD browser_pid;
> initialize this variable in case the function fails.

Done.

[tommi (sloooow) - chröme, 2009-04-02 18:08:33]

rubber stamp lgtm.

http://codereview.chromium.org/54003/diff/2005/2012
File chrome/test/chrome_process_util.cc (right):

http://codereview.chromium.org/54003/diff/2005/2012#newcode27
Line 27: handles.push_back(base::OpenProcessHandle(*it));
If we can't check for errors on the return value then I think we need to change
OpenProcessHandle to return an error value (e.g. bool) and return the handle via
an output parameter.

I realize that's a greater change than what you're touching here, so I'm leaving
it up to you if you want to make that change now.

[Paweł Hajdan Jr., 2009-04-06 20:04:57]

Added error handling. Please take a look.

[tommi (sloooow) - chröme, 2009-04-06 20:35:04]

lgtm

[Evan Martin, 2009-04-09 00:48:09]

ping?  this lgtm as well

[Evan Stade, 2009-04-09 18:55:59]

hmm, it appears that 3 of the files are missing, so I can't land this. Is it
possible you can re-upload?