FilePathWatcher::Watch() - Listen for sub directory changes.

(1) Added a recursive boolean param to FilePathWatcher::Watch() function to watch for sub directory tree changes. Fixed all the calling sites.
(2) Added support to watch sub trees on Windows.
(3) Added FilePathWatcherTest.RecursiveWatch browser test.

BUG=144491
TEST=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171097

[kmadhusu, 2012-11-20 02:58:03]

mnissler@: Is there any specific reason why we are not listening for sub tree
changes? If this change is fine, I will add a unit test in the next patch.

Thanks.

[Lei Zhang, 2012-11-20 03:48:12]

On 2012/11/20 02:58:03, kmadhusu wrote:
> mnissler@: Is there any specific reason why we are not listening for sub tree
> changes? If this change is fine, I will add a unit test in the next patch.
> 
> Thanks.

Erm, that's because this code is used to implement FilePathWatcher, and
FilePathWatcher::Watch() is not recursive.

[Lei Zhang, 2012-11-20 03:53:03]

What we want to do is implement FilePathWatcher::RecursiveWatch() or change
FilePathWatcher::Watch() to take a recursive boolean. For unsupported platforms
/ paths, we would just return false. Thus we can implement this incrementally,
platform by platform.

On Windows, if someone calls "RecursiveWatch()", only then would we want to call
FindFirstChangeNotification() and tell it to watch subtrees.

[Mattias Nissler (ping if slow), 2012-11-20 09:25:16]

On 2012/11/20 03:53:03, Lei Zhang wrote:
> What we want to do is implement FilePathWatcher::RecursiveWatch() or change
> FilePathWatcher::Watch() to take a recursive boolean. For unsupported
platforms
> / paths, we would just return false. Thus we can implement this incrementally,
> platform by platform.
> 
> On Windows, if someone calls "RecursiveWatch()", only then would we want to
call
> FindFirstChangeNotification() and tell it to watch subtrees.

Agree. Alternatively, you could pass a bool recursive flag on construction time.
I don't care either way. Note that on windows, this is pretty simple (as you
figured), but on Linux and Mac it's going to be rather hard to get this right
(there are previous failed attempts at that).

[kmadhusu, 2012-11-21 01:41:33]

mnissler@, thestig@: Thanks for your suggestions.

Added a recursive boolean param to FilePathWatcher::Watch() and fixed all the
calling code. Added a browser test to verify the recursive watcher behavior on
windows. PTAL. Thanks.

[Lei Zhang, 2012-11-21 02:49:37]

On 2012/11/20 09:25:16, Mattias Nissler wrote:
> On 2012/11/20 03:53:03, Lei Zhang wrote:
> > What we want to do is implement FilePathWatcher::RecursiveWatch() or change
> > FilePathWatcher::Watch() to take a recursive boolean. For unsupported
> platforms
> > / paths, we would just return false. Thus we can implement this
incrementally,
> > platform by platform.
> > 
> > On Windows, if someone calls "RecursiveWatch()", only then would we want to
> call
> > FindFirstChangeNotification() and tell it to watch subtrees.
> 
> Agree. Alternatively, you could pass a bool recursive flag on construction
time.
> I don't care either way. Note that on windows, this is pretty simple (as you
> figured), but on Linux and Mac it's going to be rather hard to get this right
> (there are previous failed attempts at that).

Yes, but getting it on Windows gets this for a good portion of the users. For
media gallery's purposes, it's an optimization anyway.

[Lei Zhang, 2012-11-21 03:36:48]

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher.h:119: virtual bool Watch(const FilePath& path,
bool recursive, Delegate* delegate)
Do we need this? I don't think anyone will write new code with the deprecated
method and ask for recursive.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher.h:123: // called at most once, and from a
MessageLoop of TYPE_IO. Set |recursive| to
"Set |recursive| to true to watch the |path| and its children."

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:502: // Main directory("dir")
creation.
The rest of the test doesn't need to exist on !OS_WIN.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:506: // Create a file in the
directory("dir") tree.
These comments are clearer if you simply write:

Create "$dir/file1"

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher_win.cc (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_win.cc:74: bool recursive_watch_;
You need to initialize this.

[kmadhusu, 2012-11-21 04:13:26]

thestig@: Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher.h:119: virtual bool Watch(const FilePath& path,
bool recursive, Delegate* delegate)
On 2012/11/21 03:36:49, Lei Zhang wrote:
> Do we need this? I don't think anyone will write new code with the deprecated
> method and ask for recursive.

bool Watch(const FilePath& path, bool recursive, const Callback& callback) calls
this function.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher.h:123: // called at most once, and from a
MessageLoop of TYPE_IO. Set |recursive| to
On 2012/11/21 03:36:49, Lei Zhang wrote:
> "Set |recursive| to true to watch the |path| and its children."

Done.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:502: // Main directory("dir")
creation.
On 2012/11/21 03:36:49, Lei Zhang wrote:
> The rest of the test doesn't need to exist on !OS_WIN.

On !OS_WIN, code returns @line 500.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:506: // Create a file in the
directory("dir") tree.
On 2012/11/21 03:36:49, Lei Zhang wrote:
> These comments are clearer if you simply write:
> 
> Create "$dir/file1"

Done.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher_win.cc (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_win.cc:74: bool recursive_watch_;
On 2012/11/21 03:36:49, Lei Zhang wrote:
> You need to initialize this.

Done.

[Lei Zhang, 2012-11-21 04:21:02]

It would be good to test file modification too.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher.h:119: virtual bool Watch(const FilePath& path,
bool recursive, Delegate* delegate)
On 2012/11/21 04:13:26, kmadhusu wrote:
> On 2012/11/21 03:36:49, Lei Zhang wrote:
> > Do we need this? I don't think anyone will write new code with the
deprecated
> > method and ask for recursive.
> 
> bool Watch(const FilePath& path, bool recursive, const Callback& callback)
calls
> this function.

Maybe it's time to make the new Watch() method call impl_->Watch() directly?
We'll eventually do that anyway, since the old Watch() method will eventually go
away.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:502: // Main directory("dir")
creation.
On 2012/11/21 04:13:26, kmadhusu wrote:
> On 2012/11/21 03:36:49, Lei Zhang wrote:
> > The rest of the test doesn't need to exist on !OS_WIN.
> 
> On !OS_WIN, code returns @line 500.

Sure, now imagine what the code looks like if the OS_WIN code isn't there:

foo() {
  bar();
  return;
  qux();  // Why is this even here?
}

[Mattias Nissler (ping if slow), 2012-11-21 09:28:48]

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher.cc (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.cc:53: bool FilePathWatcher::Watch(const FilePath&
path, bool recursive,
parameter decls on separate lines

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.cc:66: bool FilePathWatcher::Watch(const FilePath&
path, bool recursive,
ditto

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.h:119: virtual bool Watch(const FilePath& path,
bool recursive, Delegate* delegate)
Don't add recursive here, this is the deprecated interface that new code should
not use.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.h:125: // the same loop. Returns true on success.
Please put a note saying that recursive watches are not supported on all
platforms and will CHECK on platforms that don't support it.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher_browsertest.cc:500: return;
Can't you put the entire test into include guards?

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher_kqueue.cc (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher_kqueue.cc:441: NOTIMPLEMENTED();
What's the effect of this at run time? I'd rather have something that CHECKS()
or at least DCHECKS() to avoid surprises.

[Lei Zhang, 2012-11-21 20:01:08]

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher_kqueue.cc (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher_kqueue.cc:441: NOTIMPLEMENTED();
On 2012/11/21 09:28:48, Mattias Nissler wrote:
> What's the effect of this at run time? I'd rather have something that CHECKS()
> or at least DCHECKS() to avoid surprises.

I don't think we want to put in CHECKs. Let's say we are on "foo OS" and foo OS
does not provide any way to do recursive watches. If we had a CHECK here, the
higher level code that wants to call FilePathWatcher would need to know they
cannot do so on foo OS. That makes FilePathWatcher harder to use.

As is, calls to FilePathWatcher on foo OS would just return false, and the
caller can handle the false case, which it should be doing anyway.

file_path_watcher.h should have clear documentation that the recursive flag may
fail on platforms, so callers know what to expect.

[kmadhusu, 2012-11-21 20:30:30]

On 2012/11/21 20:01:08, Lei Zhang wrote:
>
https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
> File base/files/file_path_watcher_kqueue.cc (right):
> 
>
https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
> base/files/file_path_watcher_kqueue.cc:441: NOTIMPLEMENTED();
> On 2012/11/21 09:28:48, Mattias Nissler wrote:
> > What's the effect of this at run time? I'd rather have something that
CHECKS()
> > or at least DCHECKS() to avoid surprises.
> 
> I don't think we want to put in CHECKs. Let's say we are on "foo OS" and foo
OS
> does not provide any way to do recursive watches. If we had a CHECK here, the
> higher level code that wants to call FilePathWatcher would need to know they
> cannot do so on foo OS. That makes FilePathWatcher harder to use.
> 
> As is, calls to FilePathWatcher on foo OS would just return false, and the
> caller can handle the false case, which it should be doing anyway.
> 
> file_path_watcher.h should have clear documentation that the recursive flag
may
> fail on platforms, so callers know what to expect.

On official build, NOTIMPLEMENTED() does nothing (stripped by compiler). 
On other builds, NOTIMPLEMENTED() displays a LOG(ERROR) message at runtime.

I don't want to crash the renderer by adding a CHECK()/DCHECK(). I will clearly
document these details in FilePathWatcher::Watch().

[kmadhusu, 2012-11-22 01:39:41]

FilePathWatcher browser test is extensively using the deprecated
FilePathWatcher::Watch()
function. I will submit a separate CL to fix those tests. Then I will fix the
new test.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher.h:119: virtual bool Watch(const FilePath& path,
bool recursive, Delegate* delegate)
On 2012/11/21 04:21:02, Lei Zhang wrote:
> On 2012/11/21 04:13:26, kmadhusu wrote:
> > On 2012/11/21 03:36:49, Lei Zhang wrote:
> > > Do we need this? I don't think anyone will write new code with the
> deprecated
> > > method and ask for recursive.
> > 
> > bool Watch(const FilePath& path, bool recursive, const Callback& callback)
> calls
> > this function.
> 
> Maybe it's time to make the new Watch() method call impl_->Watch() directly?
> We'll eventually do that anyway, since the old Watch() method will eventually
go
> away.

Done.

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/8003/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:502: // Main directory("dir")
creation.
On 2012/11/21 04:21:02, Lei Zhang wrote:
> On 2012/11/21 04:13:26, kmadhusu wrote:
> > On 2012/11/21 03:36:49, Lei Zhang wrote:
> > > The rest of the test doesn't need to exist on !OS_WIN.
> > 
> > On !OS_WIN, code returns @line 500.
> 
> Sure, now imagine what the code looks like if the OS_WIN code isn't there:
> 
> foo() {
>   bar();
>   return;
>   qux();  // Why is this even here?
> }
 
Will fix in the next patch.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher.cc (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.cc:53: bool FilePathWatcher::Watch(const FilePath&
path, bool recursive,
On 2012/11/21 09:28:48, Mattias Nissler wrote:
> parameter decls on separate lines

Done.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.cc:66: bool FilePathWatcher::Watch(const FilePath&
path, bool recursive,
On 2012/11/21 09:28:48, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.h:119: virtual bool Watch(const FilePath& path,
bool recursive, Delegate* delegate)
On 2012/11/21 09:28:48, Mattias Nissler wrote:
> Don't add recursive here, this is the deprecated interface that new code
should
> not use.

Done. Modified new code to call the impl_->Watch() directly.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher.h:125: // the same loop. Returns true on success.
On 2012/11/21 09:28:48, Mattias Nissler wrote:
> Please put a note saying that recursive watches are not supported on all
> platforms and will CHECK on platforms that don't support it.

Done.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher_browsertest.cc:500: return;
On 2012/11/21 09:28:48, Mattias Nissler wrote:
> Can't you put the entire test into include guards?

Browser test is extensively using the deprecated FilePathWatcher::Watch()
function. Once I fix the unit test, I will address this comment.

[Mattias Nissler (ping if slow), 2012-11-22 08:51:34]

I wasn't sure whether I should re-review, and then I understood you'd be porting
the test to callback-style-notifications in a pre-factoring CL, correct?

Here are my comments before understanding that FWIW.

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
File base/files/file_path_watcher_kqueue.cc (right):

https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
base/files/file_path_watcher_kqueue.cc:441: NOTIMPLEMENTED();
On 2012/11/21 20:01:09, Lei Zhang wrote:
> On 2012/11/21 09:28:48, Mattias Nissler wrote:
> > What's the effect of this at run time? I'd rather have something that
CHECKS()
> > or at least DCHECKS() to avoid surprises.
> 
> I don't think we want to put in CHECKs. Let's say we are on "foo OS" and foo
OS
> does not provide any way to do recursive watches. If we had a CHECK here, the
> higher level code that wants to call FilePathWatcher would need to know they
> cannot do so on foo OS. That makes FilePathWatcher harder to use.
> 
> As is, calls to FilePathWatcher on foo OS would just return false, and the
> caller can handle the false case, which it should be doing anyway.
> 
> file_path_watcher.h should have clear documentation that the recursive flag
may
> fail on platforms, so callers know what to expect.

All right, you convinced me that returning false is probably good enough.

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
File base/files/file_path_watcher.cc (right):

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
base/files/file_path_watcher.cc:53: bool FilePathWatcher::Watch(const FilePath&
path,
nit: no need to break the line.

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:129: *result =
watcher->Watch(target, recursive_watch, delegate);
this doesn't compile.

[kmadhusu, 2012-11-26 17:36:32]

On 2012/11/22 08:51:34, Mattias Nissler wrote:
> I wasn't sure whether I should re-review, and then I understood you'd be
porting
> the test to callback-style-notifications in a pre-factoring CL, correct?
> 

You are right. I will port all the tests in a separate CL and will let you know
once this CL is ready for review again.
 
> Here are my comments before understanding that FWIW.
> 
>
https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
> File base/files/file_path_watcher_kqueue.cc (right):
> 
>
https://codereview.chromium.org/11415066/diff/13001/base/files/file_path_watc...
> base/files/file_path_watcher_kqueue.cc:441: NOTIMPLEMENTED();
> On 2012/11/21 20:01:09, Lei Zhang wrote:
> > On 2012/11/21 09:28:48, Mattias Nissler wrote:
> > > What's the effect of this at run time? I'd rather have something that
> CHECKS()
> > > or at least DCHECKS() to avoid surprises.
> > 
> > I don't think we want to put in CHECKs. Let's say we are on "foo OS" and foo
> OS
> > does not provide any way to do recursive watches. If we had a CHECK here,
the
> > higher level code that wants to call FilePathWatcher would need to know they
> > cannot do so on foo OS. That makes FilePathWatcher harder to use.
> > 
> > As is, calls to FilePathWatcher on foo OS would just return false, and the
> > caller can handle the false case, which it should be doing anyway.
> > 
> > file_path_watcher.h should have clear documentation that the recursive flag
> may
> > fail on platforms, so callers know what to expect.
> 
> All right, you convinced me that returning false is probably good enough.
> 
>
https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
> File base/files/file_path_watcher.cc (right):
> 
>
https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
> base/files/file_path_watcher.cc:53: bool FilePathWatcher::Watch(const
FilePath&
> path,
> nit: no need to break the line.
> 
>
https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
> File base/files/file_path_watcher_browsertest.cc (right):
> 
>
https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
> base/files/file_path_watcher_browsertest.cc:129: *result =
> watcher->Watch(target, recursive_watch, delegate);
> this doesn't compile.

[kmadhusu, 2012-12-03 20:11:35]

Rebased and addressed review comments. Fixed browser tests. PTAL.

Thanks.

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
File base/files/file_path_watcher.cc (right):

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
base/files/file_path_watcher.cc:53: bool FilePathWatcher::Watch(const FilePath&
path,
On 2012/11/22 08:51:34, Mattias Nissler wrote:
> nit: no need to break the line.

Done.

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/1024/base/files/file_path_watch...
base/files/file_path_watcher_browsertest.cc:129: *result =
watcher->Watch(target, recursive_watch, delegate);
On 2012/11/22 08:51:34, Mattias Nissler wrote:
> this doesn't compile.

Fixed.

[Lei Zhang, 2012-12-03 21:44:39]

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
base/files/file_path_watcher.h:127: // NOTE: Recursive watch is not supported on
all platforms. Watch() will
Does it work on network shared on Windows? This may not be just per-platform.

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
File base/files/file_path_watcher_win.cc (right):

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
base/files/file_path_watcher_win.cc:77: bool recursive_watch_;
Can you just pass this to UpdateWatch() and SetupWatchHandle()? Unlike the other
variables, it doesn't seemed to be used anywhere else like in
OnObjectSignaled().

[kmadhusu, 2012-12-03 22:54:56]

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
base/files/file_path_watcher.h:127: // NOTE: Recursive watch is not supported on
all platforms. Watch() will
On 2012/12/03 21:44:39, Lei Zhang wrote:
> Does it work on network shared on Windows? This may not be just per-platform.

On Windows, notifications may not be returned when calling
FindFirstChangeNotification for a remote file system. (Ref:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364417(v=vs.85).aspx).
This note is very specific for a recursive_watch functionality. On windows, we
support recursive_watch but if the Windows API fails it will return false. Do
you want me to add all these details here?

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
File base/files/file_path_watcher_win.cc (right):

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
base/files/file_path_watcher_win.cc:77: bool recursive_watch_;
On 2012/12/03 21:44:39, Lei Zhang wrote:
> Can you just pass this to UpdateWatch() and SetupWatchHandle()? Unlike the
other
> variables, it doesn't seemed to be used anywhere else like in
> OnObjectSignaled().

UpdateWatch() is also called from FilePathWatcherImpl::OnObjectSignaled. That is
the reason, I didn't pass recursive_watch as param from
Watch()->UpdateWatch()->SetupWatchHandle().

[Lei Zhang, 2012-12-03 23:16:20]

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
base/files/file_path_watcher.h:127: // NOTE: Recursive watch is not supported on
all platforms. Watch() will
On 2012/12/03 22:54:57, kmadhusu wrote:
> On 2012/12/03 21:44:39, Lei Zhang wrote:
> > Does it work on network shared on Windows? This may not be just
per-platform.
> 
> On Windows, notifications may not be returned when calling
> FindFirstChangeNotification for a remote file system. (Ref:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364417%28v=vs.85%29...).
> This note is very specific for a recursive_watch functionality. On windows, we
> support recursive_watch but if the Windows API fails it will return false. Do
> you want me to add all these details here?

No, we should go into such details here, but I'm concerned this statement as
written makes readers think it's supported for all paths on a supported
platform.

[kmadhusu, 2012-12-04 00:06:36]

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/11415066/diff/6009/base/files/file_path_watch...
base/files/file_path_watcher.h:127: // NOTE: Recursive watch is not supported on
all platforms. Watch() will
On 2012/12/03 23:16:20, Lei Zhang wrote:
> On 2012/12/03 22:54:57, kmadhusu wrote:
> > On 2012/12/03 21:44:39, Lei Zhang wrote:
> > > Does it work on network shared on Windows? This may not be just
> per-platform.
> > 
> > On Windows, notifications may not be returned when calling
> > FindFirstChangeNotification for a remote file system. (Ref:
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364417%2528v=vs.85%...).
> > This note is very specific for a recursive_watch functionality. On windows,
we
> > support recursive_watch but if the Windows API fails it will return false.
Do
> > you want me to add all these details here?
> 
> No, we should go into such details here, but I'm concerned this statement as
> written makes readers think it's supported for all paths on a supported
> platform.

Rephrased the comment. Let me know your comments.

[Lei Zhang, 2012-12-04 00:28:46]

lgtm

[Mattias Nissler (ping if slow), 2012-12-04 09:27:35]

One test I'd like to see added, otherwise LGTM.

https://codereview.chromium.org/11415066/diff/14017/base/files/file_path_watc...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/14017/base/files/file_path_watc...
base/files/file_path_watcher_browsertest.cc:540: ASSERT_TRUE(WaitForEvents());
Can you also test whether file modifications get detected? I.e. add another
block in which you write subdir_file1 or child_dir_file1.

[kmadhusu, 2012-12-04 18:47:05]

https://codereview.chromium.org/11415066/diff/14017/base/files/file_path_watc...
File base/files/file_path_watcher_browsertest.cc (right):

https://codereview.chromium.org/11415066/diff/14017/base/files/file_path_watc...
base/files/file_path_watcher_browsertest.cc:540: ASSERT_TRUE(WaitForEvents());
On 2012/12/04 09:27:35, Mattias Nissler wrote:
> Can you also test whether file modifications get detected? I.e. add another
> block in which you write subdir_file1 or child_dir_file1.

Done. Added a block to write into child_dir_file1 and a block to modify
child_dir_file1 attributes.

[commit-bot: I haz the power, 2012-12-04 18:51:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11415066/13004

[commit-bot: I haz the power, 2012-12-04 18:51:10]

Presubmit check for 11415066-13004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    net
    remoting

Presubmit checks took 1.8s to calculate.

[kmadhusu, 2012-12-04 19:02:39]

szym@:  Can you do OWNER's check for net/ changes?
alexeypa@: Can you do OWNER's check for remoting/ changes?

Thanks.

[szym, 2012-12-04 19:15:48]

net/dns/ changes LGTM

[alexeypa (please no reviews), 2012-12-04 19:19:53]

remoting/* - lgtm

[commit-bot: I haz the power, 2012-12-04 19:30:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11415066/13004

[commit-bot: I haz the power, 2012-12-04 19:40:16]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 20:32:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11415066/13004

[commit-bot: I haz the power, 2012-12-05 00:36:41]

Change committed as 171097