Reland 56341 - Add support for watching directories to FileWatcher.

Reland 56341 - Add support for watching directories to FileWatcher.

BUG=none
TEST=Unit tests in file_watcher_unittest.cc.

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

[Mattias Nissler (ping if slow), 2010-08-17 16:54:00]

Due to a test failure on WinXP, I had to revert. Turns out that the MoveFileEx
status just isn't accurate, the only change from the original CL is the change
to the MoveParent unit test, which now explicitly verifies the original
directory is gone after moving it. OK to reland?

[tony, 2010-08-17 17:11:55]

LGTM

[Paweł Hajdan Jr., 2010-08-17 18:20:45]

Rubber-stamp LGTM

[Mattias Nissler (ping if slow), 2010-08-18 17:08:50]

On 2010/08/17 18:20:45, Paweł Hajdan Jr. wrote:
> Rubber-stamp LGTM

Sorry to bother you again with this. I've received a race report from thread
sanitizer caused by gmock's EXPECT_CALL not being thread safe. I've changed the
test delegate now to count calls in a thread safe fashion, it'd be great if one
of you could have another look.

I've also found another actual race in the windows implementation. It's similar
to mac and linux and fixed by implementing Cancel() properly, sorry I missed it
before. And then there seems to be just no reliable way of moving directories in
windows. So I've removed the ASSERT, if the move doesn't happen, the test will
break anyway.

[tony, 2010-08-18 18:05:48]

http://codereview.chromium.org/3184010/diff/6001/7005
File chrome/browser/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/3184010/diff/6001/7005#newcode431
chrome/browser/file_path_watcher_unittest.cc:431:
ASSERT_FALSE(file_util::PathExists(dir));
If the directory doesn't get created, will the asserts below succeed?  Should we
be checking the return value of file_util::Move?  Perhaps it's better to just
mark this test as flaky on Windows?

[Mattias Nissler (ping if slow), 2010-08-18 18:17:22]

http://codereview.chromium.org/3184010/diff/6001/7005
File chrome/browser/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/3184010/diff/6001/7005#newcode431
chrome/browser/file_path_watcher_unittest.cc:431:
ASSERT_FALSE(file_util::PathExists(dir));
On 2010/08/18 18:05:48, tony wrote:
> If the directory doesn't get created, will the asserts below succeed?  Should
we
> be checking the return value of file_util::Move?  Perhaps it's better to just
> mark this test as flaky on Windows?

The asserts below will succeed, since the directory will be already there and
that case is reported as success by CreateDirectory(). So it's safe to continue
the test.

Also note that the above block tries to test that moving the directory generates
notifications (which is important, since the watched file is now gone). If that
doesn't happen the test will fail, which is good.

And finally note that the return value of file_util::Move() seems to be reliable
on Win7, (tested for a few 1000 iterations of the test), but PathExists is not.
On WinXP, file_util::Move doesn't return reliable values. It returns
ERROR_INVALID_HANDLE every now and then, maybe that's got something to do with
the still existing watch. So there seems no portable way to assert the directory
is really gone. However, we don't really care, since the test will fail if
moving the directory doesn't alert the watcher.

With all that said, I believe it's most reasonable to just remove the ASSERT.
I've also run this in a loop for a couple of hours and didn't get failures.

[tony, 2010-08-18 18:20:53]

Ok, LGTM!