POSIX: Get IPCSyncChannel unittests working

POSIX: Get IPCSyncChannel unittests working

[agl, 2009-01-16 19:02:43]

This converts ipc sync channel unittests to use the cross-platform
WaitableEvent. After this (an another change which is currently out for review),
the tests all pass on Linux. (I'm still waiting for Windows to finish building
so that I can verify that they still pass there too, but I'll not submit till
that's done).

Cheers.

[jam, 2009-01-16 21:28:22]

very nice, some comments:

http://codereview.chromium.org/18324/diff/1/2
File chrome/common/child_process.cc (right):

http://codereview.chromium.org/18324/diff/1/2#newcode14
Line 14: #if defined(OS_WIN)
nit: I think you don't really need the ifdef here, since webkit_glue.h can be
included in all platforms..  I realize it's not used right now, but it looks
like it'll be used later when the TODO is done, so it'll cut down on ifdefs..

http://codereview.chromium.org/18324/diff/1/4
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/18324/diff/1/4#newcode7
Line 7: #include "base/debug_util.h"
why is this added?

http://codereview.chromium.org/18324/diff/1/4#newcode376
Line 376: dispatch_watcher_.StopWatching();
Why is this needed?

On Windows, ~ObjectWatcher will call this automatically.  It seems whatever
object is used on POSIX should do the same?

http://codereview.chromium.org/18324/diff/1/5
File chrome/common/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/18324/diff/1/5#newcode31
Line 31: static const int INFINITE = -1;
This also had to be defined in SyncChannel, should this be declared somewhere in
base, perhaps in waitable_event.h?

[agl, 2009-01-16 22:56:05]

(tested on Windows now, still passes)

http://codereview.chromium.org/18324/diff/1/2
File chrome/common/child_process.cc (right):

http://codereview.chromium.org/18324/diff/1/2#newcode14
Line 14: #if defined(OS_WIN)
On 2009/01/16 21:28:22, John Abd-El-Malek wrote:
> nit: I think you don't really need the ifdef here, since webkit_glue.h can be
> included in all platforms..  I realize it's not used right now, but it looks
> like it'll be used later when the TODO is done, so it'll cut down on ifdefs..

Yep, it works without the ifdef

http://codereview.chromium.org/18324/diff/1/4
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/18324/diff/1/4#newcode7
Line 7: #include "base/debug_util.h"
On 2009/01/16 21:28:22, John Abd-El-Malek wrote:
> why is this added?

I was using the debugging utils, but I should take it out. thanks

http://codereview.chromium.org/18324/diff/1/4#newcode376
Line 376: dispatch_watcher_.StopWatching();
On 2009/01/16 21:28:22, John Abd-El-Malek wrote:
> Why is this needed?
> 
> On Windows, ~ObjectWatcher will call this automatically.  It seems whatever
> object is used on POSIX should do the same?

You're right, this is vestigial from when I was debugging

http://codereview.chromium.org/18324/diff/1/5
File chrome/common/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/18324/diff/1/5#newcode31
Line 31: static const int INFINITE = -1;
On 2009/01/16 21:28:22, John Abd-El-Malek wrote:
> This also had to be defined in SyncChannel, should this be declared somewhere
in
> base, perhaps in waitable_event.h?

Made base::kNoTimeout and converted all the uses in chrome/common. I'll probably
run across INFINITE in other places and convert as I hit them.

[jam, 2009-01-16 23:28:07]

lgtm

http://codereview.chromium.org/18324/diff/1/5
File chrome/common/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/18324/diff/1/5#newcode31
Line 31: static const int INFINITE = -1;
On 2009/01/16 22:56:05, agl wrote:
> On 2009/01/16 21:28:22, John Abd-El-Malek wrote:
> > This also had to be defined in SyncChannel, should this be declared
somewhere
> in
> > base, perhaps in waitable_event.h?
> 
> Made base::kNoTimeout and converted all the uses in chrome/common. I'll
probably
> run across INFINITE in other places and convert as I hit them.

cool, although personally I think kInfinite is more readable than kNoTimeout.

[jam, 2009-01-17 02:12:37]

http://codereview.chromium.org/18324/diff/16/20
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/18324/diff/16/20#newcode390
Line 390: DCHECK(sync_messages_with_no_timeout_allowed_ || timeout_ms !=
base::kNoTimeout);
80 characters