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.

Based on patch by Janwar Dinata <j.dinata@gmail.com>; reviewed at http://codereview.chromium.org/92151

http://crbug.com/8968

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

[Paweł Hajdan Jr., 2009-05-12 07:29:25]

Evan, please do the final review, and also verify that Janwar Dinata has signed
the CLA.

[Evan Martin, 2009-05-12 18:27:54]

CLA is signed.

I'm uncomfortable with how many threads are involved in this.  I worry that on
startup we'll have n different threads all hammering the disk.  I think it would
be more consistent with our other code if there was just a single thread (or
even better, require the caller to pass in a MessageLoop* for running blocking
operations on).

Another way of putting this is: since Watch() itself blocks on the disk, maybe
it shouldn't use threads at all?

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

http://codereview.chromium.org/115229/diff/1/3#newcode59
Line 59: void EnsureSetupFinished() {
As a user of this class, it's unclear to me when I'd want to call this. 
Shouldn't Watch() just implicitly ensure setup is finished?

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

http://codereview.chromium.org/115229/diff/1/4#newcode100
Line 100: // Returns true, If watch for |path| has been added successfully.
Watches
s/, I/ i/

http://codereview.chromium.org/115229/diff/1/4#newcode111
Line 111: bool is_inode_watched(ino_t inode) const;
This should be IsInodeWatched().  We only do underscore_stype for simple
accessors.

http://codereview.chromium.org/115229/diff/1/4#newcode380
Line 380: NOTIMPLEMENTED();
Could you add comments as to what these cases mean?  E.g., when I see these
NOTIMPLEMENTEDs printing out warnings, what is the path to correcting them?

[Paweł Hajdan Jr., 2009-05-12 19:15:18]

On 2009/05/12 18:27:54, Evan Martin wrote:
> CLA is signed.
> 
> I'm uncomfortable with how many threads are involved in this.  I worry that on
> startup we'll have n different threads all hammering the disk.  I think it
would
> be more consistent with our other code if there was just a single thread (or
> even better, require the caller to pass in a MessageLoop* for running blocking
> operations on).

Hmm... I'll probably modify it to take a MessageLoop*.

> Another way of putting this is: since Watch() itself blocks on the disk, maybe
> it shouldn't use threads at all?

I think it should use threads. The difference is that the Windows version hits
the disk once. This implementation walks entire directory subtree, which may be
very expensive, even when on separate thread (but there is no better way with
inotify afaik).

> http://codereview.chromium.org/115229/diff/1/3
> File base/directory_watcher.h (right):
> 
> http://codereview.chromium.org/115229/diff/1/3#newcode59
> Line 59: void EnsureSetupFinished() {
> As a user of this class, it's unclear to me when I'd want to call this. 
> Shouldn't Watch() just implicitly ensure setup is finished?

I think that with this inotify implementation it would block for too long when
watching big subtrees. But after the change with passing MessageLoop* this
EnsureSetupFinished thing would disappear.

> http://codereview.chromium.org/115229/diff/1/4
> File base/directory_watcher_inotify.cc (right):
> 
> http://codereview.chromium.org/115229/diff/1/4#newcode100
> Line 100: // Returns true, If watch for |path| has been added successfully.
> Watches
> s/, I/ i/

Fixed.

> http://codereview.chromium.org/115229/diff/1/4#newcode111
> Line 111: bool is_inode_watched(ino_t inode) const;
> This should be IsInodeWatched().  We only do underscore_stype for simple
> accessors.

Fixed.

> http://codereview.chromium.org/115229/diff/1/4#newcode380
> Line 380: NOTIMPLEMENTED();
> Could you add comments as to what these cases mean?  E.g., when I see these
> NOTIMPLEMENTEDs printing out warnings, what is the path to correcting them?

What do you think about added comments?

[Evan Martin, 2009-05-12 19:42:22]

Looks good to me (except for the threads bit).

[Evan Martin, 2009-05-12 19:43:04]

+aaron

Aaron, Watch() is blocking on Windows, right?  Is there a plan to move that call
to the file thread?  How important is it that Watch() block until the whole tree
is being watched?

[Paweł Hajdan Jr., 2009-05-13 19:12:05]

This is sort of WIP, because in test code file_thread will be NULL or something.
I will fix that, but this is how the code would look like (backend_loop etc).
What do you think?

[Evan Martin, 2009-05-13 19:15:48]

Yeah, this looks good.  You can either pass in MessageLoop::current() for the
extra thread argument, or you can make NULL tell the watcher to use
MessageLoop::current() itself...

[Paweł Hajdan Jr., 2009-05-14 17:33:23]

Problem with NULL file_thread fixed, verified on trybot. Please look.

[Evan Martin, 2009-05-14 17:48:28]

LGTM

http://codereview.chromium.org/115229/diff/1032/25
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/115229/diff/1032/25#newcode273
Line 273: // TODO(aa): Enable this when DirectoryWatcher is implemented for
linux.
I guess this TODO can be fixed now!  Maybe in a differnt CL if you prefer.

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

On 2009/05/14 17:48:28, Evan Martin wrote:
> http://codereview.chromium.org/115229/diff/1032/25#newcode273
> Line 273: // TODO(aa): Enable this when DirectoryWatcher is implemented for
> linux.
> I guess this TODO can be fixed now!  Maybe in a differnt CL if you prefer.

The Linux recursive implementation is incomplete now. The code will now compile
and run, but I'd prefer to wait for complete recursive implementation.