Enable non-recursive watches for Windows DirectoryWatcher....

Enable non-recursive watches for Windows DirectoryWatcher.

- enable test for it on Windows
- clean up directory_watcher_unittest.cc

http://crbug.com/5072

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

[Paweł Hajdan Jr., 2009-03-19 12:03:41]

I tried to fix this issue two times, now it's the third.

Tested on Windows Vista, Mac and Linux. I should get a try result from Win XP
soon, but I think it should be ok.

[M-A Ruel, 2009-03-19 13:43:52]

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

http://codereview.chromium.org/42388/diff/1/3#newcode99
Line 99: void ConsumeExtraNotifications() {
Oh I don't like that...

but AFAIK it is only necessary on Windows Vista? If so, can you make it sleep
only on vista?

[Paweł Hajdan Jr., 2009-03-19 18:41:35]

On 2009/03/19 13:43:52, M-A wrote:
> http://codereview.chromium.org/42388/diff/1/3
> File base/directory_watcher_unittest.cc (right):
> 
> http://codereview.chromium.org/42388/diff/1/3#newcode99
> Line 99: void ConsumeExtraNotifications() {
> Oh I don't like that...

I'm not a fan of it either. Just couldn't find better solution. :-/

> but AFAIK it is only necessary on Windows Vista? If so, can you make it sleep
> only on vista?

Sure. Done.

[M-A Ruel, 2009-03-19 18:50:42]

lhtm

[M-A Ruel, 2009-03-19 19:04:12]

lgtm

[Paweł Hajdan Jr., 2009-03-20 16:44:14]

On 2009/03/19 18:41:35, Paweł Hajdan Jr. wrote:
> > but AFAIK it is only necessary on Windows Vista? If so, can you make it
sleep
> > only on vista?
> 
> Sure. Done.

It's not so good. git try tells me that without this sleep it fails on XP. What
do you think? I would then just check it in, with sleep only on Windows (but all
Windows versions).

[M-A Ruel, 2009-03-20 17:05:47]

On 2009/03/20 16:44:14, Paweł Hajdan Jr. wrote:
> On 2009/03/19 18:41:35, Paweł Hajdan Jr. wrote:
> > > but AFAIK it is only necessary on Windows Vista? If so, can you make it
> sleep
> > > only on vista?
> > 
> > Sure. Done.
> 
> It's not so good. git try tells me that without this sleep it fails on XP.
What
> do you think? I would then just check it in, with sleep only on Windows (but
all
> Windows versions).

Sure

[Paweł Hajdan Jr., 2009-03-21 19:19:51]

So the code only sleeps on Windows now, but I had to increase the timeout a lot
to reliably pass on trybot. What do you think?

I considered making this long part a separate test which would be DISABLED_, but
as it's all running on the trybot and there are already many slow tests in
base_unittests, this may be a not very big benefit.

[Paweł Hajdan Jr., 2009-03-23 20:15:50]

I sent this changeset to a trybot once again, just to be sure. Guess what?
SubDir test failed (I think Windows trybot is an XP).

This is my yet another attempt at enabling recursive watches on Windows
(disabled NonRecursive check on earlier versions). What do you think?

[M-A Ruel, 2009-03-23 20:28:56]

On 2009/03/23 20:15:50, Paweł Hajdan Jr. wrote:
> I sent this changeset to a trybot once again, just to be sure. Guess what?
> SubDir test failed (I think Windows trybot is an XP).
> 
> This is my yet another attempt at enabling recursive watches on Windows
> (disabled NonRecursive check on earlier versions). What do you think?

I'm fine with disabling one test on one OS and another test on another OS. I
also like separating SubDir in two.

I assume you wanted to say that "SubDirNonRecursive test failed" on your try
run. If so, disable it on Windows for now. Pretty sad but no short term fix. You
can remove ConsumeExtraNotifications() altogether.

[Paweł Hajdan Jr., 2009-03-24 14:57:17]

After our recent talk I changed the approach (trying to consume a fixed number
of notifications failed anyway). Now it isn't so hacky.

Passes on my Vista box and the trybot.

[M-A Ruel, 2009-03-24 15:13:54]

lgtm with the change in directory_watcher.h

http://codereview.chromium.org/42388/diff/4002/5003
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/42388/diff/4002/5003#newcode177
Line 177: // Create a test bile before the test. On Windows we get a
notification
file

Also, please document this limitation in directory_watcher.h.

[M-A Ruel, 2009-03-24 15:25:19]

lgtm