Porting in chrome/

Porting in chrome/

[Paweł Hajdan Jr., 2009-01-21 11:35:41]

This makes additional 8 files compile, but I'm not sure if we want all the
#ifdefs and wstring hacks.

I could eventually try the refactoring in bookmarks storage, but it could
require changes in other places where it's used and so on (but it'll have to be
done at some point).

[Dean McNamee, 2009-01-21 14:02:13]

I agree with you on the wstring hacks.  I've been meaning to try to find a few
better solutions for cases like this.  I'll try to talk to Mark and see if he
has any ideas.

Meanwhile here are a few comments:

http://codereview.chromium.org/18446/diff/1/7
File chrome/common/process_watcher.cc (right):

http://codereview.chromium.org/18446/diff/1/7#newcode25
Line 25: timer_.Start(base::TimeDelta::FromMilliseconds(kWaitInterval), this,
I'd probably put |this| on a new line also

http://codereview.chromium.org/18446/diff/1/7#newcode56
Line 56: base::KillProcess(base::Process(process_).pid() , ResultCodes::HUNG,
HUNG on it's own line also

http://codereview.chromium.org/18446/diff/1/7#newcode66
Line 66: #endif
Hmm, it seems like there should be some CloseProcessHandle(ProcessHandle) that
does nothing on POSIX?  Dunno..

http://codereview.chromium.org/18446/diff/1/7#newcode84
Line 84: // If already exited, then we are done!
I realize this is an old comment, but it probably reads better as:

Check if the process has already exited.

[Paweł Hajdan Jr., 2009-01-22 18:22:24]

Fixed all of your comments.

[sky, 2009-01-22 20:42:07]

The FilePath stuff sure makes the code ugly. Is the plan to convert everything
over to filepath?

The bookmark/table related changes LGTM.

[Paweł Hajdan Jr., 2009-01-22 20:44:57]

On 2009/01/22 20:42:07, sky wrote:
> The FilePath stuff sure makes the code ugly. Is the plan to convert everything
> over to filepath?

Probably yes. I think the good plan would be to drop these bookmarks_storage.cc
changes, refactor the code and then port.

[sky, 2009-01-22 20:52:43]

I like that idea much better:)

[Mark Mentovai, 2009-01-22 21:58:51]

lgtm

http://codereview.chromium.org/18446/diff/46/228
File base/process_util.h (right):

http://codereview.chromium.org/18446/diff/46/228#newcode64
Line 64: bool CloseProcessHandle(ProcessHandle process);
Comment to describe what this is supposed to do.

http://codereview.chromium.org/18446/diff/46/235
File chrome/common/process_watcher.cc (right):

http://codereview.chromium.org/18446/diff/46/235#newcode10
Line 10: #include "base/process.h"
I woulda sorted process.h before process_util.h but whatever.