Add support for almost-recursive watches in Linux DirectoryWatcher...

Add support for almost-recursive watches in Linux DirectoryWatcher

After this patch DirectoryWatcher when asked for recursive watch will scan the subtree and add inotify watches for each subfolder, but further changes to the tree structure won't trigger adding/removing watches.

Support for really recursive watches is planned. This is just to divide the work, because the task is not easy.

http://crbug.com/8968

[j.dinata, 2009-04-24 20:40:54]

This is suppose to be on issue http://codereview.chromium.org/92032 but gcl
change will not add file_util.h and file_util_posix.cc, therefore I created this
new issue.

1) Move GetInode to file_util.h
2) On recurisve mode: spawn a child watcher, instead of keeping track subtree
watches.
3) Run file enumerator on seperate thread.

[Paweł Hajdan Jr., 2009-04-24 21:26:04]

http://codereview.chromium.org/92151/diff/1/4
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/92151/diff/1/4#newcode84
Line 84: 
nit: unnecessary empty line

http://codereview.chromium.org/92151/diff/1/4#newcode91
Line 91: void HandleInotifyEvent(inotify_event* event);
Is it possible to make the pointer const?

http://codereview.chromium.org/92151/diff/1/4#newcode94
Line 94: void OnEnumeratedPath(const std::set<FilePath>& subtree);
You probably want to use FilePathSet here. Also, |subtree| should be called
|path| instead.

http://codereview.chromium.org/92151/diff/1/4#newcode98
Line 98: // Returns true if watch for |path| has been added successfuly. Watches
nit: successfully

http://codereview.chromium.org/92151/diff/1/4#newcode108
Line 108: bool isPathWatched(ino_t inode) const;
It should be named inode_watched then (note the unix_hacker naming style, it's a
cheap operation).

http://codereview.chromium.org/92151/diff/1/4#newcode111
Line 111: bool watchSubtree(const FilePathSet& subtree);
nit: WatchSubtree

Also, change it to something like:
// Register watches for |path| and its directory subtree. Returns true on
success.
bool WatchSubtree(const FilePathSet& path);

http://codereview.chromium.org/92151/diff/1/4#newcode113
Line 113: // Lock to protect attributes.
Please be more precise. You can say "members below" or "attributes below"...

http://codereview.chromium.org/92151/diff/1/4#newcode127
Line 127: // Hold inode for paths that are watched.
nit: Set of watched inodes.

http://codereview.chromium.org/92151/diff/1/4#newcode128
Line 128: InodeSet path_watched_;
inodes_watched_ ?

http://codereview.chromium.org/92151/diff/1/4#newcode152
Line 152: :watcher_(watcher), path_(path) {}
Oops. Please read the google style guide for c++ on how to format that kind of
code.

http://codereview.chromium.org/92151/diff/1/4#newcode160
Line 160: !subdirectory.empty();
nit: indent should be one space more, here and on next line.

http://codereview.chromium.org/92151/diff/1/4#newcode162
Line 162: subtree.insert(subdirectory);
nit: and this should be indented 2, not 4 spaces

http://codereview.chromium.org/92151/diff/1/4#newcode358
Line 358: : root_watch_(InotifyReader::kInvalidWatch),
nit: this should be indented 4 spaces

http://codereview.chromium.org/92151/diff/1/4#newcode377
Line 377: if  (path_enumerator_thread_started_)
No need for this check. Stop for a stopped thread is a no-op.

http://codereview.chromium.org/92151/diff/1/4#newcode460
Line 460: DirectoryWatcherImpl* child = new DirectoryWatcherImpl();
I'm a bit afraid about using this recursive pattern for this patch (it'll
probably be useful for full recursive support). Let's do this patch the simplest
possible way. Just record all watched inodes in DirectoryWatcherImpl...

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

http://codereview.chromium.org/92151/diff/1/5#newcode171
Line 171: sleep(1);
Instead of sleep, you can stop the enumeration thread. Make some friends or
something to get access to it. I can describe it in more detail if needed.

http://codereview.chromium.org/92151/diff/1/5#newcode251
Line 251: bool recursive;
No need for temp variable, see below.

http://codereview.chromium.org/92151/diff/1/5#newcode258
Line 258: this, recursive));
instead of recursive here, just say (i % 2) == 0

http://codereview.chromium.org/92151/diff/1/5#newcode276
Line 276: // testdir
This comment should read more naturally. Please include some header and a
bulleted list (1. 2. 3. what will happen). Like:

1. Start with directory structure (...) and set up recursive watches for (...)
and (...)
2. Move ... from ... to ...
3. Make sure that recursive watches still work as intended.

http://codereview.chromium.org/92151/diff/1/5#newcode306
Line 306: VerifyExpectedNumberOfModifications();
Good, but do some further writes after the move to verify that now the watches
are correct.

Also, instead of one big block of operations, verify number of modifications
more frequently.

[Paweł Hajdan Jr., 2009-04-27 19:09:32]

Some comments, but it's close. Be sure to ping Evan for review.

http://codereview.chromium.org/92151/diff/2001/3003
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/92151/diff/2001/3003#newcode87
Line 87: friend void DirectoryWatcher::StopPathEnumeratorThread(void) const;
Um, this is not a good way to do it, but close.

Let's add this kind of method to PlatformDelegate, with a more generic name,
like bool EnsureSetupFinished, returning true on success. Make it a no-op for
non-Linux implementations, and stop the thread for Linux.

http://codereview.chromium.org/92151/diff/2001/3003#newcode95
Line 95: void HandleInotifyEvent(const inotify_event* event);
nit: Let's call it OnInotifyEvent for consistency...

http://codereview.chromium.org/92151/diff/2001/3003#newcode101
Line 101: // Watch entire subtree, when |recursive| is true.
nit: please use "if" instead of "when"

http://codereview.chromium.org/92151/diff/2001/3003#newcode112
Line 112: // Unix_hacker naming style for this cheap operation.
No need for this comment (about naming). It's a recognized convention, at least
for Chromium.

http://codereview.chromium.org/92151/diff/2001/3003#newcode128
Line 128: typedef std::set<ino_t> InodeSet;
nit: please move this typedef up, close to WatchSet.

http://codereview.chromium.org/92151/diff/2001/3003#newcode150
Line 150: class DirectoryWatcherImplEnumeratePathTask : public Task {
Ooops. This name ended up quite long. Let's call it just
RegisterSubtreeWatchesTask.

http://codereview.chromium.org/92151/diff/2001/3003#newcode168
Line 168: watcher_->OnEnumeratedPath(subtree);
Let's just have OnEnumeratedPath take a FilePath parameter instead of set...
Seems more intuitive and simpler to me. Then you can remove the WatchSubtree
method and do all the work in OnEnumeratedPath.

http://codereview.chromium.org/92151/diff/2001/3003#newcode182
Line 182: :delegate_(delegate), path_(path) {}
nit: please format as above, ...EnumeratePathTask is formatted correctly.

http://codereview.chromium.org/92151/diff/2001/3003#newcode362
Line 362: : root_watch_(InotifyReader::kInvalidWatch),
nit: watch_ is a more accurate name, at least for now. Please use watch_ and
we'll see if/how to change it in the later patch.

http://codereview.chromium.org/92151/diff/2001/3003#newcode376
Line 376: path_enumerator_thread_.Stop();
This thread might have been adding watches up to this point. Stopping this
thread must be the first thing done in this method.

http://codereview.chromium.org/92151/diff/2001/3004
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/92151/diff/2001/3004#newcode254
Line 254: this, (i%2)==0));
nit: spaces around operators...

http://codereview.chromium.org/92151/diff/2001/3001
File base/file_util.h (right):

http://codereview.chromium.org/92151/diff/2001/3001#newcode340
Line 340: // Store inode number of |path| on |inode|. Return true on success.
nit: on -> in

[Evan Martin, 2009-04-27 21:43:24]

(Please ping me again after you've addressed Paweł's comments.)

[j.dinata, 2009-04-29 19:55:53]

Hi Pawel, Evan, and Aaron, 

Please comment on this change list.

This change list is from Pawel's last review.

Thanks

[Paweł Hajdan Jr., 2009-04-29 20:29:42]

This is almost good now. After you fix these issues, I'm going to submit it as a
try job to verify that it works on Linux, Mac, and Windows. I'll send you the
results if something breaks.

http://codereview.chromium.org/92151/diff/2005/2007
File base/directory_watcher.h (right):

http://codereview.chromium.org/92151/diff/2005/2007#newcode23
Line 23: virtual ~Delegate() {}
Good catch! One nit: please move the ctor above the method.

http://codereview.chromium.org/92151/diff/2005/2007#newcode42
Line 42: void EnsureSetupFinished() {
nit: can we make it private and friend relevant tests?

You can do that using FRIEND_TEST from gtest_prod.h. You'll then have to take
most of the unit test code out of the anonymous namespace (so friend will take
effect).

http://codereview.chromium.org/92151/diff/2005/2007#newcode56
Line 56: virtual void EnsureSetupFinished() {}
Please just say in the comment that this operation blocks until all required OS
native watches are set up.

http://codereview.chromium.org/92151/diff/2005/2010
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/92151/diff/2005/2010#newcode87
Line 87: void EnsureSetupFinished();
nit: This should be public, and I think that the comment above is not so
necessary. I think that comment in directory_watcher.h explains it well.

http://codereview.chromium.org/92151/diff/2005/2010#newcode89
Line 89: public:
nit: public should be indented 1 space, not 2

http://codereview.chromium.org/92151/diff/2005/2010#newcode102
Line 102: // if |recursive| is true, watch entire subtree.
nit: Should be If (uppercase first letter)

http://codereview.chromium.org/92151/diff/2005/2010#newcode111
Line 111: 
nit: please remove this blank line

http://codereview.chromium.org/92151/diff/2005/2010#newcode129
Line 129: // Keep track to registered watches.
nit: to -> of

http://codereview.chromium.org/92151/diff/2005/2010#newcode141
Line 141: MessageLoop* loop_;
Let's say it's a loop we'll post directory change notifications to.

http://codereview.chromium.org/92151/diff/2005/2010#newcode149
Line 149: const FilePath& path)
nit: please align this line with the line above

Something like CtorName(param1,
                        param2

http://codereview.chromium.org/92151/diff/2005/2010#newcode452
Line 452: if (path_enumerator_thread_.Start()) {
Let's add a NOTREACHED() if the thread couldn't be started.

http://codereview.chromium.org/92151/diff/2005/2010#newcode468
Line 468: void DirectoryWatcherImpl::EnsureSetupFinished() {
nit: this should me moved to the anonymous namespace above

http://codereview.chromium.org/92151/diff/2005/2011
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/92151/diff/2005/2011#newcode86
Line 86: // Reset directory_mods_ after verify so we get current mods
nit-like: maybe reset the counter in SetExpected... instead. Then Verify will
only verify things, and Set will set up all things required. And the comment
won't be needed then.

http://codereview.chromium.org/92151/diff/2005/2011#newcode254
Line 254: this, (i%2) == 0));
nit: should be (i % 2) instead of (i%2)

http://codereview.chromium.org/92151/diff/2005/2011#newcode283
Line 283: 
nit: for consistency with the next block, let's remove this empty line

http://codereview.chromium.org/92151/diff/2005/2011#newcode284
Line 284: // Verify that modifications to a subdirectory are noticed by
recursive watch.
This comment looks misleading here. I'd just delete it.

http://codereview.chromium.org/92151/diff/2005/2011#newcode293
Line 293: // Write a file to the subdirectories.
This write part makes the test long, and we've verified this earlier. Please
delete lines 293-302.

http://codereview.chromium.org/92151/diff/2005/2011#newcode305
Line 305: // Why this move test_file2 into SubDir and delete SubDir2?
This comment looks a bit weird...

http://codereview.chromium.org/92151/diff/2005/2011#newcode307
Line 307: ASSERT_TRUE((file_util::Move(test_dir_.Append(subdir2),
nit: unneeded pair of parentheses, and please align the next line

http://codereview.chromium.org/92151/diff/2005/2011#newcode311
Line 311: WriteTestDirFile(test_file.value(), "some content_1");
Now this is the place I want to verify that watches still work (you can
cut'n'paste lines 293-302 here). However, I think that with current
implementation of DirectoryWatcher on Linux it won't pass (that's okay for me).
In that case, just remove this test and we'll work on it for the final patch.

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

Oh, and there's one lint nit:
http://codereview.chromium.org/lint_patch/issue92151_2005_2010

Please fix it. You can ignore the other lint message about C header. Actually
it's bogus in this case.

[j.dinata, 2009-05-01 06:40:28]

Another iteration based on Pawel's review

[Paweł Hajdan Jr., 2009-05-01 07:40:28]

Just few nits. Please also sync with trunk. I recently landed a change in
directory_watcher_unittest, there may be some conflicts.

http://codereview.chromium.org/92151/diff/3011/3013
File base/directory_watcher.h (right):

http://codereview.chromium.org/92151/diff/3011/3013#newcode12
Line 12: 
nit: no need for this empty line

http://codereview.chromium.org/92151/diff/3011/3013#newcode54
Line 54: // This operation will blocks until all required OS native watches are
nit: blocks -> block

And I think that the comment above should now be removed. It's too
implementation-specific ("To ensure... No-op on window.").

http://codereview.chromium.org/92151/diff/3011/3013#newcode59
Line 59: FRIEND_TEST(DirectoryWatcherTest, SubDirRecursive);
nit: add empty line between FRIEND_TEST and EnsureSetupFinished.

http://codereview.chromium.org/92151/diff/3011/3016
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/92151/diff/3011/3016#newcode98
Line 98: // if |recursive| is true, watch entire subtree.
nit: should be If (uppercase first letter).

http://codereview.chromium.org/92151/diff/3011/3016#newcode99
Line 99: // Returns true. If watch for |path| has been added successfully.
Watches
nit: no dot. Should be: Returns true if watch ...

http://codereview.chromium.org/92151/diff/3011/3016#newcode136
Line 136: // loop where we post directory change notifications to.
nit: Loop (uppercase first letter)

[j.dinata, 2009-05-05 05:33:16]

Hi

Here another iteration for nit fix and trunk sync on directory_watcher_unittest

Thanks

[Paweł Hajdan Jr., 2009-05-05 15:25:38]

http://codereview.chromium.org/92151/diff/6001/6007
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/92151/diff/6001/6007#newcode300
Line 300: // Outside of annonymous namespace so that FRIEND_TEST can take
effect.
This comment is not needed. For consistency, none of TEST_F blocks should be in
the anonymous namespace.

[j.dinata, 2009-05-07 17:31:52]

Hi Pawel,

I sync up with the latest on this for easier merge.

Thanks

[Paweł Hajdan Jr., 2009-05-07 18:00:36]

I don't understand why you're moving tests around (for not exiting directory and
SubDirRecursive). Can you leave them where they are, so the diff will be easir
to read?

[j.dinata, 2009-05-12 03:34:13]

Hi,
This is reordering on directory_watcher_unittest.cc

Thanks

[Paweł Hajdan Jr., 2009-05-12 07:26:37]

LGTM with one nit, which I will fix in the version which will be landing.

http://codereview.chromium.org/92151/diff/8005/9019
File base/directory_watcher_unittest.cc (left):

http://codereview.chromium.org/92151/diff/8005/9019#oldcode395
Line 395: #endif  // defined(OS_WIN) || defined(OS_MACOSX)
Let's not move this line at this point. I will fix that.

[Paweł Hajdan Jr., 2009-05-14 17:52:06]

Landed in http://codereview.chromium.org/115229 and committed
http://src.chromium.org/viewvc/chrome?view=rev&revision=16070

Thanks for your patch!