Require FilePathWatcher destructor to be called in sequence with Watch().

Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

TBR=scottbyer@chromium.org
BUG=650723
Committed: https://crrev.com/1ce19c6494518bcc1cc1fd4b2bfc73e6f6e41fa0
Cr-Commit-Position: refs/heads/master@{#431921}

[fdoray, 2016-10-21 18:13:03]

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

[commit-bot: I haz the power, 2016-10-21 18:13:39]

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-10-21 19:06:59]

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

[commit-bot: I haz the power, 2016-10-21 19:07:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fdoray, 2016-10-24 13:11:34]

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

[commit-bot: I haz the power, 2016-10-24 13:11:45]

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-10-24 13:55:32]

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

[commit-bot: I haz the power, 2016-10-24 13:55:33]

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

[fdoray, 2016-11-04 19:09:23]

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

[commit-bot: I haz the power, 2016-11-04 19:09:58]

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-11-04 19:38:32]

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

[commit-bot: I haz the power, 2016-11-04 19:38:33]

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-11-08 14:54:51]

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

[commit-bot: I haz the power, 2016-11-08 14:55:09]

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-11-08 15:06:35]

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

[commit-bot: I haz the power, 2016-11-08 15:06:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[fdoray, 2016-11-08 16:18:03]

Description was changed from

==========
SequenceChecker in FilePathWatcher

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

to

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that we can get
rid of the MessageLoop destruction observers (cleanup that must be
done on the sequence from which Watch() is called can be done from
the FilePathWatcher destructor rather than from a destruction
observer). Getting rid of destruction observers in FilePathWatcher
is important because they are not supported in TaskScheduler.

BUG=650723
==========

[fdoray, 2016-11-08 17:53:26]

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

[commit-bot: I haz the power, 2016-11-08 17:54:01]

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-11-08 20:14:44]

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

[commit-bot: I haz the power, 2016-11-08 20:14:45]

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

[fdoray, 2016-11-08 21:10:02]

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

[commit-bot: I haz the power, 2016-11-08 21:10:31]

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-11-08 22:19:01]

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

[commit-bot: I haz the power, 2016-11-08 22:19:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[fdoray, 2016-11-09 15:52:39]

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

[commit-bot: I haz the power, 2016-11-09 15:52:58]

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-11-09 17:27:43]

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

[commit-bot: I haz the power, 2016-11-09 17:27:44]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-11-09 17:48:31]

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

[fdoray, 2016-11-09 17:48:32]

PTAL

[fdoray, 2016-11-09 17:49:42]

Description was changed from

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that we can get
rid of the MessageLoop destruction observers (cleanup that must be
done on the sequence from which Watch() is called can be done from
the FilePathWatcher destructor rather than from a destruction
observer). Getting rid of destruction observers in FilePathWatcher
is important because they are not supported in TaskScheduler.

BUG=650723
==========

to

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

BUG=650723
==========

[dcheng, 2016-11-09 18:39:12]

lgtm

https://codereview.chromium.org/2438913003/diff/120001/base/files/file_path_w...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2438913003/diff/120001/base/files/file_path_w...
base/files/file_path_watcher.h:30: // Must be destructed on the sequence that
invokes Watch().
Nit: destroyed is slightly more standard terminology in Chromium

[fdoray, 2016-11-10 15:46:20]

fdoray@chromium.org changed reviewers:
+ scottbyer@chromium.org, thestig@chromium.org

[fdoray, 2016-11-10 15:46:21]

thestig@: Please review changes in chrome/browser/media_galleries and
chrome/common

scottbyer@: Please review changes in chrome/service.

https://codereview.chromium.org/2438913003/diff/120001/base/files/file_path_w...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2438913003/diff/120001/base/files/file_path_w...
base/files/file_path_watcher.h:30: // Must be destructed on the sequence that
invokes Watch().
On 2016/11/09 18:39:11, dcheng wrote:
> Nit: destroyed is slightly more standard terminology in Chromium

Done.

[Lei Zhang, 2016-11-10 18:59:31]

lgtm

https://codereview.chromium.org/2438913003/diff/160001/chrome/browser/media_g...
File chrome/browser/media_galleries/fileapi/file_path_watcher_util.h (right):

https://codereview.chromium.org/2438913003/diff/160001/chrome/browser/media_g...
chrome/browser/media_galleries/fileapi/file_path_watcher_util.h:25: // Stops a
file path watch started by StartFilePathWatchOnMediaTaskRunner. Must
nit: I really like to refer to functions as FuncName() so it is obvious that it
is a function and not a class.

[fdoray, 2016-11-11 15:47:39]

Scott Byer: Please review changes in chrome/service/

https://codereview.chromium.org/2438913003/diff/160001/chrome/browser/media_g...
File chrome/browser/media_galleries/fileapi/file_path_watcher_util.h (right):

https://codereview.chromium.org/2438913003/diff/160001/chrome/browser/media_g...
chrome/browser/media_galleries/fileapi/file_path_watcher_util.h:25: // Stops a
file path watch started by StartFilePathWatchOnMediaTaskRunner. Must
On 2016/11/10 18:59:31, Lei Zhang (slow) wrote:
> nit: I really like to refer to functions as FuncName() so it is obvious that
it
> is a function and not a class.

Done.

[fdoray, 2016-11-14 18:06:22]

Description was changed from

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

BUG=650723
==========

to

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

TBR=scottbyer@chromium.org
BUG=650723
==========

[Scott Byer, 2016-11-14 18:08:59]

lgtm

[fdoray, 2016-11-14 18:11:02]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2016-11-14 18:11:02]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2438913003/#ps180001
(title: "CR thestig #36 (fix comment)")

[commit-bot: I haz the power, 2016-11-14 18:11:57]

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-11-14 19:30:08]

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

[commit-bot: I haz the power, 2016-11-14 19:30:10]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[fdoray, 2016-11-14 19:42:16]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-11-14 19:43:53]

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-11-14 22:38:29]

Description was changed from

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

TBR=scottbyer@chromium.org
BUG=650723
==========

to

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

TBR=scottbyer@chromium.org
BUG=650723
==========

[commit-bot: I haz the power, 2016-11-14 22:38:30]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-14 22:58:36]

Description was changed from

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

TBR=scottbyer@chromium.org
BUG=650723
==========

to

==========
Require FilePathWatcher destructor to be called in sequence with Watch().

file_path_watcher_win.cc and file_path_watcher_kqueue.cc need to do
some cleanup on the sequence from which Watch() is called. They use
destruction observers to do this cleanup when the MessageLoop from
which Watch() is called is destroyed (after that, it is too late to
post tasks to the MessageLoop sequence).

With this CL, the FilePathWatcher destructor must be called on the
sequence from which Watch() is called. That means that will be able
to move the work done in MessageLoop destruction observers to the
FilePathWatcher destructor. Getting rid of destruction observers in
FilePathWatcher is important because they are not supported in
TaskScheduler.

TBR=scottbyer@chromium.org
BUG=650723

Committed: https://crrev.com/1ce19c6494518bcc1cc1fd4b2bfc73e6f6e41fa0
Cr-Commit-Position: refs/heads/master@{#431921}
==========

[commit-bot: I haz the power, 2016-11-14 22:58:37]

Patchset 10 (id:??) landed as
https://crrev.com/1ce19c6494518bcc1cc1fd4b2bfc73e6f6e41fa0
Cr-Commit-Position: refs/heads/master@{#431921}

[tommycli, 2016-11-19 00:27:53]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2514113003/ by tommycli@chromium.org.

The reason for reverting is: I'm sorry, but I'm pretty confident that this patch
is causing some trybot flakes.

See: https://bugs.chromium.org/p/chromium/issues/detail?id=665511

Flakes link:
https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9....