Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 6377: Add a DirectoryWatcher class, allowing objects to be notified whenever... (Closed)

Created:
12 years, 2 months ago by Evan Martin
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Add a DirectoryWatcher class, allowing objects to be notified whenever a directory's contents change. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=3504

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+321 lines, -7 lines) Patch
M base/build/base.vcproj View 1 2 3 3 chunks +13 lines, -5 lines 0 comments Download
M base/build/base_unittests.vcproj View 1 2 3 2 chunks +6 lines, -2 lines 0 comments Download
A base/directory_watcher.h View 1 2 3 1 chunk +42 lines, -0 lines 0 comments Download
A base/directory_watcher_unittest.cc View 1 2 3 1 chunk +174 lines, -0 lines 1 comment Download
A base/directory_watcher_win.cc View 1 2 3 1 chunk +86 lines, -0 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
Evan Martin
12 years, 2 months ago (2008-10-09 02:33:02 UTC) #1
Evan Martin
Missing parts: - needs to test unregistering - needs to test file mutation (in addition ...
12 years, 2 months ago (2008-10-09 02:34:09 UTC) #2
darin (slow to review)
I think it would be great to follow the pattern used by ObjectWatcher. Passing a ...
12 years, 2 months ago (2008-10-09 20:57:36 UTC) #3
Aaron Boodman
Seems good to me.
12 years, 2 months ago (2008-10-09 20:59:02 UTC) #4
cpu_(ooo_6.6-7.5)
http://codereview.chromium.org/6377/diff/1/2 File base/directory_watcher_win.cc (right): http://codereview.chromium.org/6377/diff/1/2#newcode29 Line 29: FindCloseChangeNotification(handle_); I would have to unregister the handle ...
12 years, 2 months ago (2008-10-09 23:17:05 UTC) #5
darin (slow to review)
http://codereview.chromium.org/6377/diff/1/2 File base/directory_watcher_win.cc (right): http://codereview.chromium.org/6377/diff/1/2#newcode48 Line 48: Yup, this should be using base::ObjectWatcher
12 years, 2 months ago (2008-10-09 23:18:56 UTC) #6
brettw
http://codereview.chromium.org/6377/diff/1/6 File base/directory_watcher.h (right): http://codereview.chromium.org/6377/diff/1/6#newcode5 Line 5: // This module lets you register interest in ...
12 years, 2 months ago (2008-10-10 00:10:52 UTC) #7
darin (slow to review)
http://codereview.chromium.org/6377/diff/1/6 File base/directory_watcher.h (right): http://codereview.chromium.org/6377/diff/1/6#newcode13 Line 13: class DirectoryWatcherImpl; any reason not to make DirectoryWatcherImpl ...
12 years, 2 months ago (2008-10-10 21:17:35 UTC) #8
Evan Martin
Ok, I think I've addressed all feedback, as well as fleshed out the tests. Please ...
12 years, 2 months ago (2008-10-14 20:24:14 UTC) #9
darin (slow to review)
http://codereview.chromium.org/6377/diff/12/215 File base/directory_watcher.h (right): http://codereview.chromium.org/6377/diff/12/215#newcode4 Line 4: please add a comment block up here that ...
12 years, 2 months ago (2008-10-15 00:02:13 UTC) #10
Evan Martin
Thanks for the detailed comments. I think I've fixed them all, and I also used ...
12 years, 2 months ago (2008-10-15 20:13:47 UTC) #11
darin (slow to review)
LGTM http://codereview.chromium.org/6377/diff/17/605 File base/directory_watcher.h (right): http://codereview.chromium.org/6377/diff/17/605#newcode18 Line 18: // directory. are there any promises you ...
12 years, 2 months ago (2008-10-15 20:19:34 UTC) #12
Evan Martin
http://codereview.chromium.org/6377/diff/17/605 File base/directory_watcher.h (right): http://codereview.chromium.org/6377/diff/17/605#newcode18 Line 18: // directory. On 2008/10/15 20:19:35, darin wrote: > ...
12 years, 2 months ago (2008-10-15 21:47:21 UTC) #13
Evan Martin
My unit test turned out to be flaky. I was doing: modify file; post a ...
12 years, 2 months ago (2008-10-17 01:12:03 UTC) #14
darin (slow to review)
12 years, 2 months ago (2008-10-17 02:43:51 UTC) #15
lgtm

http://codereview.chromium.org/6377/diff/609/22
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/6377/diff/609/22#newcode69
Line 69: MessageLoopForIO loop_;
I don't think DirectoryWatcher is limited to only running on a MessageLoopForIO
thread.  I think an ordinary MessageLoop should work just fine here.

Powered by Google App Engine
This is Rietveld 408576698