Remove MessageLoop::current() from base::win::ObjectWatcher.

Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

TBR=ben@chromium.org
BUG=616447
Committed: https://crrev.com/76db1513714741d8d6e6dd6e872d50f20b090fce
Cr-Commit-Position: refs/heads/master@{#407874}

[fdoray, 2016-07-07 14:47:07]

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

[commit-bot: I haz the power, 2016-07-07 14:47:21]

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

[fdoray, 2016-07-07 14:50:11]

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

[fdoray, 2016-07-07 14:50:12]

Can you review this CL? Thanks.

[fdoray, 2016-07-07 17:04:48]

fdoray@chromium.org changed reviewers:
+ cpu@chromium.org
- jschuh@chromium.org

[fdoray, 2016-07-07 17:04:49]

cpu@: Can you review this CL? (jschuh@ very slow)

[commit-bot: I haz the power, 2016-07-07 17:25:35]

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

[commit-bot: I haz the power, 2016-07-07 17:25:36]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-07-14 07:19:10]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2016-07-14 07:19:11]

mostly good to me.

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher.cc
File base/win/object_watcher.cc (right):

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:46: object_ = NULL;
nit: nullptr throughout

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:47: wait_object_ = NULL;
take absolutely everything back to its initial state here to be sure that
dangling refs aren't held (the task runner is the only culprit, but it's good to
be thorough). for style points, retain the ordering in the class decl so that
it's easy to audit them. may as well make a helper function to do this that can
be called from StartWatchingInternal's failure path:
  callback_.Reset();
  object_ = nullptr;
  wait_object_ = nullptr;
  task_runner_.reset();
  run_once_ = true;
  weak_factory_.InvalidateWeakPtrs();

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:74: CHECK(delegate);
seems to me that this should be DCHECK since it represents a programmer error.
wdyt?

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:80: if (!ThreadTaskRunnerHandle::IsSet())
NOTREACHED for this, too? isn't it a programming error as well? does it make
sense for this to be a DHCECK(ThreadTaskRunnerHandle::IsSet()); at the top of
the function and no "return false;", since gracefully handling programmer error
is counter to coding style?

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher.h
File base/win/object_watcher.h (right):

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.h:103: Closure callback_;
  // A callback pre-bound to Signal() that is posted to the caller's task runner
when the wait completes.

[fdoray, 2016-07-18 16:05:18]

PTAnL

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher.cc
File base/win/object_watcher.cc (right):

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:46: object_ = NULL;
On 2016/07/14 07:19:11, grt (UTC plus 2) wrote:
> nit: nullptr throughout

Done.

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:47: wait_object_ = NULL;
On 2016/07/14 07:19:11, grt (UTC plus 2) wrote:
> take absolutely everything back to its initial state here to be sure that
> dangling refs aren't held (the task runner is the only culprit, but it's good
to
> be thorough). for style points, retain the ordering in the class decl so that
> it's easy to audit them. may as well make a helper function to do this that
can
> be called from StartWatchingInternal's failure path:
>   callback_.Reset();
>   object_ = nullptr;
>   wait_object_ = nullptr;
>   task_runner_.reset();
>   run_once_ = true;
>   weak_factory_.InvalidateWeakPtrs();

Done.

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:74: CHECK(delegate);
On 2016/07/14 07:19:11, grt (UTC plus 2) wrote:
> seems to me that this should be DCHECK since it represents a programmer error.
> wdyt?

Done.

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.cc:80: if (!ThreadTaskRunnerHandle::IsSet())
On 2016/07/14 07:19:11, grt (UTC plus 2) wrote:
> NOTREACHED for this, too? isn't it a programming error as well? does it make
> sense for this to be a DHCECK(ThreadTaskRunnerHandle::IsSet()); at the top of
> the function and no "return false;", since gracefully handling programmer
error
> is counter to coding style?

Done.

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher.h
File base/win/object_watcher.h (right):

https://codereview.chromium.org/2125763003/diff/20001/base/win/object_watcher...
base/win/object_watcher.h:103: Closure callback_;
On 2016/07/14 07:19:11, grt (UTC plus 2) wrote:
>   // A callback pre-bound to Signal() that is posted to the caller's task
runner
> when the wait completes.

Done.

[grt (UTC plus 2), 2016-07-18 19:42:35]

lgtm w/ nits

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher.cc
File base/win/object_watcher.cc (right):

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.cc:87: callback_ = base::Bind(&ObjectWatcher::Signal,
weak_factory_.GetWeakPtr(),
nit: omit base:: since we're already in the base:: namespace here

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher.h
File base/win/object_watcher.h (right):

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.h:109: // The object being watched
nit: period at end of sentence

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.h:112: // Returned by RegisterWaitForSingleObject
// The wait handle returned by...

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.h:115: // Used to get back to the origin thread
// The task runner of the caller's thread.
(or something like that?)

[fdoray, 2016-07-18 21:42:04]

cpu@: PTAL

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher.cc
File base/win/object_watcher.cc (right):

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.cc:87: callback_ = base::Bind(&ObjectWatcher::Signal,
weak_factory_.GetWeakPtr(),
On 2016/07/18 19:42:34, grt (UTC plus 2) wrote:
> nit: omit base:: since we're already in the base:: namespace here

Done.

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher.h
File base/win/object_watcher.h (right):

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.h:109: // The object being watched
On 2016/07/18 19:42:34, grt (UTC plus 2) wrote:
> nit: period at end of sentence

Done.

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.h:112: // Returned by RegisterWaitForSingleObject
On 2016/07/18 19:42:34, grt (UTC plus 2) wrote:
> // The wait handle returned by...

Done.

https://codereview.chromium.org/2125763003/diff/60001/base/win/object_watcher...
base/win/object_watcher.h:115: // Used to get back to the origin thread
On 2016/07/18 19:42:34, grt (UTC plus 2) wrote:
> // The task runner of the caller's thread.
> (or something like that?)

Done.

[fdoray, 2016-07-26 18:18:31]

Description was changed from

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

BUG=616447
==========

to

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

BUG=616447
==========

[fdoray, 2016-07-26 18:18:31]

fdoray@chromium.org changed reviewers:
- cpu@chromium.org

[fdoray, 2016-07-26 18:20:20]

Description was changed from

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

BUG=616447
==========

to

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

TBR=ben@chromium.org
BUG=616447
==========

[fdoray, 2016-07-26 18:21:10]

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

[fdoray, 2016-07-26 18:21:10]

ben@: PTAL to missing includes added to base/files/file_path_watcher_win.cc,
base/process/kill_win.cc, chrome/common/service_process_util_win.cc and
ui/gfx/font_fallback_win.cc

[fdoray, 2016-07-26 18:21:30]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-07-26 18:21:31]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2125763003/#ps80001
(title: "CR grt #12 (comments)")

[commit-bot: I haz the power, 2016-07-26 18:21:50]

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-07-26 19:10:29]

Description was changed from

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

TBR=ben@chromium.org
BUG=616447
==========

to

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

TBR=ben@chromium.org
BUG=616447
==========

[commit-bot: I haz the power, 2016-07-26 19:10:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-26 19:14:05]

Description was changed from

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

TBR=ben@chromium.org
BUG=616447
==========

to

==========
Remove MessageLoop::current() from base::win::ObjectWatcher.

Why? The fact that there's a MessageLoop on the thread is an
unnecessary implementation detail. When browser threads are migrated to
base/task_scheduler, tasks will no longer have access to a MessageLoop
but they will be able to get the current ThreadTaskRunnerHandle.

Before this CL, ObjectWatcher implemented
WillDestroyCurrentMessageLoop() to stop the watch when the
MessageLoop responsible for running the callback was destroyed.
This prevented the watch callback from being sent to a destroyed
MessageLoop.

Now that ObjectWatcher keeps a reference to a TaskRunner, this is
no longer required. The TaskRunner can't be deleted while there are
references to it. If the underlying MessageLoop has been deleted
when the watch callback is posted, the callback will simply not run.

Note that the watch will still be stopped when the ObjectWatcher is
deleted.

TBR=ben@chromium.org
BUG=616447

Committed: https://crrev.com/76db1513714741d8d6e6dd6e872d50f20b090fce
Cr-Commit-Position: refs/heads/master@{#407874}
==========

[commit-bot: I haz the power, 2016-07-26 19:14:06]

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

[fdoray, 2016-08-25 16:14:02]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2277333002/ by fdoray@chromium.org.

The reason for reverting is: Breaks Win10 builder. crbug.com/632184.