Make WaitableEventWatcher TaskScheduler-friendly.

Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG=650318
Committed: https://crrev.com/cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597
Cr-Commit-Position: refs/heads/master@{#422856}

[fdoray, 2016-09-26 19:01:01]

Description was changed from

==========
Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG=
==========

to

==========
Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG=650318
==========

[fdoray, 2016-09-26 19:01:08]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 19:02:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 19:39:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:39:04]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[fdoray, 2016-09-26 19:46:15]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 19:47:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 22:46:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 22:46:10]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fdoray, 2016-09-27 12:27:06]

fdoray@chromium.org changed reviewers:
+ dcheng@chromium.org

[fdoray, 2016-09-27 12:27:06]

PTAL

[fdoray, 2016-09-27 12:43:12]

The CQ bit was checked by fdoray@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 12:43:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 14:26:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 14:26:55]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-03 21:24:02]

Overall, looks good. Just a few minor nits.

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher.h:98: // Initialized in
StartWatching(). Set to true before the callback runs. Reset
Nit: "set to true" => maybe should be worded differently, since it's a smart
pointer to a Flag object, not a bool by itself?

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_watcher_posix.cc (left):

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher_posix.cc:128: MessageLoop *const
current_ml = MessageLoop::current();
Just to be sure: do you know if there's any efficiency reason to not call
STRH::Get() multiple times and caching the result in a stack pointer?

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_watcher_posix.cc (right):

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher_posix.cc:64: :
task_runner_(task_runner), callback_(callback), flag_(flag) {}
Nit: std::move

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher_posix.cc:136: const auto
internal_callback = base::Bind(
Nit: I think we can just write Closure here (to me, it's not so obvious what
type is from just looking at the RHS)

[fdoray, 2016-10-04 12:59:11]

PTAnL

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_watcher.h (right):

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher.h:98: // Initialized in
StartWatching(). Set to true before the callback runs. Reset
On 2016/10/03 21:24:01, dcheng wrote:
> Nit: "set to true" => maybe should be worded differently, since it's a smart
> pointer to a Flag object, not a bool by itself?

Done.

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_watcher_posix.cc (left):

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher_posix.cc:128: MessageLoop *const
current_ml = MessageLoop::current();
On 2016/10/03 21:24:02, dcheng wrote:
> Just to be sure: do you know if there's any efficiency reason to not call
> STRH::Get() multiple times and caching the result in a stack pointer?

Changed to DCHECK(SequencedTaskRunnerHandle::IsSet) since
SequencedTaskRunnerHandle::Get() never returns nullptr (DCHECKs if no
SequencedTaskRunnerHandle is set).

Speed of SequencedTaskRunnerHandle::Get/IsSet():

POSIX: Involves calls to pthread_getspecific which "has been designed to favor
speed and simplicity over error reporting"
http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_getspecific....

Windows: Involves calls to TlsGetValue which "was implemented with speed as the
primary goal"
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686812(v=vs.85).aspx

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
File base/synchronization/waitable_event_watcher_posix.cc (right):

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher_posix.cc:64: :
task_runner_(task_runner), callback_(callback), flag_(flag) {}
On 2016/10/03 21:24:02, dcheng wrote:
> Nit: std::move

Done.

https://codereview.chromium.org/2368423002/diff/40001/base/synchronization/wa...
base/synchronization/waitable_event_watcher_posix.cc:136: const auto
internal_callback = base::Bind(
On 2016/10/03 21:24:02, dcheng wrote:
> Nit: I think we can just write Closure here (to me, it's not so obvious what
> type is from just looking at the RHS)

Done.

[dcheng, 2016-10-04 16:39:11]

lgtm

[fdoray, 2016-10-04 17:07:22]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-10-04 17:07:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 18:04:15]

Description was changed from

==========
Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG=650318
==========

to

==========
Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG=650318
==========

[commit-bot: I haz the power, 2016-10-04 18:04:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-04 18:06:51]

Description was changed from

==========
Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG=650318
==========

to

==========
Make WaitableEventWatcher TaskScheduler-friendly.

With this CL, WaitableEventWatcher uses SequencedTaskRunnerHandle
instead of MessageLoop::current() to post back to the sequence that
called StartWatching().

Also, WaitableEventWatcher no longer registers itself as a destruction
observer of the MessageLoop from which StartWatching() is called. If
the watched WaitableEvent is signaled after the MessageLoop is
destroyed, the task posted by WaitableEventWatcher to the
SequencedTaskRunner will simply not run (no crash).

MessageLoop::current() and destruction observer do not work with
TaskScheduler.

BUG=650318

Committed: https://crrev.com/cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597
Cr-Commit-Position: refs/heads/master@{#422856}
==========

[commit-bot: I haz the power, 2016-10-04 18:06:52]

Patchset 5 (id:??) landed as
https://crrev.com/cc6bd5c938f5d4dfa33ed1bbe962a2effc12d597
Cr-Commit-Position: refs/heads/master@{#422856}