Add DirectoryWatcher implementation for Mac.

Add DirectoryWatcher implementation for Mac.

This uses FSEvents and supports both recursive and non-recursive modes.

TEST=Make sure that tests matching DirectoryWatcherTest.* from base_unittests pass on Mac.

http://crbug.com/10967

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14977

[Paweł Hajdan Jr., 2009-04-27 13:43:59]

Please review:

pink: directory_watcher_mac.cc (this is one of my first Mac-specific files, so
please review carefully)
evan: changes in directory_watcher_unittest.cc
mark: gyp changes (I'm sure that it can be made better than I've done it in this
patch)

[Mark Mentovai, 2009-04-27 13:52:57]

Not a thorough review, but the .gyp LG.

http://codereview.chromium.org/99057/diff/1/2
File base/base.gyp (right):

http://codereview.chromium.org/99057/diff/1/2#newcode645
Line 645: ['OS == "mac"', {
Since this is empty now, you can just have an 'OS != "mac"' block to exclude
mac_util_unittest.cc from other platforms.

http://codereview.chromium.org/99057/diff/1/3
File base/directory_watcher_mac.cc (right):

http://codereview.chromium.org/99057/diff/1/3#newcode7
Line 7: #import <CoreServices/CoreServices.h>
Not an Obj-C header, use #include

http://codereview.chromium.org/99057/diff/1/3#newcode16
Line 16: const CFAbsoluteTime kFSEventsLatencySeconds = 0.3;
This name makes things confusing when it's used below, since it follows the same
naming conventions as the constants provided by the system.

http://codereview.chromium.org/99057/diff/1/3#newcode20
Line 20: DirectoryWatcherImpl() {}
Do you want to initialize the various members to safe values?

[pink (ping after 24hrs), 2009-04-27 14:40:10]

http://codereview.chromium.org/99057/diff/1/3
File base/directory_watcher_mac.cc (right):

http://codereview.chromium.org/99057/diff/1/3#newcode16
Line 16: const CFAbsoluteTime kFSEventsLatencySeconds = 0.3;
On 2009/04/27 13:52:57, Mark Mentovai wrote:
> This name makes things confusing when it's used below, since it follows the
same
> naming conventions as the constants provided by the system.

agreed. it's hard to tell this is one of our constants. Since it's namespaced,
there's no need to prefix with |FSEvents| to avoid collisions.

http://codereview.chromium.org/99057/diff/1/3#newcode53
Line 53: // Backend stream we receive events from (as callbacks).
might indicate this is a strong reference.

http://codereview.chromium.org/99057/diff/1/3#newcode74
Line 74: DCHECK(path_.value().empty());  // Can only watch one path.
so what would happen in release builds? can't we just un-register the old one
and start watching the new one so that nothing "leaks"?

[Paweł Hajdan Jr., 2009-04-27 18:17:50]

On 2009/04/27 13:52:57, Mark Mentovai wrote:
> Not a thorough review, but the .gyp LG.
> 
> http://codereview.chromium.org/99057/diff/1/2
> File base/base.gyp (right):
> 
> http://codereview.chromium.org/99057/diff/1/2#newcode645
> Line 645: ['OS == "mac"', {
> Since this is empty now, you can just have an 'OS != "mac"' block to exclude
> mac_util_unittest.cc from other platforms.

Done.

> http://codereview.chromium.org/99057/diff/1/3
> File base/directory_watcher_mac.cc (right):
> 
> http://codereview.chromium.org/99057/diff/1/3#newcode7
> Line 7: #import <CoreServices/CoreServices.h>
> Not an Obj-C header, use #include

Done.

> http://codereview.chromium.org/99057/diff/1/3#newcode16
> Line 16: const CFAbsoluteTime kFSEventsLatencySeconds = 0.3;
> This name makes things confusing when it's used below, since it follows the
same
> naming conventions as the constants provided by the system.

Done.

> http://codereview.chromium.org/99057/diff/1/3#newcode20
> Line 20: DirectoryWatcherImpl() {}
> Do you want to initialize the various members to safe values?

I don't see a big need to do that. path_.value().empty() indicates if the object
is initialized, and it's the general pattern in existing Windows and Linux
implementations. I added one more DCHECK for a sane state though.

On 2009/04/27 14:40:10, pink wrote:
> http://codereview.chromium.org/99057/diff/1/3#newcode53
> Line 53: // Backend stream we receive events from (as callbacks).
> might indicate this is a strong reference.
> 
> http://codereview.chromium.org/99057/diff/1/3#newcode74
> Line 74: DCHECK(path_.value().empty());  // Can only watch one path.
> so what would happen in release builds? can't we just un-register the old one
> and start watching the new one so that nothing "leaks"?

It's a general pattern in Windows and Mac implementations that a
DirectoryWatcher instance starts with not watching anything, then may watch
something, and then must be destroyed (cannot be "recycled"). I think it's a
good thing (simplifies other things). And current users don't need "recycling".

[Evan Martin, 2009-04-27 21:36:12]

Test changes LGTM, though I am scared it won't work on Windows.  I seem to
recall Vista and XP had different behaviors in this area...

Maybe it's worth just checking if there was a notification *at all* rather than
counting the number of notifications to cut down on cross-platform problems.

http://codereview.chromium.org/99057/diff/1007/9
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/99057/diff/1007/9#newcode129
Line 129: // We will inform thread when we get first notification.
I don't understand this comment or how it relates to test_ below it.

[Paweł Hajdan Jr., 2009-04-29 13:28:31]

pink: ping

On 2009/04/27 21:36:12, Evan Martin wrote:
> Test changes LGTM, though I am scared it won't work on Windows.  I seem to
> recall Vista and XP had different behaviors in this area...
> 
> Maybe it's worth just checking if there was a notification *at all* rather
than
> counting the number of notifications to cut down on cross-platform problems.

git-try tells me it works fine on XP. In fact, now checking for notification is
even less strict (while still checking important properties). I even think that
the test currently disabled for XP might start working. But I'll check that
later.

> http://codereview.chromium.org/99057/diff/1007/9
> File base/directory_watcher_unittest.cc (right):
> 
> http://codereview.chromium.org/99057/diff/1007/9#newcode129
> Line 129: // We will inform thread when we get first notification.
> I don't understand this comment or how it relates to test_ below it.

Ooops. This comment didn't make sense. I changed it.

[pink (ping after 24hrs), 2009-04-29 19:34:35]

http://codereview.chromium.org/99057/diff/12/14
File base/directory_watcher_mac.cc (right):

http://codereview.chromium.org/99057/diff/12/14#newcode93
Line 93: CFArrayCreate(NULL, reinterpret_cast<const void**>(&cf_path), 1, NULL);
i don't think this will work. You're creating an array with an item whose value
is the address of a local variable? Won't that die a horrible death as soon as
this function returns? Also, when do cf_path and watched_paths get released? Who
is responsible for doing so?

[Paweł Hajdan Jr., 2009-04-30 09:03:04]

After this patch things should be properly released. Tested on the trybots.
Please look.

pink, Mark: thanks for assistance on irc!

[pink (ping after 24hrs), 2009-04-30 13:34:36]

http://codereview.chromium.org/99057/diff/18/20
File base/directory_watcher_mac.cc (right):

http://codereview.chromium.org/99057/diff/18/20#newcode90
Line 90: CFStringRef cf_path = CFStringCreateWithCString(NULL,
path.value().c_str(),
i'd prefer you to use scoped_cftyperef<> here (and for watched_paths). That way
it's impossible to forget the release (or lose it in a merge, or return early,
etc etc)

http://codereview.chromium.org/99057/diff/18/20#newcode93
Line 93: CFArrayCreate(NULL, reinterpret_cast<const void**>(&cf_path), 1, NULL);
you need to pass |kCFTypeArrayCallBacks| for the last parameter or everything
will leak.

http://codereview.chromium.org/99057/diff/18/20#newcode111
Line 111: CFRelease(cf_path);
these can go when you switch to the scoped ptr.

[Paweł Hajdan Jr., 2009-04-30 15:44:47]

Done. I had to use a temporary variable because otherwise gcc complained that
unary & operand must be an lvalue. Tested on trybot.

[pink (ping after 24hrs), 2009-04-30 16:01:44]

lgtm