Refactor: Simplify WaitableEventWatcher.

Refactor: Simplify WaitableEventWatcher.

This change uses a callback instead of a delegate for specifying what
should be called when a WaitableEvent occurs.

This simplifies the class and gets rid of a workaround internal to the
class to prevent name collision on "Delegate" inner classes.

Tested (on linux and windows):
  ninja -C out/Debug chrome
  out/Debug/base_unittests --gtest_filter=*WaitableEventWatcherTest*

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179987

[dmichael (off chromium), 2013-01-25 21:28:11]

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:81:
Callback<void(WaitableEvent*)>* callback);
const Callback<...>&?

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:104:
Callback<void(WaitableEvent*)>* callback() { return callback_; }
I would return by const&

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:108: void OnObjectSignaled(HANDLE
h);
mark virtual, and OVERRIDE

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:124:
Callback<void(WaitableEvent*)>* callback_;
This should probably be by value.

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher_posix.cc (right):

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher_posix.cc:127: (WaitableEvent* event,
Callback<void(WaitableEvent*)>* callback) {
nit: paren should be on the previous line.
extra tiny nit: I also prefer 1 param per line for declarations and definitions
if they don't fit on 1

https://codereview.chromium.org/11953112/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/11953112/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.h:371:
base::Callback<void(base::WaitableEvent*)> watcher_callback_;
IMO, The class shouldn't need to remember its callback

[tfarina, 2013-01-26 15:24:18]

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:81:
Callback<void(WaitableEvent*)>* callback);
a typedef would be helpful too. It would increase the readability.

[dmichael (off chromium), 2013-01-28 16:54:10]

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:81:
Callback<void(WaitableEvent*)>* callback);
On 2013/01/26 15:24:18, tfarina wrote:
> a typedef would be helpful too. It would increase the readability.
I personally prefer not using a typedef when it's short like this, so you don't
have to go look up the typedef to figure out what it is. I would leave it alone
and maybe ask about it during OWNERS review.

[teravest, 2013-01-28 17:10:55]

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:81:
Callback<void(WaitableEvent*)>* callback);
On 2013/01/28 16:54:11, dmichael wrote:
> On 2013/01/26 15:24:18, tfarina wrote:
> > a typedef would be helpful too. It would increase the readability.
> I personally prefer not using a typedef when it's short like this, so you
don't
> have to go look up the typedef to figure out what it is. I would leave it
alone
> and maybe ask about it during OWNERS review.

We'll see what brett has to say. I changed this to use a typedef, but I'm happy
to change it back.

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:104:
Callback<void(WaitableEvent*)>* callback() { return callback_; }
On 2013/01/25 21:28:11, dmichael wrote:
> I would return by const&

Done.

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:108: void OnObjectSignaled(HANDLE
h);
On 2013/01/25 21:28:11, dmichael wrote:
> mark virtual, and OVERRIDE

Done.

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:124:
Callback<void(WaitableEvent*)>* callback_;
On 2013/01/25 21:28:11, dmichael wrote:
> This should probably be by value.

Done.

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher_posix.cc (right):

https://codereview.chromium.org/11953112/diff/3001/base/synchronization/waita...
base/synchronization/waitable_event_watcher_posix.cc:127: (WaitableEvent* event,
Callback<void(WaitableEvent*)>* callback) {
On 2013/01/25 21:28:11, dmichael wrote:
> nit: paren should be on the previous line.
> extra tiny nit: I also prefer 1 param per line for declarations and
definitions
> if they don't fit on 1

Done.

https://codereview.chromium.org/11953112/diff/3001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/11953112/diff/3001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.h:371:
base::Callback<void(base::WaitableEvent*)> watcher_callback_;
On 2013/01/25 21:28:11, dmichael wrote:
> IMO, The class shouldn't need to remember its callback

Done.

[brettw, 2013-01-28 20:56:59]

lgtm

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:26: typedef
Callback<void(WaitableEvent*)> WaitableEventCallback;
I think the typedef is fine. I usually try to keep stuff out of the global (or
base in this case) namespace when possible, so in this case I'd probably do it
inside the class. Maybe we can come up with a shorter name ("EventCallback"?).

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:77: //
---------------------------------------------------------------------------
Whoever wrote this kind of went crazy with the horizontal lines. Would you mind
removing them while you're here? It doesn't really match our style.

[tfarina, 2013-01-28 21:06:10]

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:67: #if defined(OS_WIN)
for a possible future CL. Just an idea. We should try hard to avoid ifdefs. So
instead of ifdefing here, we could have a WaitableEventWatcherWin and
WaitableEventWatcherLinux and with gyp compile only the class necessary for each
platform.

[teravest, 2013-01-28 21:25:09]

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:26: typedef
Callback<void(WaitableEvent*)> WaitableEventCallback;
On 2013/01/28 20:56:59, brettw wrote:
> I think the typedef is fine. I usually try to keep stuff out of the global (or
> base in this case) namespace when possible, so in this case I'd probably do it
> inside the class. Maybe we can come up with a shorter name ("EventCallback"?).

Done.

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:77: //
---------------------------------------------------------------------------
On 2013/01/28 20:56:59, brettw wrote:
> Whoever wrote this kind of went crazy with the horizontal lines. Would you
mind
> removing them while you're here? It doesn't really match our style.

Done.

[dmichael (off chromium), 2013-01-28 21:47:27]

lgtm

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/11953112/diff/9001/base/synchronization/waita...
base/synchronization/waitable_event_watcher.h:67: #if defined(OS_WIN)
On 2013/01/28 21:06:10, tfarina wrote:
> for a possible future CL. Just an idea. We should try hard to avoid ifdefs. So
> instead of ifdefing here, we could have a WaitableEventWatcherWin and
> WaitableEventWatcherLinux and with gyp compile only the class necessary for
each
> platform.
That's what we have, but the header is shared. That seems to be the most common
way to do platform-specific classes, AFAICT. IPC::Channel gets around it by
using a pImpl class that's platform specific, so I could see doing it that way
in a future CL if it's important. But IMHO it's not worth the trouble here.

[commit-bot: I haz the power, 2013-01-28 22:13:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11953112/9003

[commit-bot: I haz the power, 2013-01-28 22:29:53]

Failed to trigger a try job on linux_clang
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-28 22:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11953112/6004

[commit-bot: I haz the power, 2013-01-29 00:08:52]

Retried try job too often on linux_rel for step(s) ipc_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-01-31 15:36:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11953112/6004

[commit-bot: I haz the power, 2013-01-31 16:25:48]

Retried try job too often on linux_rel for step(s) ipc_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[dmichael (off chromium), 2013-01-31 17:34:49]

I think you have a legitimate failure; you probably want to try ipc_tests
locally and see if you can reproduce.


On Thu, Jan 31, 2013 at 9:25 AM, <commit-bot@chromium.org> wrote:

> Retried try job too often on linux_rel for step(s) ipc_tests
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=linux_rel&**number=94704<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&number=94704>
>
>
https://chromiumcodereview.**appspot.com/11953112/<https://chromiumcodereview...
>

[teravest1, 2013-01-31 17:37:07]

Yeah, I spent some time this morning tracking it down. I was trying to
see if the windows one would go away with enough attempts, since I was
getting weird goma errors.

On Thu, Jan 31, 2013 at 10:34 AM, David Michael <dmichael@chromium.org> wrote:
> I think you have a legitimate failure; you probably want to try ipc_tests
> locally and see if you can reproduce.
>
>
> On Thu, Jan 31, 2013 at 9:25 AM, <commit-bot@chromium.org> wrote:
>>
>> Retried try job too often on linux_rel for step(s) ipc_tests
>>
>>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
>>
>> https://chromiumcodereview.appspot.com/11953112/
>
>

[commit-bot: I haz the power, 2013-01-31 17:44:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11953112/23007

[commit-bot: I haz the power, 2013-01-31 22:37:58]

Change committed as 179987