WaitableEvent

WaitableEvent is the replacement for Windows events. Previously in the code, a HANDLE from CreateEvent was used for signaling, both within a process and across processes.

WaitableEvent is the cross platform replacement for this. To convert:
  * HANDLE -> base::WaitableEvent*
  * ScopedHandle -> scoped_ptr<base::WaitableEvent>
  * CreateEvent -> new base::WaitableEvent
  * SetEvent -> base::WaitableEvent::Signal
  * ResetEvent -> base::WaitableEvent::Reset
  * ObjectWatcher -> base::WaitableEventWatcher
  * WaitForMultipleObjects -> static base::WaitableEvent::WaitMany

ObjectWatcher remains for Windows specific code. WaitableEventWatcher has an identical interface save,
  * It uses WaitableEvents, not HANDLEs
  * It returns void from StartWatching and StopWatcher, rather than errors. System internal errors are fatal to the address space

IMPORTANT: There are semantic differences between the different platforms. WaitableEvents on Windows are implemented on top of events. Windows events work across process and this is used mostly for modal dialog support. Windows events can be duplicated with DuplicateHandle.

On other platforms, WaitableEvent works only within a single process. In the future we shall have to replace the current uses of cross-process events with IPCs.

BEWARE: HANDLE, on Windows, is a void *. Since any pointer type coerces to void *, you can pass a WaitableEvent * where a HANDLE is expected without any build-time errors.

[agl, 2009-01-06 21:50:27]

I think POSIX support is working now. I still have to do the Windows support,
but I probably need to setup my Windows build for that.

[Evan Martin, 2009-01-13 23:11:37]

I skimmed.

http://codereview.chromium.org/16554/diff/210/214
File base/waitable_event.h (right):

http://codereview.chromium.org/16554/diff/210/214#newcode68
Line 68: WaitableEvent(HANDLE h);
explicit ?

http://codereview.chromium.org/16554/diff/210/214#newcode101
Line 101: // of the wait-list
typo: final period

[jam, 2009-01-13 23:22:03]

cool, some comments

http://codereview.chromium.org/16554/diff/210/245
File chrome/common/ipc_sync_channel_unittest.cc (right):

http://codereview.chromium.org/16554/diff/210/245#newcode16
Line 16: #include "base/waitable_event.h"
curious why this is needed?  the rest of the file isn't changed

http://codereview.chromium.org/16554/diff/210/252
File chrome/plugin/plugin_channel_base.cc (right):

http://codereview.chromium.org/16554/diff/210/252#newcode10
Line 10: #include "base/waitable_event.h"
curious why this is needed?  the rest of the file isn't changed

http://codereview.chromium.org/16554/diff/210/253
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/16554/diff/210/253#newcode10
Line 10: #include "base/waitable_event.h"
curious why this is needed?  the rest of the file isn't changed

http://codereview.chromium.org/16554/diff/210/254
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/16554/diff/210/254#newcode174
Line 174: scoped_ptr<base::WaitableEvent> event(new base::WaitableEvent(0, 1));
I think (false, true) is more readable than (0, 1)

[agl, 2009-01-14 02:20:29]

(Thanks to previous reviewers: all points addressed.)

darin: This building and running happily on Linux and Windows now.

[darin (slow to review), 2009-01-14 22:50:02]

overall, this looks great.  my comments are mostly just style nits.  i haven't
reviewed the posix implementations in depth.  i will do that in a second pass.

http://codereview.chromium.org/16554/diff/518/326
File base/waitable_event.h (right):

http://codereview.chromium.org/16554/diff/518/326#newcode28
Line 28: #if defined(OS_WIN)
looks like this section can be deleted

http://codereview.chromium.org/16554/diff/518/326#newcode32
Line 32: #if defined(OS_POSIX)
looks like this section can be deleted.

http://codereview.chromium.org/16554/diff/518/326#newcode61
Line 61: // initially_signaled is not supported on non-Windows platforms.
that's a problem, or can we remove this parameter altogether?  should you have a
TODO?  the existing implementation supports it.  it seems strange to not support
it since you could at least support it by calling Signal() at the end of the
constructor, right?

http://codereview.chromium.org/16554/diff/518/326#newcode68
Line 68: explicit WaitableEvent(HANDLE h);
nit: maybe rename h to event_handle or handle?  it is google style to spell out
variable names.

http://codereview.chromium.org/16554/diff/518/326#newcode95
Line 95: #if defined(OS_WIN)
do we really need to expose these things in the public section of the class? 
why do we need to expose the WaitableEvent's handle_ on windows?  why do we need
to expose this Waiter class on non-windows?  exposing lock() seems really
dangerous.

perhaps the answer for non-windows is to make WaitableEventWatcher be a friend. 
then Waiter and lock_ can be only exposed to WaitableEventWatcher.

http://codereview.chromium.org/16554/diff/518/326#newcode128
Line 128: virtual bool Compare(void* tag) = 0;
what is a tag?

http://codereview.chromium.org/16554/diff/518/326#newcode134
Line 134: //
---------------------------------------------------------------------------
when in rome...

please don't add these separator comments since the class doesn't have them. 
or, add them universally.

http://codereview.chromium.org/16554/diff/518/326#newcode141
Line 141: static unsigned WaitMany(WaitableEvent** waitables, unsigned count);
in chrome code, we generally just use "int" for usage like these.  unsigned
doesn't really save you from having to deal with values that "go negative", and
int is just fewer bytes to type, etc.  it seems good to keep our code consistent
about such things, so i'd probably change these to "int"

http://codereview.chromium.org/16554/diff/518/326#newcode156
Line 156: bool Dequeue(Waiter*, void*);
it would be good to give this void* parameter a name.  otherwise it is a bit
mysterious what this void* refers to.

http://codereview.chromium.org/16554/diff/518/326#newcode157
Line 157: static unsigned EnqueueMany(std::pair<WaitableEvent*, unsigned>*
waitables,
It is often a good idea to typedef a std::pair to help give the pairing a good
name.  For example, in this case, it is unclear what the unsigned parameter
means, but if the pair had a good name, it might be obvious what the unsigned
parameter means.

http://codereview.chromium.org/16554/diff/518/326#newcode163
Line 163: std::list<Waiter*> waiters_;
would it be a useful optimization to add a next pointer to a Waiter?  can these
just be held in a singly-linked list?

http://codereview.chromium.org/16554/diff/518/328
File base/waitable_event_posix.cc (right):

http://codereview.chromium.org/16554/diff/518/328#newcode48
Line 48: AutoLock l(lock_);
google style is to spell out variable names.  how about "locked" instead of "l"?

http://codereview.chromium.org/16554/diff/518/329
File base/waitable_event_unittest.cc (right):

http://codereview.chromium.org/16554/diff/518/329#newcode6
Line 6: #include "base/logging.h"
do you really need this header?

http://codereview.chromium.org/16554/diff/518/329#newcode8
Line 8: #include "base/waitable_event_watcher.h"
this header seems unnecessary

http://codereview.chromium.org/16554/diff/518/329#newcode14
Line 14: using base::WaitableEventWatcher;
this class is not used in this file.

http://codereview.chromium.org/16554/diff/518/329#newcode93
Line 93: const double seconds_;
nit: do these really need to be const?  seems gratuitous to me.

http://codereview.chromium.org/16554/diff/518/330
File base/waitable_event_watcher.h (right):

http://codereview.chromium.org/16554/diff/518/330#newcode28
Line 28: class WaitableEventWatcherDelegate {
please follow the pattern of ObjectWatcher and make this an inner class of
WaitableEventWatcher named Delegate.

http://codereview.chromium.org/16554/diff/518/330#newcode36
Line 36: virtual void OnEvent(WaitableEvent* waitable_event) = 0;
OnWaitableEventSignaled?  (verbose i know, but for classes that have a ton of
methods, it is nice to make these "forced" methods more verbose in name.)

you should probably document that the WaitableEvent may no longer be signaled by
the time this method is called.  the method is only called to indicate that it
was signaled at some point in the past.

http://codereview.chromium.org/16554/diff/518/330#newcode63
Line 63: // worry about OnObjectSignaled being called on a deleted MyClass
pointer.
typo: OnObjectSignaled

http://codereview.chromium.org/16554/diff/518/330#newcode73
Line 73: private ObjectWatcher::Delegate
nit: our convention is to indent by 4 spaces here

http://codereview.chromium.org/16554/diff/518/330#newcode77
Line 77: {
you should probably put the opening "{" at the end of each of the previous
lines.

http://codereview.chromium.org/16554/diff/518/330#newcode83
Line 83: //
---------------------------------------------------------------------------
micro-nit: the separator comments are usually reserved for grouping methods,
defining sections, or highlighting a comment.  it seems a bit verbose to use
them like this.

http://codereview.chromium.org/16554/diff/518/330#newcode84
Line 84: // When @event is signaled, the given delegate is called on the thread
of the
micro-nit: the "@event" notation isn't used too much in the chrome code.  we
usually just write "event"

http://codereview.chromium.org/16554/diff/518/330#newcode106
Line 106: WaitableEvent* GetWatchedObject();
GetWatchedEvent?  GetWatchedWaitableEvent?  do we need this method for anything?

http://codereview.chromium.org/16554/diff/518/330#newcode115
Line 115: void OnObjectSignaled(HANDLE handle);
nit: please prefix with virtual.  helps to document the fact that this is
virtual so that when reading the class you can see that there are virtual
functions.

http://codereview.chromium.org/16554/diff/518/330#newcode128
Line 128: AsyncCallbackTask* acbt_;
nit: please spell out the name of this variable

http://codereview.chromium.org/16554/diff/518/330#newcode132
Line 132: };  // namespace base
nit: no ';' at the close of a namespace

http://codereview.chromium.org/16554/diff/518/331
File base/waitable_event_watcher_posix.cc (right):

http://codereview.chromium.org/16554/diff/518/331#newcode35
Line 35: AutoLock l(lock_);
s/l/locked/ ?

http://codereview.chromium.org/16554/diff/518/331#newcode46
Line 46: bool flag_;
nit: indentation

http://codereview.chromium.org/16554/diff/518/331#newcode55
Line 55: AsyncWaiter(MessageLoop* ml, Task* task, Flag* flag)
nit: spell out variable names

http://codereview.chromium.org/16554/diff/518/332
File base/waitable_event_watcher_unittest.cc (right):

http://codereview.chromium.org/16554/diff/518/332#newcode98
Line 98: // Simulate a MessageLoop that dies before an ObjectWatcher.  This
ordinarily
typo: ObjectWatcher

http://codereview.chromium.org/16554/diff/518/333
File base/waitable_event_watcher_win.cc (right):

http://codereview.chromium.org/16554/diff/518/333#newcode23
Line 23: if (!watcher_.StartWatching(event->handle(), this))
perhaps your StartWatching function should also return a bool?  i would only
assign delegate_ and event_ if watcher_.StartWatching succeeds.

http://codereview.chromium.org/16554/diff/518/333#newcode38
Line 38: WaitableEvent *const event = event_;
nit: this const is gratuitous.  we generally avoid overuse of const since it can
hurt readability.

http://codereview.chromium.org/16554/diff/518/333#newcode39
Line 39: WaitableEventWatcherDelegate *const delegate = delegate_;
ditto

http://codereview.chromium.org/16554/diff/518/334
File base/waitable_event_win.cc (right):

http://codereview.chromium.org/16554/diff/518/334#newcode13
Line 13: #include "base/lock.h"
these seem like unnecessary includes

http://codereview.chromium.org/16554/diff/518/334#newcode74
Line 74: std::vector<HANDLE> handles;
you could also create a stack based HANDLE array of size MAXIMUM_WAIT_OBJECTS,
which is 64.  you could also CHECK that count does not exceed that.  this might
be slightly better than a heap vector.

http://codereview.chromium.org/16554/diff/518/334#newcode80
Line 80: const DWORD r =
nit: const is unnecessary here.  it just adds noise.  r should be spelled out
per google style.  how about result or rv ("rv" is allowed because it is a
common abbrev for "return value")?

http://codereview.chromium.org/16554/diff/518/337
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/16554/diff/518/337#newcode245
Line 245: base::WaitableEvent* shutdown_event_;
should this be a scoped_ptr?

http://codereview.chromium.org/16554/diff/518/339
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/16554/diff/518/339#newcode138
Line 138: modal_dialog_event_->handle(),
ah, ok... this is why you need the handle() getter.

http://codereview.chromium.org/16554/diff/518/339#newcode758
Line 758: new base::WaitableEvent(modal_dialog_event));
please make sure that CreateNewWindow always takes ownership over this newly
allocated WaitableEvent object.  we might need to worry about memory leaks in
error cases.  the documentation for CreateNewWindow should also specify that it
takes ownership over the given WaitableEvent object.

http://codereview.chromium.org/16554/diff/518/341
File chrome/browser/render_view_host_delegate.h (right):

http://codereview.chromium.org/16554/diff/518/341#newcode64
Line 64: base::WaitableEvent* modal_dialog_event) = 0;
i wish we had a better way to pass these objects around.  methods that transfer
object ownership like this are kind of errorprone.  i can't think of a good
solution...

http://codereview.chromium.org/16554/diff/518/355
File chrome/common/ipc_message.h (right):

http://codereview.chromium.org/16554/diff/518/355#newcode15
Line 15: // IPC message logging hasn't been ported to other platforms yet.
TODO(port)?

http://codereview.chromium.org/16554/diff/518/356
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/16554/diff/518/356#newcode16
Line 16: #if defined(OS_POSIX)
#if !defined(OS_WIN) might be more accurate

http://codereview.chromium.org/16554/diff/518/356#newcode173
Line 173: dispatch_event_(new WaitableEvent(true, false)),
why is dispatch_event_ allocated separately?  can't it be allocated inline as a
member var?

http://codereview.chromium.org/16554/diff/518/358
File chrome/common/ipc_sync_message.cc (right):

http://codereview.chromium.org/16554/diff/518/358#newcode14
Line 14: #include "base/logging.h"
nit: please sort includes

http://codereview.chromium.org/16554/diff/518/358#newcode22
Line 22: // TODO(playmobil): reinstantiate once ObjectWatcher is ported.
maybe this code can be enabled now?

http://codereview.chromium.org/16554/diff/518/358#newcode53
Line 53: // TODO(playmobil): reinstantiate once ObjectWatcher is ported.
maybe this code can be enabled now?

http://codereview.chromium.org/16554/diff/518/359
File chrome/common/ipc_sync_message.h (right):

http://codereview.chromium.org/16554/diff/518/359#newcode16
Line 16: class WaitableEvent;
nit: do not indent this

http://codereview.chromium.org/16554/diff/518/359#newcode17
Line 17: };
nit: no ';' at the close of a namespace

http://codereview.chromium.org/16554/diff/518/359#newcode36
Line 36: // If this handle is passed in, then window messages will start being
pumped
need to fix this comment as it refers to "handle"

http://codereview.chromium.org/16554/diff/518/361
File chrome/plugin/npobject_proxy.h (right):

http://codereview.chromium.org/16554/diff/518/361#newcode20
Line 20: class WaitableEvent;
nit: no indent, no ';' at close of namespace

http://codereview.chromium.org/16554/diff/518/361#newcode39
Line 39: base::WaitableEvent* modal_dialog_event);
nit: please add a comment about the fact that modal_dialog_event is expected to
remain valid for the lifetime of the NPObjectProxy.

btw, please verify that modal_dialog_event is not leaked in this case.

http://codereview.chromium.org/16554/diff/518/364
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/16554/diff/518/364#newcode174
Line 174: scoped_ptr<base::WaitableEvent> event
nit: chrome convention is to put the "(" on the first line, so you end up with:

  scoped_ptr<base::WaitableEvent> event(
      new base::WaitableEvent(false, true));

http://codereview.chromium.org/16554/diff/518/366
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/16554/diff/518/366#newcode202
Line 202: new base::WaitableEvent(modal_dialog_event);
nit: indent the second line by 4 spaces instead

http://codereview.chromium.org/16554/diff/518/367
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/16554/diff/518/367#newcode1806
Line 1806: base::WaitableEvent* waitable_event = new
nit: more common to put the "new" on the next line

http://codereview.chromium.org/16554/diff/518/369
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/16554/diff/518/369#newcode590
Line 590: modal_loop_pump_messages_event_.reset(0);
nit: just do reset() without the 0, or pass NULL

http://codereview.chromium.org/16554/diff/518/370
File chrome/renderer/webplugin_delegate_proxy.h (right):

http://codereview.chromium.org/16554/diff/518/370#newcode30
Line 30: namespace base {
micro-nit: put the namespace decls in alphabetical order

http://codereview.chromium.org/16554/diff/518/371
File chrome/test/testing_browser_process.h (right):

http://codereview.chromium.org/16554/diff/518/371#newcode26
Line 26: shutdown_event_ = new base::WaitableEvent(true, false);
hmm... to keep purify happy, we should probably make sure that shutdown_event_
actually gets destroyed.  maybe it should be a scoped_ptr.

[agl, 2009-01-15 00:29:16]

http://codereview.chromium.org/16554/diff/518/326
File base/waitable_event.h (right):

http://codereview.chromium.org/16554/diff/518/326#newcode28
Line 28: #if defined(OS_WIN)
On 2009/01/14 22:50:02, darin wrote:
> looks like this section can be deleted

Done.

http://codereview.chromium.org/16554/diff/518/326#newcode32
Line 32: #if defined(OS_POSIX)
On 2009/01/14 22:50:02, darin wrote:
> looks like this section can be deleted.

Done.

http://codereview.chromium.org/16554/diff/518/326#newcode61
Line 61: // initially_signaled is not supported on non-Windows platforms.
On 2009/01/14 22:50:02, darin wrote:
> that's a problem, or can we remove this parameter altogether?

Have removed comment. Support will be in the next set of POSIX patches.

http://codereview.chromium.org/16554/diff/518/326#newcode68
Line 68: explicit WaitableEvent(HANDLE h);
On 2009/01/14 22:50:02, darin wrote:
> nit: maybe rename h to event_handle or handle?  it is google style to spell
out
> variable names.

Done.

http://codereview.chromium.org/16554/diff/518/326#newcode95
Line 95: #if defined(OS_WIN)
On 2009/01/14 22:50:02, darin wrote:
> do we really need to expose these things in the public section of the class?

The Waiter class can be private - I have made it so.

> why do we need to expose the WaitableEvent's handle_ on windows?

Code which is at the interface between process needs handle() in order to
DuplicateHandle a WaitableEvent to another process. At some point, as discussed,
this might be hidden inside an IPC class, which could then be a friend. However,
for now we still need it.


> [why] expose this Waiter class on non-windows?

On non-Windows the WaitableEventWatcher needs Waiter. However, WEWatcher is a
friend, so I've moved that into the private section.

>  exposing lock() seems really
> dangerous.

A leftover - removed. Cheers.

http://codereview.chromium.org/16554/diff/518/326#newcode128
Line 128: virtual bool Compare(void* tag) = 0;
On 2009/01/14 22:50:02, darin wrote:
> what is a tag?

The comment points to ~Handle for an explanation. Given that it's a fairly long,
and implementation specific, quirk I didn't want to duplicate the comment here.

http://codereview.chromium.org/16554/diff/518/326#newcode134
Line 134: //
---------------------------------------------------------------------------
On 2009/01/14 22:50:02, darin wrote:
> when in rome...
> 
> please don't add these separator comments since the class doesn't have them. 
> or, add them universally.

Done.

http://codereview.chromium.org/16554/diff/518/326#newcode141
Line 141: static unsigned WaitMany(WaitableEvent** waitables, unsigned count);
On 2009/01/14 22:50:02, darin wrote:
> in chrome code, we generally just use "int" for usage like these.  unsigned
> doesn't really save you from having to deal with values that "go negative",
and
> int is just fewer bytes to type, etc.  it seems good to keep our code
consistent
> about such things, so i'd probably change these to "int"

This is one thing I'll push back on. For values which cannot be negative (like
the length of an array), I much prefer unsigned because I understand them. When
using int's I have to keep in mind that they "cannot" overflow (C++ is undefined
if they do).

http://codereview.chromium.org/16554/diff/518/326#newcode157
Line 157: static unsigned EnqueueMany(std::pair<WaitableEvent*, unsigned>*
waitables,
On 2009/01/14 22:50:02, darin wrote:
> It is often a good idea to typedef a std::pair to help give the pairing a good
> name.  For example, in this case, it is unclear what the unsigned parameter
> means, but if the pair had a good name, it might be obvious what the unsigned
> parameter means.

Done.

http://codereview.chromium.org/16554/diff/518/326#newcode163
Line 163: std::list<Waiter*> waiters_;
On 2009/01/14 22:50:02, darin wrote:
> would it be a useful optimization to add a next pointer to a Waiter?  can
these
> just be held in a singly-linked list?

Yes, they can. I didn't bother doing that because I didn't think the waitlists
would be very long. If you feel it's worthwhile I'll do it in a later CL.

http://codereview.chromium.org/16554/diff/518/328
File base/waitable_event_posix.cc (right):

http://codereview.chromium.org/16554/diff/518/328#newcode48
Line 48: AutoLock l(lock_);
On 2009/01/14 22:50:02, darin wrote:
> google style is to spell out variable names.  how about "locked" instead of
"l"?

Google style for this is exactly to call the variable "l" (see
http://www/~m3b/mutex.html, search for "MutexLock"). I changed it anyway.

http://codereview.chromium.org/16554/diff/518/329
File base/waitable_event_unittest.cc (right):

http://codereview.chromium.org/16554/diff/518/329#newcode6
Line 6: #include "base/logging.h"
On 2009/01/14 22:50:02, darin wrote:
> do you really need this header?

Done.

http://codereview.chromium.org/16554/diff/518/329#newcode8
Line 8: #include "base/waitable_event_watcher.h"
On 2009/01/14 22:50:02, darin wrote:
> this header seems unnecessary

Thanks. A leftover from when WEWatcher was tested in this file.

http://codereview.chromium.org/16554/diff/518/329#newcode14
Line 14: using base::WaitableEventWatcher;
On 2009/01/14 22:50:02, darin wrote:
> this class is not used in this file.

Thanks. A leftover from when WEWatcher was tested in this file.

http://codereview.chromium.org/16554/diff/518/329#newcode93
Line 93: const double seconds_;
On 2009/01/14 22:50:02, darin wrote:
> nit: do these really need to be const?  seems gratuitous to me.

Everything should be const if possible!

http://codereview.chromium.org/16554/diff/518/330
File base/waitable_event_watcher.h (right):

http://codereview.chromium.org/16554/diff/518/330#newcode28
Line 28: class WaitableEventWatcherDelegate {
On 2009/01/14 22:50:02, darin wrote:
> please follow the pattern of ObjectWatcher and make this an inner class of
> WaitableEventWatcher named Delegate.

Since WaitableEventWatcher inherits from ObjectWatcher::Delegate, we cannot have
a member class called 'Delegate'.

http://codereview.chromium.org/16554/diff/518/330#newcode36
Line 36: virtual void OnEvent(WaitableEvent* waitable_event) = 0;
On 2009/01/14 22:50:02, darin wrote:
> OnWaitableEventSignaled?  (verbose i know, but for classes that have a ton of
> methods, it is nice to make these "forced" methods more verbose in name.)
> 
> you should probably document that the WaitableEvent may no longer be signaled
by
> the time this method is called.  the method is only called to indicate that it
> was signaled at some point in the past.

Done.

http://codereview.chromium.org/16554/diff/518/330#newcode63
Line 63: // worry about OnObjectSignaled being called on a deleted MyClass
pointer.
On 2009/01/14 22:50:02, darin wrote:
> typo: OnObjectSignaled

Done.

http://codereview.chromium.org/16554/diff/518/330#newcode73
Line 73: private ObjectWatcher::Delegate
On 2009/01/14 22:50:02, darin wrote:
> nit: our convention is to indent by 4 spaces here

Done.

http://codereview.chromium.org/16554/diff/518/330#newcode77
Line 77: {
On 2009/01/14 22:50:02, darin wrote:
> you should probably put the opening "{" at the end of each of the previous
> lines.

Done.

http://codereview.chromium.org/16554/diff/518/330#newcode106
Line 106: WaitableEvent* GetWatchedObject();
On 2009/01/14 22:50:02, darin wrote:
> GetWatchedEvent?  GetWatchedWaitableEvent?  do we need this method for
anything?

It's part of the ObjectWatcher interface, which I'm currently duplicating
exactly. It is used. Should I rename it?

http://codereview.chromium.org/16554/diff/518/330#newcode115
Line 115: void OnObjectSignaled(HANDLE handle);
On 2009/01/14 22:50:02, darin wrote:
> nit: please prefix with virtual.  helps to document the fact that this is
> virtual so that when reading the class you can see that there are virtual
> functions.

Done.

http://codereview.chromium.org/16554/diff/518/330#newcode128
Line 128: AsyncCallbackTask* acbt_;
On 2009/01/14 22:50:02, darin wrote:
> nit: please spell out the name of this variable

Done.

http://codereview.chromium.org/16554/diff/518/330#newcode132
Line 132: };  // namespace base
On 2009/01/14 22:50:02, darin wrote:
> nit: no ';' at the close of a namespace

Done.

http://codereview.chromium.org/16554/diff/518/331
File base/waitable_event_watcher_posix.cc (right):

http://codereview.chromium.org/16554/diff/518/331#newcode35
Line 35: AutoLock l(lock_);
On 2009/01/14 22:50:02, darin wrote:
> s/l/locked/ ?

Done.

http://codereview.chromium.org/16554/diff/518/331#newcode46
Line 46: bool flag_;
On 2009/01/14 22:50:02, darin wrote:
> nit: indentation

indentation is correct: flag_ is protected by lock_;

http://codereview.chromium.org/16554/diff/518/331#newcode55
Line 55: AsyncWaiter(MessageLoop* ml, Task* task, Flag* flag)
On 2009/01/14 22:50:02, darin wrote:
> nit: spell out variable names

Done.

http://codereview.chromium.org/16554/diff/518/332
File base/waitable_event_watcher_unittest.cc (right):

http://codereview.chromium.org/16554/diff/518/332#newcode98
Line 98: // Simulate a MessageLoop that dies before an ObjectWatcher.  This
ordinarily
On 2009/01/14 22:50:02, darin wrote:
> typo: ObjectWatcher

Done.

http://codereview.chromium.org/16554/diff/518/333
File base/waitable_event_watcher_win.cc (right):

http://codereview.chromium.org/16554/diff/518/333#newcode23
Line 23: if (!watcher_.StartWatching(event->handle(), this))
On 2009/01/14 22:50:02, darin wrote:
> perhaps your StartWatching function should also return a bool?  i would only
> assign delegate_ and event_ if watcher_.StartWatching succeeds.

ObjectWatcher::StartWatching cannot actually fail. All paths which return false
have a NOTREACHED in them. I thus made it return void to stop people needlessly
checking and dealing with cases which can't occur. I'll change this if you feel
strongly.

http://codereview.chromium.org/16554/diff/518/333#newcode38
Line 38: WaitableEvent *const event = event_;
On 2009/01/14 22:50:02, darin wrote:
> nit: this const is gratuitous.  we generally avoid overuse of const since it
can
> hurt readability.

Done.

http://codereview.chromium.org/16554/diff/518/333#newcode39
Line 39: WaitableEventWatcherDelegate *const delegate = delegate_;
On 2009/01/14 22:50:02, darin wrote:
> ditto

Done.

http://codereview.chromium.org/16554/diff/518/334
File base/waitable_event_win.cc (right):

http://codereview.chromium.org/16554/diff/518/334#newcode13
Line 13: #include "base/lock.h"
On 2009/01/14 22:50:02, darin wrote:
> these seem like unnecessary includes

Yep, thanks.

http://codereview.chromium.org/16554/diff/518/334#newcode74
Line 74: std::vector<HANDLE> handles;
On 2009/01/14 22:50:02, darin wrote:
> you could also create a stack based HANDLE array of size MAXIMUM_WAIT_OBJECTS,
> which is 64.  you could also CHECK that count does not exceed that.  this
might
> be slightly better than a heap vector.

Agreed. Switched to a stack allocated array of HANDLE.

http://codereview.chromium.org/16554/diff/518/334#newcode80
Line 80: const DWORD r =
On 2009/01/14 22:50:02, darin wrote:
> nit: const is unnecessary here.  it just adds noise.  r should be spelled out
> per google style.  how about result or rv ("rv" is allowed because it is a
> common abbrev for "return value")?

Done.

http://codereview.chromium.org/16554/diff/518/337
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/16554/diff/518/337#newcode245
Line 245: base::WaitableEvent* shutdown_event_;
On 2009/01/14 22:50:02, darin wrote:
> should this be a scoped_ptr?

I don't think so. Previously it was a HANDLE, this destroying the object doesn't
close the HANDLE. A scoped_ptr would destroy the WaitableEvent and close the
HANDLE.

(In general, ScopedHandle maps to scoped_ptr)

http://codereview.chromium.org/16554/diff/518/339
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/16554/diff/518/339#newcode758
Line 758: new base::WaitableEvent(modal_dialog_event));
On 2009/01/14 22:50:02, darin wrote:
> please make sure that CreateNewWindow always takes ownership over this newly
> allocated WaitableEvent object.  we might need to worry about memory leaks in
> error cases.  the documentation for CreateNewWindow should also specify that
it
> takes ownership over the given WaitableEvent object.
> 

Have updated the documentation for CreateNewWindow. After tracing the data flow,
the WaitableEvent ends up in a scoped_ptr in RenderViewHost::modal_dialog_event_
and so will be free when the RenderViewHost is.

http://codereview.chromium.org/16554/diff/518/355
File chrome/common/ipc_message.h (right):

http://codereview.chromium.org/16554/diff/518/355#newcode15
Line 15: // IPC message logging hasn't been ported to other platforms yet.
On 2009/01/14 22:50:02, darin wrote:
> TODO(port)?

Done.

http://codereview.chromium.org/16554/diff/518/356
File chrome/common/ipc_sync_channel.cc (right):

http://codereview.chromium.org/16554/diff/518/356#newcode16
Line 16: #if defined(OS_POSIX)
On 2009/01/14 22:50:02, darin wrote:
> #if !defined(OS_WIN) might be more accurate

Done.

http://codereview.chromium.org/16554/diff/518/356#newcode173
Line 173: dispatch_event_(new WaitableEvent(true, false)),
On 2009/01/14 22:50:02, darin wrote:
> why is dispatch_event_ allocated separately?  can't it be allocated inline as
a
> member var?

Yes it can. Changed.

http://codereview.chromium.org/16554/diff/518/358
File chrome/common/ipc_sync_message.cc (right):

http://codereview.chromium.org/16554/diff/518/358#newcode14
Line 14: #include "base/logging.h"
On 2009/01/14 22:50:02, darin wrote:
> nit: please sort includes

Done.

http://codereview.chromium.org/16554/diff/518/358#newcode22
Line 22: // TODO(playmobil): reinstantiate once ObjectWatcher is ported.
On 2009/01/14 22:50:02, darin wrote:
> maybe this code can be enabled now?

It's enabled in the next CL.

http://codereview.chromium.org/16554/diff/518/358#newcode53
Line 53: // TODO(playmobil): reinstantiate once ObjectWatcher is ported.
On 2009/01/14 22:50:02, darin wrote:
> maybe this code can be enabled now?

It's enabled in the next CL.

http://codereview.chromium.org/16554/diff/518/359
File chrome/common/ipc_sync_message.h (right):

http://codereview.chromium.org/16554/diff/518/359#newcode16
Line 16: class WaitableEvent;
On 2009/01/14 22:50:02, darin wrote:
> nit: do not indent this

Done.

http://codereview.chromium.org/16554/diff/518/359#newcode17
Line 17: };
On 2009/01/14 22:50:02, darin wrote:
> nit: no ';' at the close of a namespace

Done.

http://codereview.chromium.org/16554/diff/518/359#newcode36
Line 36: // If this handle is passed in, then window messages will start being
pumped
On 2009/01/14 22:50:02, darin wrote:
> need to fix this comment as it refers to "handle"

Done.

http://codereview.chromium.org/16554/diff/518/361
File chrome/plugin/npobject_proxy.h (right):

http://codereview.chromium.org/16554/diff/518/361#newcode20
Line 20: class WaitableEvent;
On 2009/01/14 22:50:02, darin wrote:
> nit: no indent, no ';' at close of namespace

Done.

http://codereview.chromium.org/16554/diff/518/361#newcode39
Line 39: base::WaitableEvent* modal_dialog_event);
On 2009/01/14 22:50:02, darin wrote:
> nit: please add a comment about the fact that modal_dialog_event is expected
to
> remain valid for the lifetime of the NPObjectProxy.

Done.

> btw, please verify that modal_dialog_event is not leaked in this case.

NPObjectProxy doesn't own the event. From tracing the callers, I think we're
good. At the very least, if we weren't leaking HANDLEs before, we shouldn't be
leaking WaitableEvents now.

http://codereview.chromium.org/16554/diff/518/364
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/16554/diff/518/364#newcode174
Line 174: scoped_ptr<base::WaitableEvent> event
On 2009/01/14 22:50:02, darin wrote:
> nit: chrome convention is to put the "(" on the first line, so you end up
with:
> 
>   scoped_ptr<base::WaitableEvent> event(
>       new base::WaitableEvent(false, true));

Done.

http://codereview.chromium.org/16554/diff/518/366
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/16554/diff/518/366#newcode202
Line 202: new base::WaitableEvent(modal_dialog_event);
On 2009/01/14 22:50:02, darin wrote:
> nit: indent the second line by 4 spaces instead

Sorry, VC inserted a tab here.

http://codereview.chromium.org/16554/diff/518/367
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/16554/diff/518/367#newcode1806
Line 1806: base::WaitableEvent* waitable_event = new
On 2009/01/14 22:50:02, darin wrote:
> nit: more common to put the "new" on the next line

Done.

http://codereview.chromium.org/16554/diff/518/369
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/16554/diff/518/369#newcode590
Line 590: modal_loop_pump_messages_event_.reset(0);
On 2009/01/14 22:50:02, darin wrote:
> nit: just do reset() without the 0, or pass NULL

Done.

http://codereview.chromium.org/16554/diff/518/370
File chrome/renderer/webplugin_delegate_proxy.h (right):

http://codereview.chromium.org/16554/diff/518/370#newcode30
Line 30: namespace base {
On 2009/01/14 22:50:02, darin wrote:
> micro-nit: put the namespace decls in alphabetical order

Done.

http://codereview.chromium.org/16554/diff/518/371
File chrome/test/testing_browser_process.h (right):

http://codereview.chromium.org/16554/diff/518/371#newcode26
Line 26: shutdown_event_ = new base::WaitableEvent(true, false);
On 2009/01/14 22:50:02, darin wrote:
> hmm... to keep purify happy, we should probably make sure that shutdown_event_
> actually gets destroyed.  maybe it should be a scoped_ptr.

Done.

[darin (slow to review), 2009-01-15 06:51:48]

http://codereview.chromium.org/16554/diff/518/326
File base/waitable_event.h (right):

http://codereview.chromium.org/16554/diff/518/326#newcode141
Line 141: static unsigned WaitMany(WaitableEvent** waitables, unsigned count);
I totally get that point-of-view.  I used to push for unsigned whenever possible
too, but in my experience (see the mozilla code base), the end result is all too
often a crap-load of int<->unsigned casts all over the code.  Some things are
int, some things are unsigned, and the meeting of the two is ugly.

Anyhow, since "count" is basically a parameter that would be satisfied by sizeof
or arraysize, perhaps size_t would be slightly better than unsigned.  Then, it
would make sense to have WaitMany return size_t.  That said, what should
WaitMany return to indicate an error?  size_t(-1) perhaps?

http://codereview.chromium.org/16554/diff/518/326#newcode163
Line 163: std::list<Waiter*> waiters_;
Up to you.

http://codereview.chromium.org/16554/diff/518/328
File base/waitable_event_posix.cc (right):

http://codereview.chromium.org/16554/diff/518/328#newcode48
Line 48: AutoLock l(lock_);
Interesting.  I guess that is regarded as an abbreviation that is "extremely
well known" within google3 or something.  Sigh... shows my lack of experience
with google3 I guess.

http://codereview.chromium.org/16554/diff/518/330
File base/waitable_event_watcher.h (right):

http://codereview.chromium.org/16554/diff/518/330#newcode28
Line 28: class WaitableEventWatcherDelegate {
I see.  It seems unfortunate that the "ideal" interface has to be ditched due to
such an implementation detail on one platform.

A simple solution is to create a private inner class that holds delegate_ and
event_.  Then, that class can implement ObjectWatcher::Delegate.  Such a change
would also kill off the #if defined(OS_WIN) at the top of the class decl.

It may make sense to employ a similar technique for the OS_POSIX case.

http://codereview.chromium.org/16554/diff/518/330#newcode106
Line 106: WaitableEvent* GetWatchedObject();
The name "GetWatchedObject" makes sense for the ObjectWatcher, which watches
objects.  Here, since we are watching WaitableEvents, we probably want a name
that corresponds to that.

http://codereview.chromium.org/16554/diff/518/331
File base/waitable_event_watcher_posix.cc (right):

http://codereview.chromium.org/16554/diff/518/331#newcode46
Line 46: bool flag_;
wow.  i'm not familiar with that style rule.  i don't see it in the style guide,
but maybe i'm not looking in the right place.  in any case, that isn't a
convention we've been using in chrome code, so it looks like a typo.

it seems like a useful documentation technique, but i suspect it would also
appear jarring to most folks on the chrome team.  they would probably be tempted
to "correct" it.

http://codereview.chromium.org/16554/diff/518/333
File base/waitable_event_watcher_win.cc (right):

http://codereview.chromium.org/16554/diff/518/333#newcode23
Line 23: if (!watcher_.StartWatching(event->handle(), this))
those NOTREACHED lines should probably have been DCHECK(false) instead.  they
were only meant to alert developers in case such conditions did happen.  it is
possible for RegisterWaitForSingleObject to fail under some situations according
to MSDN, and some developers may be interested in those cases.  since we're
dealing with fairly low level helper classes, it might be best to propagate
errors.

http://codereview.chromium.org/16554/diff/518/337
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/16554/diff/518/337#newcode245
Line 245: base::WaitableEvent* shutdown_event_;
OK

[agl, 2009-01-15 19:42:18]

http://codereview.chromium.org/16554/diff/518/326
File base/waitable_event.h (right):

http://codereview.chromium.org/16554/diff/518/326#newcode95
Line 95: #if defined(OS_WIN)
On 2009/01/15 00:29:16, agl wrote:
> The Waiter class can be private - I have made it so.

I take it back, it has to be public. Friendship isn't sufficient to allow a
class to inherit from a private class.

http://codereview.chromium.org/16554/diff/518/326#newcode141
Line 141: static unsigned WaitMany(WaitableEvent** waitables, unsigned count);
On 2009/01/15 06:51:49, darin wrote:
> Anyhow, since "count" is basically a parameter that would be satisfied by
sizeof
> or arraysize, perhaps size_t would be slightly better than unsigned.

size_t it is.

http://codereview.chromium.org/16554/diff/518/330
File base/waitable_event_watcher.h (right):

http://codereview.chromium.org/16554/diff/518/330#newcode28
Line 28: class WaitableEventWatcherDelegate {
On 2009/01/15 06:51:49, darin wrote:
> A simple solution is to create a private inner class that holds delegate_ and
> event_.  Then, that class can implement ObjectWatcher::Delegate.  Such a
change
> would also kill off the #if defined(OS_WIN) at the top of the class decl.

Done

http://codereview.chromium.org/16554/diff/518/330#newcode106
Line 106: WaitableEvent* GetWatchedObject();
On 2009/01/15 06:51:49, darin wrote:
> The name "GetWatchedObject" makes sense for the ObjectWatcher, which watches
> objects.  Here, since we are watching WaitableEvents, we probably want a name
> that corresponds to that.

Done.

http://codereview.chromium.org/16554/diff/518/331
File base/waitable_event_watcher_posix.cc (right):

http://codereview.chromium.org/16554/diff/518/331#newcode46
Line 46: bool flag_;
On 2009/01/15 06:51:49, darin wrote:
> in any case, that isn't a
> convention we've been using in chrome code, so it looks like a typo.

Indentation changed.

http://codereview.chromium.org/16554/diff/518/333
File base/waitable_event_watcher_win.cc (right):

http://codereview.chromium.org/16554/diff/518/333#newcode23
Line 23: if (!watcher_.StartWatching(event->handle(), this))
On 2009/01/15 06:51:49, darin wrote:
> those NOTREACHED lines should probably have been DCHECK(false) instead.

Return values made bools and errors propagated.

[darin (slow to review), 2009-01-15 22:46:34]

On 2009/01/15 19:42:18, agl wrote:
> http://codereview.chromium.org/16554/diff/518/326
> File base/waitable_event.h (right):
> 
> http://codereview.chromium.org/16554/diff/518/326#newcode95
> Line 95: #if defined(OS_WIN)
> On 2009/01/15 00:29:16, agl wrote:
> > The Waiter class can be private - I have made it so.
> 
> I take it back, it has to be public. Friendship isn't sufficient to allow a
> class to inherit from a private class.

Ah, right.  OK.

LGTM for everything except the actual posix implementations since I haven't
reviewed them yet.