Remove destruction observer from base::FilePathWatcherKQueue.

Remove destruction observer from base::FilePathWatcherKQueue.

Now that FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

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

[fdoray, 2016-11-21 22:01:02]

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

[commit-bot: I haz the power, 2016-11-21 22:02:34]

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-21 23:50:01]

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

[commit-bot: I haz the power, 2016-11-21 23:50:03]

Dry run: This issue passed the CQ dry run.

[fdoray, 2016-11-22 13:17:28]

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

[fdoray, 2016-11-22 13:17:28]

PTAL

[dcheng, 2016-11-23 02:14:25]

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:98: // FileDescriptorWatcher when |recursive| is
false.
I don't understand this comment: is there more background to this that I can
read somewhere?

[fdoray, 2016-11-23 12:58:37]

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:98: // FileDescriptorWatcher when |recursive| is
false.
On 2016/11/23 02:14:25, dcheng wrote:
> I don't understand this comment: is there more background to this that I can
> read somewhere?

file_path_watcher_mac.cc:31 -> FilePathWatcherKQueue is used when |recursive| is
false
file_path_watcher_kqueue.cc:276 -> FilePathWatcherKQueue uses
FileDescriptorWatcher, which means that it can only be used from a thread that
instantiated a FileDescriptorWatcher (see file_descriptor_watcher_posix.h:85)

[dcheng, 2016-11-23 14:16:11]

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:98: // FileDescriptorWatcher when |recursive| is
false.
On 2016/11/23 12:58:37, fdoray wrote:
> On 2016/11/23 02:14:25, dcheng wrote:
> > I don't understand this comment: is there more background to this that I can
> > read somewhere?
> 
> file_path_watcher_mac.cc:31 -> FilePathWatcherKQueue is used when |recursive|
is
> false
> file_path_watcher_kqueue.cc:276 -> FilePathWatcherKQueue uses
> FileDescriptorWatcher, which means that it can only be used from a thread that
> instantiated a FileDescriptorWatcher (see file_descriptor_watcher_posix.h:85)

I'm wondering more about the Mac-specific bits. It seems weird to expose this
(implementation-specific) difference in the API, and I guess I'm wondering if
there's a way to not require that difference.

[fdoray, 2016-11-23 16:21:41]

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:98: // FileDescriptorWatcher when |recursive| is
false.
On 2016/11/23 14:16:11, dcheng wrote:
> On 2016/11/23 12:58:37, fdoray wrote:
> > On 2016/11/23 02:14:25, dcheng wrote:
> > > I don't understand this comment: is there more background to this that I
can
> > > read somewhere?
> > 
> > file_path_watcher_mac.cc:31 -> FilePathWatcherKQueue is used when
|recursive|
> is
> > false
> > file_path_watcher_kqueue.cc:276 -> FilePathWatcherKQueue uses
> > FileDescriptorWatcher, which means that it can only be used from a thread
that
> > instantiated a FileDescriptorWatcher (see
file_descriptor_watcher_posix.h:85)
> 
> I'm wondering more about the Mac-specific bits. It seems weird to expose this
> (implementation-specific) difference in the API, and I guess I'm wondering if
> there's a way to not require that difference.

Three potential solutions:

1. Document that this must always be called from a thread that supports
FileDescriptorWatcher (even though it's only true for Mac/non-recursive).
2. Use FilePathWatcherFSEvents instead of FilePathWatcherKQueue when we detect
that the thread doesn't support FileDescriptorWatcher.
3. Delete FilePathWatcherKQueue and use FilePathWatcherFSEvents all the time. It
seems like the reason why we have FilePathWatcherKQueue is because FSEvents is
flaky on Mac OS X 10.6.7
https://codereview.chromium.org/6731064/diff/22001/content/common/file_path_w...
We only support OS X 10.9+. I verified that FSEvents is not flaky on OS X 10.11
using http://code.google.com/p/chromium/issues/detail?id=54822#c31. I don't have
Macs running other versions of OS X.

I like solution #3.

[dcheng, 2016-11-23 19:22:47]

I'm OK with this, but perhaps get a Mac person to sign off on this?

[dcheng, 2016-11-23 19:22:56]

(i.e. thakis or mark)

[fdoray, 2016-11-23 20:21:53]

mark@: Could we get rid of FilePathWatcherKQueue? See discussion here
https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...

[fdoray, 2016-11-23 20:22:19]

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

[fdoray, 2016-11-23 20:22:20]

mark@: Could we get rid of FilePathWatcherKQueue? See discussion here
https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...

[Mark Mentovai, 2016-11-29 19:00:22]

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:98: // FileDescriptorWatcher when |recursive| is
false.
On 2016/11/23 16:21:41, fdoray wrote:
> On 2016/11/23 14:16:11, dcheng wrote:
> > On 2016/11/23 12:58:37, fdoray wrote:
> > > On 2016/11/23 02:14:25, dcheng wrote:
> > > > I don't understand this comment: is there more background to this that I
> can
> > > > read somewhere?
> > > 
> > > file_path_watcher_mac.cc:31 -> FilePathWatcherKQueue is used when
> |recursive|
> > is
> > > false
> > > file_path_watcher_kqueue.cc:276 -> FilePathWatcherKQueue uses
> > > FileDescriptorWatcher, which means that it can only be used from a thread
> that
> > > instantiated a FileDescriptorWatcher (see
> file_descriptor_watcher_posix.h:85)
> > 
> > I'm wondering more about the Mac-specific bits. It seems weird to expose
this
> > (implementation-specific) difference in the API, and I guess I'm wondering
if
> > there's a way to not require that difference.
> 
> Three potential solutions:
> 
> 1. Document that this must always be called from a thread that supports
> FileDescriptorWatcher (even though it's only true for Mac/non-recursive).
> 2. Use FilePathWatcherFSEvents instead of FilePathWatcherKQueue when we detect
> that the thread doesn't support FileDescriptorWatcher.
> 3. Delete FilePathWatcherKQueue and use FilePathWatcherFSEvents all the time.
It
> seems like the reason why we have FilePathWatcherKQueue is because FSEvents is
> flaky on Mac OS X 10.6.7
>
https://codereview.chromium.org/6731064/diff/22001/content/common/file_path_w...
> We only support OS X 10.9+. I verified that FSEvents is not flaky on OS X
10.11
> using http://code.google.com/p/chromium/issues/detail?id=54822#c31. I don't
have
> Macs running other versions of OS X.

FSEvents are more heavyweight than a kqueue()-based watcher. They’ll also break
down sooner. I would generally prefer to use the kqueue()-based watcher where we
are able.

[fdoray, 2016-11-29 19:09:47]

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/2517323002/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:98: // FileDescriptorWatcher when |recursive| is
false.
On 2016/11/29 19:00:22, Mark Mentovai wrote:
> On 2016/11/23 16:21:41, fdoray wrote:
> > On 2016/11/23 14:16:11, dcheng wrote:
> > > On 2016/11/23 12:58:37, fdoray wrote:
> > > > On 2016/11/23 02:14:25, dcheng wrote:
> > > > > I don't understand this comment: is there more background to this that
I
> > can
> > > > > read somewhere?
> > > > 
> > > > file_path_watcher_mac.cc:31 -> FilePathWatcherKQueue is used when
> > |recursive|
> > > is
> > > > false
> > > > file_path_watcher_kqueue.cc:276 -> FilePathWatcherKQueue uses
> > > > FileDescriptorWatcher, which means that it can only be used from a
thread
> > that
> > > > instantiated a FileDescriptorWatcher (see
> > file_descriptor_watcher_posix.h:85)
> > > 
> > > I'm wondering more about the Mac-specific bits. It seems weird to expose
> this
> > > (implementation-specific) difference in the API, and I guess I'm wondering
> if
> > > there's a way to not require that difference.
> > 
> > Three potential solutions:
> > 
> > 1. Document that this must always be called from a thread that supports
> > FileDescriptorWatcher (even though it's only true for Mac/non-recursive).
> > 2. Use FilePathWatcherFSEvents instead of FilePathWatcherKQueue when we
detect
> > that the thread doesn't support FileDescriptorWatcher.
> > 3. Delete FilePathWatcherKQueue and use FilePathWatcherFSEvents all the
time.
> It
> > seems like the reason why we have FilePathWatcherKQueue is because FSEvents
is
> > flaky on Mac OS X 10.6.7
> >
>
https://codereview.chromium.org/6731064/diff/22001/content/common/file_path_w...
> > We only support OS X 10.9+. I verified that FSEvents is not flaky on OS X
> 10.11
> > using http://code.google.com/p/chromium/issues/detail?id=54822#c31. I don't
> have
> > Macs running other versions of OS X.
> 
> FSEvents are more heavyweight than a kqueue()-based watcher. They’ll also
break
> down sooner. I would generally prefer to use the kqueue()-based watcher where
we
> are able.

Then I'll do #1 (document that FilePathWatcher::Watch() may only be called from
a thread that supports FileDescriptorWatcher).

[Mark Mentovai, 2016-11-29 19:22:54]

That sounds fine to me.

[fdoray, 2016-11-29 19:40:22]

mark@: PTAL

[fdoray, 2016-11-29 19:41:12]

Description was changed from

==========
Remove destruction observer from base::FilePathWatcherKQueue.

Now that the FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

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

to

==========
Remove destruction observer from base::FilePathWatcherKQueue.

Now that FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

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

[Mark Mentovai, 2016-11-29 19:43:01]

LGTM

[fdoray, 2016-11-29 20:02:40]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2016-11-29 20:03:11]

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-29 22:20:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480449760806210,
"parent_rev": "bf5fd6fe5d757b949f5782af274642c847074fcd", "commit_rev":
"dec347a20d80beef1060b4a9d94342745627fc9c"}

[commit-bot: I haz the power, 2016-11-29 22:20:34]

Description was changed from

==========
Remove destruction observer from base::FilePathWatcherKQueue.

Now that FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

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

to

==========
Remove destruction observer from base::FilePathWatcherKQueue.

Now that FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

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

[commit-bot: I haz the power, 2016-11-29 22:20:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-29 22:24:54]

Description was changed from

==========
Remove destruction observer from base::FilePathWatcherKQueue.

Now that FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

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

to

==========
Remove destruction observer from base::FilePathWatcherKQueue.

Now that FilePathWatcher::~FilePathWatcher() is called on the same
sequence as FilePathWatcher::Watch() (see
https://codereview.chromium.org/2438913003/), the work done in
FilePathWatcherKQueue::WillDestroyCurrentMessageLoop() can safely be
moved to FilePathWatcherKQueue::Cancel().

BUG=650318

Committed: https://crrev.com/52e11d48575954e29aa9407c4343d76f0475327a
Cr-Commit-Position: refs/heads/master@{#435092}
==========

[commit-bot: I haz the power, 2016-11-29 22:24:55]

Patchset 2 (id:??) landed as
https://crrev.com/52e11d48575954e29aa9407c4343d76f0475327a
Cr-Commit-Position: refs/heads/master@{#435092}