Add a DirectoryWatcher class, allowing objects to be notified whenever...

Add a DirectoryWatcher class, allowing objects to be notified whenever
a directory's contents change.


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

[Evan Martin, 2008-10-09 02:34:09]

Missing parts:
- needs to test unregistering
- needs to test file mutation (in addition to file adding, which it has now)

Anything else?

[darin (slow to review), 2008-10-09 20:57:36]

I think it would be great to follow the pattern used by ObjectWatcher.  Passing
a Delegate to the constructor tends to be problematic since it results in having
to use that funky macro on Windows to pass |this| to a member in a member
initializer list.

http://codereview.chromium.org/6377/diff/1/6
File base/directory_watcher.h (right):

http://codereview.chromium.org/6377/diff/1/6#newcode28
Line 28: bool Watch(MessageLoopForIO* message_loop, const std::wstring& path);
can you explain what the message_loop parameter is here?

[Aaron Boodman, 2008-10-09 20:59:02]

Seems good to me.

[cpu_(ooo_6.6-7.5), 2008-10-09 23:17:05]

http://codereview.chromium.org/6377/diff/1/2
File base/directory_watcher_win.cc (right):

http://codereview.chromium.org/6377/diff/1/2#newcode29
Line 29: FindCloseChangeNotification(handle_);
I would have to unregister the handle from the messsage_loop first, but see my
other comment for important, last minute information.

http://codereview.chromium.org/6377/diff/1/2#newcode48
Line 48: 
I believe WatchObject is being deprecated as we speak. The idea is to use the
thread pool watchers thingy instead, I think is in object_watcher.h.

[darin (slow to review), 2008-10-09 23:18:56]

http://codereview.chromium.org/6377/diff/1/2
File base/directory_watcher_win.cc (right):

http://codereview.chromium.org/6377/diff/1/2#newcode48
Line 48: 
Yup, this should be using base::ObjectWatcher

[brettw, 2008-10-10 00:10:52]

http://codereview.chromium.org/6377/diff/1/6
File base/directory_watcher.h (right):

http://codereview.chromium.org/6377/diff/1/6#newcode5
Line 5: // This module lets you register interest in changes on a directory.
How about moving this to right above the class and giving a little more detail
about what it will do and how to use it.

http://codereview.chromium.org/6377/diff/1/6#newcode32
Line 32: scoped_ptr<DirectoryWatcherImpl> impl_;
DISALLOW_*?

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

http://codereview.chromium.org/6377/diff/1/3#newcode42
Line 42: std::wstring test_dir_;
Are we ready to start using FilePath?

[darin (slow to review), 2008-10-10 21:17:35]

http://codereview.chromium.org/6377/diff/1/6
File base/directory_watcher.h (right):

http://codereview.chromium.org/6377/diff/1/6#newcode13
Line 13: class DirectoryWatcherImpl;
any reason not to make DirectoryWatcherImpl be an inner class? 
DirectoryWatcher::Impl for example?

[Evan Martin, 2008-10-14 20:24:14]

Ok, I think I've addressed all feedback, as well as fleshed out the tests. 
Please look again.

[darin (slow to review), 2008-10-15 00:02:13]

http://codereview.chromium.org/6377/diff/12/215
File base/directory_watcher.h (right):

http://codereview.chromium.org/6377/diff/12/215#newcode4
Line 4: 
please add a comment block up here that gives an overview of the class, etc.

http://codereview.chromium.org/6377/diff/12/215#newcode28
Line 28: bool Watch(const std::wstring& path, Delegate* delegate);
Q: not interested in providing a method to stop watching?

  StartWatching(const std::wstring& path, Delegate* delegate);
  StopWatching();

By the way, maybe it would be good to use the new FilePath class here?

http://codereview.chromium.org/6377/diff/12/211
File base/directory_watcher_win.cc (right):

http://codereview.chromium.org/6377/diff/12/211#newcode13
Line 13: Impl(DirectoryWatcher::Delegate* delegate) : delegate_(delegate) {}
you should probably initialize handle_ here though it is not strictly necessary.

http://codereview.chromium.org/6377/diff/12/211#newcode35
Line 35: watcher_.StopWatching();
might want to check if handle_ is not INVALID_HANDLE_VALUE.

http://codereview.chromium.org/6377/diff/12/211#newcode60
Line 60: delegate_->OnDirectoryChanged(path_);
couldn't |this| have been deleted by the time we return from OnDirectoryChanged?
 i think you should avoid dereferencing |this| afterwards.

please add a test for this case.

[Evan Martin, 2008-10-15 20:13:47]

Thanks for the detailed comments.

I think I've fixed them all, and I also used a scoped_refptr to keep the watcher
alive during the callback as well as added a test to cover that case.  (You want
"delta from patch set 2" to see the new changes.)

[darin (slow to review), 2008-10-15 20:19:34]

LGTM

http://codereview.chromium.org/6377/diff/17/605
File base/directory_watcher.h (right):

http://codereview.chromium.org/6377/diff/17/605#newcode18
Line 18: // directory.
are there any promises you can make about the race condition stuff we spoke
about at lunch?

[Evan Martin, 2008-10-15 21:47:21]

http://codereview.chromium.org/6377/diff/17/605
File base/directory_watcher.h (right):

http://codereview.chromium.org/6377/diff/17/605#newcode18
Line 18: // directory.
On 2008/10/15 20:19:35, darin wrote:
> are there any promises you can make about the race condition stuff we spoke
> about at lunch?

MSDN says ( http://msdn.microsoft.com/en-us/library/aa364427(VS.85).aspx ):
"If a change occurs after a call to FindFirstChangeNotification but before a
call to FindNextChangeNotification, the operating system records the change.
When FindNextChangeNotification is executed, the recorded change immediately
satisfies a wait for the change notification.

FindNextChangeNotification should not be used more than once on the same handle
without using one of the wait functions. An application may miss a change
notification if it uses FindNextChangeNotification when there is a change
request outstanding."

So I think as long as I always pair FindNext...() with a wait on the object, I'm
ok(?)

[Evan Martin, 2008-10-17 01:12:03]

My unit test turned out to be flaky.  I was doing:
  modify file;
  post a quit message to my message loop;
  run message loop;
but that requires the file modified notification to get into my message loop
before the quit message does.

I modified most tests to now be deterministic, and put in a timeout for the ones
that can't be.  The interesting diff to look at is
  http://codereview.chromium.org/6377/diff2/17:609/22

[darin (slow to review), 2008-10-17 02:43:51]

lgtm

http://codereview.chromium.org/6377/diff/609/22
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/6377/diff/609/22#newcode69
Line 69: MessageLoopForIO loop_;
I don't think DirectoryWatcher is limited to only running on a MessageLoopForIO
thread.  I think an ordinary MessageLoop should work just fine here.