Implement recursive DirectoryWatcher watches on Linux

From phadjan's 3nd review

On directory_watcher_inotify.cc
Refactor:
	Move notification logic to DirectoryWatcherImpl.
	Run watchSubtree on a seperate thread.
	Track Inode, instead of FilePath.
	Fire Callback functions appropriately.

On directory_watcher_unittest.cc
	Remove redundant test.
	Fix MoveDirectoryAcrossWatchResursive.

http://crbug.com/8968

[Paweł Hajdan Jr., 2009-03-23 16:43:37]

Thanks again for working on this.

I'm a bit worried that you still did not add any tests for this new code.

Please also fix the issue description (it'll become part of the commit message).
And instead of Issue= say BUG= or http://crbug.com/<the-number> so it'll be
picked up by the commit bot.

And don't forget to actually send the review e-mail (Publish+Mail Comments) when
you post it and want me to review it. Note that this change will require a
review of at least one more Chromium developer, but as long as it still a work
in progress, it's fine with me as the only reviewer.

Did you sign the CLA?

http://codereview.chromium.org/52005/diff/1/2
File base/directory_watcher_inotify.cc (left):

http://codereview.chromium.org/52005/diff/1/2#oldcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Please retain the copyright notice.

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

http://codereview.chromium.org/52005/diff/1/2#newcode13
Line 13: 
No need to add these empty lines, here and above.

http://codereview.chromium.org/52005/diff/1/2#newcode256
Line 256: if (event->len) {
I don't like this big "if" here. Better inverse the condition and return early.

But I'm not sure why/if this is correct...

http://codereview.chromium.org/52005/diff/1/2#newcode271
Line 271: Watch watch;
I would put this declaration down to the places where it is used.

http://codereview.chromium.org/52005/diff/1/2#newcode283
Line 283: watch = Singleton<InotifyReader>::get()->AddWatch(newDirectory,
delegate);
This is a bit problematic because some of these delegates may have registered
for non-recursive notifications. You have to check for that.

And then you have to make sure that this "child" watch will get removed when the
parent watch gets removed.

http://codereview.chromium.org/52005/diff/1/2#newcode287
Line 287: // How do we pass inotify event from InotifyReader to
DirectoryWatcherImpl ?
Currently it is passed directly to the DirectoryWatcher::Delegate. Feel free to
modify it to something like InotifyReader -> DirectoryWatcherImpl ->
DirectoryWatcher::Delegate.

http://codereview.chromium.org/52005/diff/1/2#newcode291
Line 291: // When IN_CREATE event happens, need to call
DirectoryWatcherImpl->Watch to initiates a scan on the newly created FilePath.
I think you don't want to do this. The contract of DirectoryWatcherImpl says it
will watch only one path at a time (but the watch may be recursive).

So, you probably want to say in InotifyReader, that this new directory is also
associated with this DirectoryWatcherImpl, and ensure that on shutdown this
additional inotify watch will get removed.

http://codereview.chromium.org/52005/diff/1/2#newcode305
Line 305: if (watch != 0)
RemoveWatch doesn't return a watch, but a bool. Does it compile?

http://codereview.chromium.org/52005/diff/1/2#newcode316
Line 316: loop->PostTask(FROM_HERE,
This method got much bigger now. Please take the part above which deals with
recursive watches to a separate, private method.

http://codereview.chromium.org/52005/diff/1/2#newcode352
Line 352: path_ = path;
Setting these before the DCHECK could result in clobbering them when we are
already watching something and somebody calls Watch again. Please move these 2
lines from above to a place where we know that it's safe to make these
assignments.

http://codereview.chromium.org/52005/diff/1/2#newcode357
Line 357: // TODO(phajdan.jr): Support recursive watches.
You're working on this, so remove the TODO and rest of comments from here.

http://codereview.chromium.org/52005/diff/1/2#newcode370
Line 370: watch_ = Singleton<InotifyReader>::get()->AddWatch(subdirectory,
delegate_);
It is possible that before you get to this point, a watch will be added inside
of InotifyReader (because it should know it's recursive and sees new directory
added etc).

You should somehow protect against adding duplicate inotify watches for the same
DirectoryWatcher watch.

http://codereview.chromium.org/52005/diff/1/2#newcode371
Line 371: DCHECK(watch_ != InotifyReader::kInvalidWatch);
I think that DLOG would be more appropriate. The directory might just disappear
between line 368 and 370, and DCHECK is fatal (terminates the process if
compiled in debug mode).

[j.dinata, 2009-03-27 23:24:45]

Hi Pawel, 
This is my follow up on your first review.

I haven't done more on testing other than commenting out the part where it skips
recursive watching on base_unittests.

It passes base_unittests, so a step forward at minimum.

Thanks

[Paweł Hajdan Jr., 2009-03-28 19:01:11]

That's a progress, that's good.

You probably see a lot of comments here, but now you're also probably aware of
all difficulties with implementing these recursive watches, so it should be only
getting easier.

http://codereview.chromium.org/52005/diff/3001/3002
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/52005/diff/3001/3002#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
nit: it should only say 2009.

http://codereview.chromium.org/52005/diff/3001/3002#newcode41
Line 41: // |recursive| is true if subdirectory of path is to be watched also.
We have so many descriptions of |recursive| in other places
(directory_watcher.h) that IMHO it's not needed here.

http://codereview.chromium.org/52005/diff/3001/3002#newcode42
Line 42: // Otherwise |recursive| is false.
This otherwise part is not needed.

http://codereview.chromium.org/52005/diff/3001/3002#newcode43
Line 43: // |root_watch| takes the root watch pointer of |path| if exist,
This comment says that |root_watch| is a root watch... please expand it.

http://codereview.chromium.org/52005/diff/3001/3002#newcode47
Line 47: bool recusive, const Watch* root_watch);
nit: typo (recursive)

http://codereview.chromium.org/52005/diff/3001/3002#newcode51
Line 51: // |recursive| to remove watch recusively.
This gets more complicated now. Can we encode more things in Watch (maybe make
it a struct), and then RemoveWatch would only take a Watch? I think it makes
InotifyReader harder to misuse.

http://codereview.chromium.org/52005/diff/3001/3002#newcode58
Line 58: bool IsPathCoveredByWatch(Watch root_watch, const FilePath& path);
There is one problem with this approach. This works on path basis, while inotify
works on inode basis. I would rather try to answer question like "is this inode
covered by watch", but this can be done either by renaming the function, or just
doing it inside it. I'm happy with both choices.

http://codereview.chromium.org/52005/diff/3001/3002#newcode64
Line 64: class Event {
I don't like the way you created so many small classes here. I think the code
can be refactored to just add some methods to InotifyReader.

http://codereview.chromium.org/52005/diff/3001/3002#newcode73
Line 73: DISALLOW_COPY_AND_ASSIGN(Event);
nit: DISALLOW_COPY_AND_ASSIGN should be private.

http://codereview.chromium.org/52005/diff/3001/3002#newcode78
Line 78: explicit DirectoryCreationEvent(DirectoryWatcher::Delegate* delegate,
const FilePath* path, const Watch* root_watch) :
nit: explicit is not meaningful if ctor takes more than one parameter

http://codereview.chromium.org/52005/diff/3001/3002#newcode79
Line 79: path_(path), root_watch_(root_watch) {delegate_ = delegate;}
nit: why not initialize delegate_ in initializer list?

http://codereview.chromium.org/52005/diff/3001/3002#newcode106
Line 106: DISALLOW_COPY_AND_ASSIGN(DirectoryCloseWriteEvent);
nit: should be private

http://codereview.chromium.org/52005/diff/3001/3002#newcode113
Line 113: explicit DirectoryDeletionEvent(DirectoryWatcher::Delegate* delegate,
Watch wd):
nit: explicit is not needed here.

http://codereview.chromium.org/52005/diff/3001/3002#newcode114
Line 114: wd_(wd) {delegate_ = delegate;}
nit: why not in initializer list?

http://codereview.chromium.org/52005/diff/3001/3002#newcode124
Line 124: class DirectoryEventHandler {
It seems that this object is only created in InotifyReader and is very
short-lived. It also doesn't have much state except the inotify_event. Please
just move the handleEvent method to InotifyReader.

http://codereview.chromium.org/52005/diff/3001/3002#newcode128
Line 128: DirectoryEventHandler (inotify_event* event):event_(event){}
nit: explicit would be good here

http://codereview.chromium.org/52005/diff/3001/3002#newcode130
Line 130: const bool handleEvent(DirectoryWatcher::Delegate* delegate) const {
nit: instead of const bool you can say bool just as well.

http://codereview.chromium.org/52005/diff/3001/3002#newcode131
Line 131: //Only take care event that has name.
Please expand this comment (for example with a pointer to inotify doc). It would
be nice to more or less know here what does it mean that event has name etc.
without reading entire inotify doc.

http://codereview.chromium.org/52005/diff/3001/3002#newcode159
Line 159: return handled_ok;
nit: no need to store handled_ok, you can just return its value.

http://codereview.chromium.org/52005/diff/3001/3002#newcode191
Line 191: base::hash_map<Watch, WatchTable> recursive_watch_table_;
I wonder if it wouldn't be better to handle this data in DirectoryWatcherImpl.

http://codereview.chromium.org/52005/diff/3001/3002#newcode385
Line 385: base::hash_map<Watch, WatchTable>::iterator i =
recursive_watch_table_.find(watch);
Fun again. What happens if some directory creation event causes InotifyReader to
add another sub-watch for root_watch which is being removed?

By the way, it may be simpler to handle deletion in DirectoryWatcherImpl.

http://codereview.chromium.org/52005/diff/3001/3002#newcode422
Line 422: return subdirectories.find(path) != subdirectories.end();
I'm seriously worried about path canonicalization issues here.

http://codereview.chromium.org/52005/diff/3001/3002#newcode446
Line 446: DLOG(WARNING) << "New event is not handled: " << strerror(errno);
This check would be better if closer to the cause. It might turn out that it's
not needed then. But here it doesn't help much.

http://codereview.chromium.org/52005/diff/3001/3002#newcode453
Line 453: bool InotifyReader::IsRecursiveWatch(const Watch watch) {
hmm... I'm not sure about this name. Let's see how it will look like after all
these changes.

http://codereview.chromium.org/52005/diff/3001/3002#newcode476
Line 476: bool recursive_;
nit: A comment like "Watch entire subtree." would be good here. Just a
convention to briefly describe all members.

http://codereview.chromium.org/52005/diff/3001/3002#newcode490
Line 490: DCHECK(watch_ == InotifyReader::kInvalidWatch);
nit: you lost the comment "Can only watch one path."

http://codereview.chromium.org/52005/diff/3001/3002#newcode492
Line 492: delegate_ = delegate;
Again, only set rest of members *after* AddWatch succeeds. This applies to
delegate_, path_, and recursive_.

http://codereview.chromium.org/52005/diff/3001/3002#newcode498
Line 498: DLOG(WARNING) << "AddWatch failed: " << strerror(errno);
I don't think this DLOG is needed here.

http://codereview.chromium.org/52005/diff/3001/3002#newcode502
Line 502: if (recursive) {
This operation may take a lot of time and hit the disk a lot. I'm not yet sure
if it wouldn't be better to do this work on different thread, because it won't
affect our return value. However, it has other drawbacks, like the caller can't
be sure that all the watches are in place. Let's wait with this issue for now,
but please be aware of it. To properly solve it some changes to
DirectoryWatcher's public interface may be needed (like a callback to call when
all initial watches are set up).

http://codereview.chromium.org/52005/diff/3001/3002#newcode508
Line 508: if (Singleton<InotifyReader>::get()->IsPathCoveredByWatch(watch_,
subdirectory))
Looks almost good now, but I think there is still a race condition. Imagine a
flow like this:

1. You get a subdirectory A, which is not yet covered by the watch.
2. You get past this check, and get ready to call AddWatch.
3. InotifyReader receives creation event for subdirectory A.
4. Now you have two watches for A.
5. That's bad, but adding a Lock should solve the problem.

http://codereview.chromium.org/52005/diff/3001/3002#newcode512
Line 512: DLOG(WARNING) << "AddWatch failed: " << strerror(errno);
I'm not sure if errno is meaningful here. But including |subdirectory| in the
message seems useful.

http://codereview.chromium.org/52005/diff/3001/3003
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/52005/diff/3001/3003#newcode160
Line 160: /*
Just go ahead and remove this part of code. No need to comment out, you have svn
or something.

http://codereview.chromium.org/52005/diff/3001/3003#newcode178
Line 178: //verify get notification on dir merge on recursive watch
Yeah, these are good ideas to test. But I think we also need to test more
complicated scenarios, like moving directory across watches. Just a note for the
future.

[j.dinata, 2009-04-04 21:44:46]

Hi Pawel, 
I have updates from your 2'nd review. 

I am hitting an intermittent problems on
ConditionVariableTest.StartupShutdownTest. It's something about the
MessagePump*.

When I run DirectoryWatcherTest.* individually it looks ok but only when I run
the whole base_unittest then I am seeing this problem.

I can mail you gdb traces, if you're interested.

[Paweł Hajdan Jr., 2009-04-05 10:33:05]

Many comments, but I see the progress and I think it's going in good direction.

Don't worry about ConditionVariable tests here. You can always run with
--gtest_filter=DirectoryWatcher*, at least for now, when nothing else in base/
uses DirectoryWatcher.

http://codereview.chromium.org/52005/diff/4001/4002
File base/directory_watcher_inotify.cc (left):

http://codereview.chromium.org/52005/diff/4001/4002#oldcode13
Line 13: 
nit: please don't remove these empty lines, see
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...>

http://codereview.chromium.org/52005/diff/4001/4002#oldcode18
Line 18: 
nit: here too

http://codereview.chromium.org/52005/diff/4001/4002
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/52005/diff/4001/4002#newcode29
Line 29: class DirectoryWatcherImpl;
nit: add empty line above.

http://codereview.chromium.org/52005/diff/4001/4002#newcode36
Line 36: typedef std::set<Watch> WatchSet; //Set of Watch.
Can this typedef be private? Also the comment is redundant.

http://codereview.chromium.org/52005/diff/4001/4002#newcode41
Line 41: typedef base::hash_map<FilePath, Watch> WatchTable;
This typedef is now not used anywhere. That's good, so remove it.

http://codereview.chromium.org/52005/diff/4001/4002#newcode43
Line 43: // Watch |path| for changes. |directory_watcher_impl| will be notified
on each change. Does
Lat's call the parameter |watcher| instead of longer |directory_watcher_impl|.

http://codereview.chromium.org/52005/diff/4001/4002#newcode53
Line 53: bool RemoveWatch(Watch watch, DirectoryWatcherImpl*
directory_watcher_impl);
Just |watcher|.

http://codereview.chromium.org/52005/diff/4001/4002#newcode61
Line 61: typedef std::pair<DirectoryWatcherImpl*, MessageLoop*>
DirectoryWatcherImplInfo;
Again, let's shorten the names here and in line below: WatcherInfo instead of
DirectoryWatcherImplInfo.

http://codereview.chromium.org/52005/diff/4001/4002#newcode62
Line 62: typedef std::multiset<DirectoryWatcherImplInfo>
DirectoryWatcherImplSet;
Just WatcherSet...

http://codereview.chromium.org/52005/diff/4001/4002#newcode71
Line 71: base::hash_map<Watch, DirectoryWatcherImplSet>
directory_watcher_impls_;
Just watchers_.

http://codereview.chromium.org/52005/diff/4001/4002#newcode105
Line 105: DirectoryWatcher::Delegate* GetDelegate_() const;
Now that's bad. It would be much better for users of DirectoryWatcherImpl to
tell it what it should do with the delegate (probably notify it), than just take
the delegate and to whatever with it. The logic of notification should live
*here*, in DirectoryWatcherImpl, not elsewhere.

http://codereview.chromium.org/52005/diff/4001/4002#newcode108
Line 108: void OnDirectoryCreation(const FilePath& path, InotifyReader::Watch
watch);
These callbacks should be made private, and see my comments in InotifyReader.

http://codereview.chromium.org/52005/diff/4001/4002#newcode169
Line 169: // Flag set to true when startup was successful.
Oops. This comment is plain wrong.

http://codereview.chromium.org/52005/diff/4001/4002#newcode246
Line 246: InotifyReaderNotifyTask(DirectoryWatcherImpl* directory_watcher_impl,
Wait a moment. We want to fire callback for DirectoryWatcher::Delegate* on the
same thread that DirectoryWatcher::Watch was called. That's okay because then
the callback setter is responsible for ensuring that the callback doesn't take
too much time etc. However, we don't want the additional code in
DirectoryWatcherImpl to run on this thread, because it will most probably hit
the disk, etc. etc. So, DirectoryWatcherImpl callback should be fired on
InotifyReader thread, and DirectoryWatcher::Delegate callback should be fired on
DirectoryWatcher::Watch thread.

http://codereview.chromium.org/52005/diff/4001/4002#newcode298
Line 298: const FilePath& path, DirectoryWatcherImpl* directory_watcher_impl,
bool recursive) {
Well, now InotifyReader has no need to know about recursive or not recursive
type of watch. I want it to be as dumb as possible, just reading things from the
buffer and passing it to smarter entities.

http://codereview.chromium.org/52005/diff/4001/4002#newcode335
Line 335: recursive_watch_.erase(watch);
That would erase all instances (it's a multiset) of |watch|.

http://codereview.chromium.org/52005/diff/4001/4002#newcode341
Line 341: directory_watcher_impls_[watch].erase(a_directory_watcher_impl);
Why not just ....erase(*it) and you don't need the line above.

http://codereview.chromium.org/52005/diff/4001/4002#newcode360
Line 360: // refers to the size of char name array.
Hm. The comment now is longer, but does it really give me more info? The
"According to manpage" part is not needed. In fact, I want to know what "name"
*is*, and I still don't know from this comment (and have to refer to the
manpage). Please briefly describe what |name| *means*, not that there is a
parameter named |name|. Then just say that |len| is length of |name|; the
uint32_t part is unnecessary.

http://codereview.chromium.org/52005/diff/4001/4002#newcode364
Line 364: DirectoryWatcherImplSet directory_watcher_impls_to_notify;
watchers_to_notify?

http://codereview.chromium.org/52005/diff/4001/4002#newcode378
Line 378: if ((event->mask & IN_ISDIR)) {
Now this is kind of logic which belongs to DirectoryWatcherImpl, and not dumb
InotifyReader.

http://codereview.chromium.org/52005/diff/4001/4002#newcode408
Line 408: DCHECK(file_util::AbsolutePath(&absolute_path));
Ooops. DCHECK is a macro, and the code inside won't execute in release mode.

http://codereview.chromium.org/52005/diff/4001/4002#newcode410
Line 410: InotifyReader::Watch child_watch = InotifyReader::kInvalidWatch;
That's just moving the code from InotifyReader to DirectoryWatcherImpl. That's
good, but I want to go further: create *child* DirectoryWatcherImpls here,
instead of trying to keep track of whole subtree in a complicated way.

Also, you should keep track of *inodes* instead of paths, because that's how
inotify operates. It should be much safer.

http://codereview.chromium.org/52005/diff/4001/4002#newcode507
Line 507: DCHECK(file_util::AbsolutePath(&absolute_path));
Code inside DCHECK won't execute in release mode. It's a macro.

http://codereview.chromium.org/52005/diff/4001/4002#newcode511
Line 511: DLOG(WARNING) << "Watch " << path.value() << " failed: " <<
strerror(errno);
No need to DLOG here. Returning false is sufficient.

http://codereview.chromium.org/52005/diff/4001/4002#newcode526
Line 526: result = watchSubdirectory(root_watch_, root_path_);
If this returns false, then the caller "thinks" that there is no watch
established. That's not true, and that's bad. You could remove the subdir
watches here if something goes bad, but that's complicated. Let's say that
watchSubdirectory result should have no effect on Watch result.

http://codereview.chromium.org/52005/diff/4001/4002#newcode531
Line 531: bool DirectoryWatcherImpl::watchSubdirectory(InotifyReader::Watch
root_watch,
Maybe better name: WatchSubtree. I think that in final code this should run on a
spearate thread. DirectoryWatcher::Watch should not be very expensive, and
walking the subtree is.

http://codereview.chromium.org/52005/diff/4001/4002#newcode547
Line 547: return false;
Don't return here. There was one failure, and it's good that we have a DLOG for
it, but then we wouldn't add watches for remaining subdirs.

See also my note above.

http://codereview.chromium.org/52005/diff/4001/4003
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/52005/diff/4001/4003#newcode218
Line 218: TEST_F(DirectoryWatcherTest, DeleteDuringNotifyNonRecursive) {
No need to separate Recursive/NonRecursive part here. I did that for SubDir test
so I can return early more easily.

http://codereview.chromium.org/52005/diff/4001/4003#newcode248
Line 248: TEST_F(DirectoryWatcherTest, MultipleWatchersSingleFileNonRecursive) {
Again, I would just add a recursive watch somewhere into the mix.

http://codereview.chromium.org/52005/diff/4001/4003#newcode327
Line 327: // Move Subdir2 to be a child of SubDir, while
That's not what this test does. It creates the final tree structure from the
start. It should create above tree initially, set up the watchers, then move one
subdir, and then verify the rest of checks.

[j.dinata, 2009-04-06 21:20:11]

Hi Pawel,
Here is another iteration.

http://codereview.chromium.org/52005/diff/4001/4002
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/52005/diff/4001/4002#newcode410
Line 410: InotifyReader::Watch child_watch = InotifyReader::kInvalidWatch;
On 2009/04/05 10:33:05, Paweł Hajdan Jr. wrote:
> That's just moving the code from InotifyReader to DirectoryWatcherImpl. That's
> good, but I want to go further: create *child* DirectoryWatcherImpls here,
> instead of trying to keep track of whole subtree in a complicated way.

Please elaborate more on this point. What do you mean by child
DirectoryWatcherImpl? I don't see why keeping track of subtree is not needed.

> Also, you should keep track of *inodes* instead of paths, because that's how
> inotify operates. It should be much safer.

[Paweł Hajdan Jr., 2009-04-14 15:19:00]

Some comments. There are still some issues I didn't comment about, but this
patch already took very long to produce, and I think it would be better to
change the approach a bit.

Please upload a separate review issue that would allow recursive watches, but
without support for dynamically adding/removing watches upon directory changes
(events from InotifyReader). That should be simple, and I think that patch can
be landed quickly. Then we will see what can be done about the rest.

http://codereview.chromium.org/52005/diff/6003/6004
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/52005/diff/6003/6004#newcode47
Line 47: Watch AddWatch(const FilePath& path, DirectoryWatcherImpl* watcher,
bool recursive);
Now the bool recursive parameter is not needed. Good!

http://codereview.chromium.org/52005/diff/6003/6004#newcode60
Line 60: typedef std::multiset<DirectoryWatcherImpl*> WatcherSet;
Now some assumptions changed, so it doesn't have to be multiset. Let's change to
std::set...

http://codereview.chromium.org/52005/diff/6003/6004#newcode66
Line 66: // Multiset is used because there may be many DirectoryWatcherImpls for
same path
And now this comment can be removed.

http://codereview.chromium.org/52005/diff/6003/6004#newcode96
Line 96: void handleEvent (inotify_event* event, FilePath changed_path);
nit: should be HandleEvent. Also please remove the changed_path parameter.

http://codereview.chromium.org/52005/diff/6003/6004#newcode110
Line 110: // Callback for InotifyReader to tell about new directory creation.
This comment is no longer true.

http://codereview.chromium.org/52005/diff/6003/6004#newcode113
Line 113: // Callback for InotifyReader to tell about new directory deletion.
As above.

http://codereview.chromium.org/52005/diff/6003/6004#newcode176
Line 176: const FilePath& path)
nit: please align this line

http://codereview.chromium.org/52005/diff/6003/6004#newcode350
Line 350: if (!event->len)
This check is now not needed here.

http://codereview.chromium.org/52005/diff/6003/6004#newcode378
Line 378: ino_t DirectoryWatcherImpl::getInode(const FilePath path) const {
I'm not sure if this function shouldn't be moved elsewhere, but let's leave it
for now.

http://codereview.chromium.org/52005/diff/6003/6004#newcode380
Line 380: int result=stat(path.value().c_str(), &buffer);
nit: spaces around "="
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...

http://codereview.chromium.org/52005/diff/6003/6004#newcode382
Line 382: DLOG(WARNING) << " Failed to stat: " << path.value() <<
strerror(errno);
I would prefer a signature more like
bool ...::getInode(const FilePath& path, ino_t* inode) const;

Then it would return true on success, which gives us explicit error handling.

http://codereview.chromium.org/52005/diff/6003/6004#newcode391
Line 391: new DirectoryWatcherImplNotifyTask(delegate_, changed_path));
changed_path should be the same path that was used for Watch. That's the
contract of DirectoryWatcher. The delegate receives the path that was being
watched, not necessarily the more specific path inside the subtree.

Then you don't need the changed_path parameter.

http://codereview.chromium.org/52005/diff/6003/6004#newcode396
Line 396: if (event->mask & IN_CLOSE_WRITE) {
Looks like this if branch is not needed. IN_CREATE, IN_DELETE and IN_CLOSE_WRITE
seem to be mutually exclusive.

http://codereview.chromium.org/52005/diff/6003/6004#newcode398
Line 398: event->mask & IN_MOVE && event->mask & IN_MOVED_TO) {
IN_MOVE includes IN_MOVED_TO

http://codereview.chromium.org/52005/diff/6003/6004#newcode399
Line 399: if (recursive_) {
I would add a DCHECK here that event->name is not empty.

http://codereview.chromium.org/52005/diff/6003/6004#newcode404
Line 404: event->mask & IN_MOVE && event->mask & IN_MOVED_FROM) {
IN_MOVE includes IN_MOVED_FROM

http://codereview.chromium.org/52005/diff/6003/6004#newcode409
Line 409: bool DirectoryWatcherImpl::isPathCovered(const FilePath& path) const {
nit: first character should be upcase too, so IsPathCovered... or maybe better
IsPathWatched.

http://codereview.chromium.org/52005/diff/6003/6004#newcode410
Line 410: return path_covered_.find(getInode(path))!=path_covered_.end();
nit: spaces around "!=".

http://codereview.chromium.org/52005/diff/6003/6004#newcode415
Line 415: if (!recursive_)
Do DCHECK(recursive) instead. The caller makes that check, and we want to be
sure that it does it correctly.

http://codereview.chromium.org/52005/diff/6003/6004#newcode419
Line 419: if (!absolute_path.IsAbsolute()) {
This check is not needed. We actually don't need this absolute_path, because now
we use inodes.

http://codereview.chromium.org/52005/diff/6003/6004#newcode429
Line 429: if (child_watch != InotifyReader::kInvalidWatch)
DLOG if couldn't add child_watch

http://codereview.chromium.org/52005/diff/6003/6004#newcode512
Line 512: if (!absolute_path.IsAbsolute()) {
This absolute path part is not needed.

http://codereview.chromium.org/52005/diff/6003/6004#newcode546
Line 546: base::Thread thread("traverse_subtree");
Oh, that's bad. Actually no code runs on this thread, and it will be stopped on
function exit. Better use WorkerPool and extract FileEnumerator part of this
method into a separate Task.

http://codereview.chromium.org/52005/diff/6003/6004#newcode552
Line 552: bool no_failure = true;
bool success would be much more intuitive.