Remove destruction observer from base::FilePathWatcherKQueue.

base/files/file_path_watcher.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This module provides a way to monitor a file or directory for changes.
 
 #ifndef BASE_FILES_FILE_PATH_WATCHER_H_
 #define BASE_FILES_FILE_PATH_WATCHER_H_
 
 #include "base/base_export.h"
@@ skipping 72 matching lines @@
 
   // A callback that always cleans up the PlatformDelegate, either when executed
   // or when deleted without having been executed at all, as can happen during
   // shutdown.
   static void CancelWatch(const scoped_refptr<PlatformDelegate>& delegate);
 
   // Returns true if the platform and OS version support recursive watches.
   static bool RecursiveWatchAvailable();
 
   // Invokes |callback| whenever updates to |path| are detected. This should be
-  // called at most once, and from a MessageLoop of TYPE_IO. Set |recursive| to
-  // true, to watch |path| and its children. The callback will be invoked on
-  // the same loop. Returns true on success.
+  // called at most once. Set |recursive| to true, to watch |path| and its
+  // children. The callback will be invoked on the same thread. Returns true on
+  // success.
+  //
+  // On Mac, this must be called from a thread that supports
+  // FileDescriptorWatcher when |recursive| is false.

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

I don't understand this comment: is there more background to this that I can
read somewhere?

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

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]

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]

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.

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

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]

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

   //
   // Recursive watch is not supported on all platforms and file systems.
   // Watch() will return false in the case of failure.
   bool Watch(const FilePath& path, bool recursive, const Callback& callback);
 
  private:
   scoped_refptr<PlatformDelegate> impl_;
 
   SequenceChecker sequence_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(FilePathWatcher);
 };
 
 }  // namespace base
 
 #endif  // BASE_FILES_FILE_PATH_WATCHER_H_