Use FSEvents for recursive file watch on Mac

Use FSEvents for recursive file watch on Mac

Brings back the old FSEvents code, changing it to support recursive watches only, and use dispatch queues instead of using the CFRunLoop from the UI thread.

BUG=144491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274735

[Mattias Nissler (ping if slow), 2014-05-26 07:39:36]

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:13: #include "base/memory/singleton.h"
used?

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:48: result = result.Append(target);
Don't you have to restart here, in case the symlink points to a target path
containing symlinks?

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:125: // left to FielPathWatcherKQueue.
*FilePathWatcherKqueue

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.h (right):

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:20: // directory.
Would be good to add a sentence explaining which strategy we use for deciding
whether we want to use kqueue and/or fsevents?

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:25: // Called from the FSEvents callback
whenever there is a change to the paths
Missing period.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:26: void OnFilePathChanged(const
std::vector<FilePath>& paths);
nit: Name is inconsistent, should probably be OnFilePathsChanged (note the
plural)

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:44: // Start watchign the FSEventStream.
*watching

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
File base/files/file_path_watcher_kqueue.h (right):

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_kqueue.h:17: // Mac-specific file watcher
implementation based on kqueue.
I assume this is merely a copy of the class declaration that was previously in
the file_path_watcher_kqueue.cc?

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_kqueue.h:55: FilePath path_;  // Full path to this
item.
#include "base/files/file_path.h"

[vandebo (ex-Chrome), 2014-05-28 00:26:55]

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:13: #include "base/memory/singleton.h"
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> used?

Done.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:48: result = result.Append(target);
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> Don't you have to restart here, in case the symlink points to a target path
> containing symlinks?

Indeed, there's a couple cases here. Fixed up.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:125: // left to FielPathWatcherKQueue.
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> *FilePathWatcherKqueue

Done.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.h (right):

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:20: // directory.
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> Would be good to add a sentence explaining which strategy we use for deciding
> whether we want to use kqueue and/or fsevents?

Done.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:25: // Called from the FSEvents callback
whenever there is a change to the paths
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> Missing period.

Done.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:26: void OnFilePathChanged(const
std::vector<FilePath>& paths);
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> nit: Name is inconsistent, should probably be OnFilePathsChanged (note the
> plural)

Done.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:44: // Start watchign the FSEventStream.
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> *watching

Done.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
File base/files/file_path_watcher_kqueue.h (right):

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_kqueue.h:17: // Mac-specific file watcher
implementation based on kqueue.
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> I assume this is merely a copy of the class declaration that was previously in
> the file_path_watcher_kqueue.cc?

Yes.

https://codereview.chromium.org/283423003/diff/80001/base/files/file_path_wat...
base/files/file_path_watcher_kqueue.h:55: FilePath path_;  // Full path to this
item.
On 2014/05/26 07:39:37, Mattias Nissler wrote:
> #include "base/files/file_path.h"

Done.

[Mattias Nissler (ping if slow), 2014-05-28 08:08:31]

LGTM w/ nits (in case I'm missing something with symlink resolution needing to
be performed again upon changes to the target path, or if you choose to address
that in a separate CL).

Also note that I'll be on paternity leave starting next week, so please rely on
other base/ OWNERS for reviewing further CL.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:41: std::vector<FilePath::StringType>
component_array;
FilePath vs. base::FilePath inconsistency here.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:41: std::vector<FilePath::StringType>
component_array;
nit: this should be |component_vector|.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:43: std::list<base::FilePath>
components;
Shouldn't this rather be std::list<base::FilePath::StringType>? Components
aren't paths.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:64: }
instead of the loop, you could also do

std::copy(target_components.begin(), target_components.end(),
          std::front_inserter(components));

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:245: resolved_target_ =
ResolvePath(target_).StripTrailingSeparators();
If we only resolve when starting the watcher, we'll fail when the target path
contains a link and that link changes.

You might want to re-resolve in UpdateEventStream. If you decide to fix this,
please also add a test.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
File base/files/file_path_watcher_mac.cc (right):

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_mac.cc:32: #endif  // OS_IOS
Uh, I'm not so fond of intermixing preprocessor and language control flow (and
the resulting style guide violation). How about wrapping the
FilePathWatcherKQueue initialization in an if (!impl_) conditional so it does
the right thing regardless of defined(OS_IOS)?

[vandebo (ex-Chrome), 2014-05-28 18:54:52]

On 2014/05/28 08:08:31, Mattias Nissler wrote:
> LGTM w/ nits (in case I'm missing something with symlink resolution needing to
> be performed again upon changes to the target path, or if you choose to
address
> that in a separate CL).

Thank you for being a thorough reviewer!
 
> Also note that I'll be on paternity leave starting next week, so please rely
on
> other base/ OWNERS for reviewing further CL.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:41: std::vector<FilePath::StringType>
component_array;
On 2014/05/28 08:08:32, Mattias Nissler wrote:
> FilePath vs. base::FilePath inconsistency here.

Fixed.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:41: std::vector<FilePath::StringType>
component_array;
On 2014/05/28 08:08:32, Mattias Nissler wrote:
> nit: this should be |component_vector|.

Done.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:43: std::list<base::FilePath>
components;
On 2014/05/28 08:08:32, Mattias Nissler wrote:
> Shouldn't this rather be std::list<base::FilePath::StringType>? Components
> aren't paths.

Indeed.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:64: }
On 2014/05/28 08:08:32, Mattias Nissler wrote:
> instead of the loop, you could also do
> 
> std::copy(target_components.begin(), target_components.end(),
>           std::front_inserter(components));

Yup, I had target_components.insert(...) before but was tripped up by the
explicit constructor on FilePath(StringType).

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.cc:245: resolved_target_ =
ResolvePath(target_).StripTrailingSeparators();
On 2014/05/28 08:08:32, Mattias Nissler wrote:
> If we only resolve when starting the watcher, we'll fail when the target path
> contains a link and that link changes.

Yup, Fixed.

> You might want to re-resolve in UpdateEventStream. If you decide to fix this,
> please also add a test.

Done.

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
File base/files/file_path_watcher_mac.cc (right):

https://codereview.chromium.org/283423003/diff/120001/base/files/file_path_wa...
base/files/file_path_watcher_mac.cc:32: #endif  // OS_IOS
On 2014/05/28 08:08:32, Mattias Nissler wrote:
> Uh, I'm not so fond of intermixing preprocessor and language control flow (and
> the resulting style guide violation). How about wrapping the
> FilePathWatcherKQueue initialization in an if (!impl_) conditional so it does
> the right thing regardless of defined(OS_IOS)?

Thanks, made it even a bit cleaner.

[vandebo (ex-Chrome), 2014-05-28 18:55:33]

+Brett for build files.

[brettw, 2014-05-28 20:55:27]

LGTM, I didn't check the code.

https://codereview.chromium.org/283423003/diff/160001/base/files/file_path_wa...
File base/files/file_path_watcher_fsevents.h (right):

https://codereview.chromium.org/283423003/diff/160001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.h:17: // There are trade-offs between the
FSEvents implementation and a kqueue
Can this start like the kqueue one ("Mac-specific...") so it's obvious that this
is for Mac only?

https://codereview.chromium.org/283423003/diff/160001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.h:70: }  // namespace
}  // namespace base

[vandebo (ex-Chrome), 2014-05-28 21:14:55]

https://codereview.chromium.org/283423003/diff/160001/base/files/file_path_wa...
File base/files/file_path_watcher_fsevents.h (right):

https://codereview.chromium.org/283423003/diff/160001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.h:17: // There are trade-offs between the
FSEvents implementation and a kqueue
On 2014/05/28 20:55:27, brettw wrote:
> Can this start like the kqueue one ("Mac-specific...") so it's obvious that
this
> is for Mac only?

Done.

https://codereview.chromium.org/283423003/diff/160001/base/files/file_path_wa...
base/files/file_path_watcher_fsevents.h:70: }  // namespace
On 2014/05/28 20:55:27, brettw wrote:
> }  // namespace base

Done.

[Mattias Nissler (ping if slow), 2014-05-30 15:47:18]

LGTM still.

[vandebo (ex-Chrome), 2014-05-30 15:51:54]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:53:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vandebo@chromium.org/283423003/200001

[commit-bot: I haz the power, 2014-05-31 01:16:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-31 01:21:16]

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

[commit-bot: I haz the power, 2014-05-31 01:21:16]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[vandebo (ex-Chrome), 2014-06-02 20:52:13]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-06-02 20:53:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vandebo@chromium.org/283423003/200001

[commit-bot: I haz the power, 2014-06-03 04:07:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-03 04:09:36]

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

[commit-bot: I haz the power, 2014-06-03 04:09:37]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[vandebo (ex-Chrome), 2014-06-03 18:01:00]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-06-03 18:02:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vandebo@chromium.org/283423003/220001

[commit-bot: I haz the power, 2014-06-03 22:02:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[vandebo (ex-Chrome), 2014-06-03 23:37:14]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-06-03 23:38:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vandebo@chromium.org/283423003/240001

[commit-bot: I haz the power, 2014-06-04 09:02:40]

Change committed as 274735

[dconnelly, 2014-06-04 10:18:50]

On 2014/06/04 09:02:40, I haz the power (commit-bot) wrote:
> Change committed as 274735

Reverted for breaking Mac tests: https://codereview.chromium.org/316853003/