Add a FileWatcher to base/.

Add a FileWatcher to base/.

This is like Directory Watcher, but only for a single file.

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

[tony, 2010-03-05 05:30:12]

See the warnings in the header file for the limitations of this class.  Kind of
crappy, but maybe it's good enough?

[Paweł Hajdan Jr., 2010-03-05 08:53:20]

Generally looks good, just some comments.

By the way, feel free to consider passing the path to the delegate. That would
make it easier to watch multiple files in one object. Anyway, just please make
sure that the real behavior is consistent with comments in the code (I commented
in places where there're currently contradictory).

http://codereview.chromium.org/661359/diff/1055/1056
File base/DEPS (left):

http://codereview.chromium.org/661359/diff/1055/1056#oldcode7
base/DEPS:7: # Testing stuff shouldn't be used by the general base code.
It'd be better to preserve this, see my comments in the _unittest file.

http://codereview.chromium.org/661359/diff/1055/1059
File base/file_watcher.h (right):

http://codereview.chromium.org/661359/diff/1055/1059#newcode18
base/file_watcher.h:18: // WARNING: To be able to get create/delete
notifications and to work cross
Sounds fine to me.

http://codereview.chromium.org/661359/diff/1055/1059#newcode21
base/file_watcher.h:21: // WARNING: OSX's FSEventStreamCreate API doesn't tell
us which file in the
How about firing possibly spurious notifications? It seems less dangerous to me.

http://codereview.chromium.org/661359/diff/1055/1060
File base/file_watcher_inotify.cc (right):

http://codereview.chromium.org/661359/diff/1055/1060#newcode64
base/file_watcher_inotify.cc:64: base::hash_map<Watch, FilePath> paths_;
It seems to be unused now and it sounds very good to me. Could you remove this
member, and use watchers_ in places where we look into paths_?

http://codereview.chromium.org/661359/diff/1055/1060#newcode66
base/file_watcher_inotify.cc:66: // Lock to protect delegates_ and paths_.
Now this comment would need to be updated to say "Lock to protect watchers_".

http://codereview.chromium.org/661359/diff/1055/1061
File base/file_watcher_mac.cc (right):

http://codereview.chromium.org/661359/diff/1055/1061#newcode47
base/file_watcher_mac.cc:47: last_modified_ = base::Time();
Looks like this should make last_modified_.is_null() return true from now on.
Imagine that the file still doesn't exist, and we're getting more
FSEventsCallbacks. Looks like this path would always be taken, resulting in
spurious callbacks.

http://codereview.chromium.org/661359/diff/1055/1061#newcode56
base/file_watcher_mac.cc:56: // Path we're watching (passed to delegate).
nit: The path is no longer passed to the delegate.

http://codereview.chromium.org/661359/diff/1055/1063
File base/file_watcher_unittest.cc (right):

http://codereview.chromium.org/661359/diff/1055/1063#newcode46
base/file_watcher_unittest.cc:46: ASSERT_TRUE(PathService::Get(base::DIR_TEMP,
&path));
I think it would be better to just use ScopedTempDir.

http://codereview.chromium.org/661359/diff/1055/1064
File base/file_watcher_win.cc (right):

http://codereview.chromium.org/661359/diff/1055/1064#newcode31
base/file_watcher_win.cc:31: FileWatcher::Delegate* delegate_;
nit: Could you add empty lines between members?

http://codereview.chromium.org/661359/diff/1055/1064#newcode32
base/file_watcher_win.cc:32: // Path we're watching (passed to delegate).
nit: The path is not passed to the delegate now.

http://codereview.chromium.org/661359/diff/1055/1064#newcode88
base/file_watcher_win.cc:88: last_modified_ = base::Time();
Similarly to the mac watcher, shouldn't this null the time?

[tony, 2010-03-08 02:22:16]

http://codereview.chromium.org/661359/diff/1055/1056
File base/DEPS (left):

http://codereview.chromium.org/661359/diff/1055/1056#oldcode7
base/DEPS:7: # Testing stuff shouldn't be used by the general base code.
On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> It'd be better to preserve this, see my comments in the _unittest file.

Ok, I put it back.  There was one remaining call to DieFileDie, but I converted
it to just file_util::Delete.  It's probably sufficient.

http://codereview.chromium.org/661359/diff/1055/1059
File base/file_watcher.h (right):

http://codereview.chromium.org/661359/diff/1055/1059#newcode21
base/file_watcher.h:21: // WARNING: OSX's FSEventStreamCreate API doesn't tell
us which file in the
On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> How about firing possibly spurious notifications? It seems less dangerous to
me.

I don't think we'll fire any spurious notifications with this code because we
check the file timestamp.

http://codereview.chromium.org/661359/diff/1055/1060
File base/file_watcher_inotify.cc (right):

http://codereview.chromium.org/661359/diff/1055/1060#newcode64
base/file_watcher_inotify.cc:64: base::hash_map<Watch, FilePath> paths_;
On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> It seems to be unused now and it sounds very good to me. Could you remove this
> member, and use watchers_ in places where we look into paths_?

Done.

http://codereview.chromium.org/661359/diff/1055/1060#newcode66
base/file_watcher_inotify.cc:66: // Lock to protect delegates_ and paths_.
On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> Now this comment would need to be updated to say "Lock to protect watchers_".

Done.

http://codereview.chromium.org/661359/diff/1055/1061
File base/file_watcher_mac.cc (right):

http://codereview.chromium.org/661359/diff/1055/1061#newcode47
base/file_watcher_mac.cc:47: last_modified_ = base::Time();
On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> Looks like this should make last_modified_.is_null() return true from now on.
> Imagine that the file still doesn't exist, and we're getting more
> FSEventsCallbacks. Looks like this path would always be taken, resulting in
> spurious callbacks.

I think the !last_modified_.is_null() in this condition will prevent that from
happening.

http://codereview.chromium.org/661359/diff/1055/1063
File base/file_watcher_unittest.cc (right):

http://codereview.chromium.org/661359/diff/1055/1063#newcode46
base/file_watcher_unittest.cc:46: ASSERT_TRUE(PathService::Get(base::DIR_TEMP,
&path));
On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> I think it would be better to just use ScopedTempDir.

Done.

http://codereview.chromium.org/661359/diff/1055/1064
File base/file_watcher_win.cc (right):

http://codereview.chromium.org/661359/diff/1055/1064#newcode31
base/file_watcher_win.cc:31: FileWatcher::Delegate* delegate_;
On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> nit: Could you add empty lines between members?

Done.

[Paweł Hajdan Jr., 2010-03-08 08:02:04]

Just one more comment about checking the modification time. My understanding is
that when there are two modifications in less than a second, we may miss one of
them. How about changing the condition from current_timestamp != saved_timestamp
to current_timestamp != saved_timestamp || Time::Now() - saved_timestamp < 2s
(pseudocode-like)? This way we wouldn't miss any notification, and the
performance also shouldn't suffer since after two seconds the situation is "back
to normal".

http://codereview.chromium.org/661359/diff/1055/1059
File base/file_watcher.h (right):

http://codereview.chromium.org/661359/diff/1055/1059#newcode21
base/file_watcher.h:21: // WARNING: OSX's FSEventStreamCreate API doesn't tell
us which file in the
On 2010/03/08 02:22:16, tony wrote:
> On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> > How about firing possibly spurious notifications? It seems less dangerous to
> me.
> 
> I don't think we'll fire any spurious notifications with this code because we
> check the file timestamp.

I meant that firing a spurious notification seems to be less dangerous than
potentially missing one. If you're going to use it to reload a file when it
changes, the feature will be flaky if it can miss some notifications.

http://codereview.chromium.org/661359/diff/1055/1061
File base/file_watcher_mac.cc (right):

http://codereview.chromium.org/661359/diff/1055/1061#newcode47
base/file_watcher_mac.cc:47: last_modified_ = base::Time();
On 2010/03/08 02:22:16, tony wrote:
> On 2010/03/05 08:53:20, Paweł Hajdan Jr. wrote:
> > Looks like this should make last_modified_.is_null() return true from now
on.
> > Imagine that the file still doesn't exist, and we're getting more
> > FSEventsCallbacks. Looks like this path would always be taken, resulting in
> > spurious callbacks.
> 
> I think the !last_modified_.is_null() in this condition will prevent that from
> happening.

Indeed, you're right. I didn't get it earlier that base::Time().is_null() is
true.

[tony, 2010-03-08 11:46:31]

On 2010/03/08 08:02:04, Paweł Hajdan Jr. wrote:
> Just one more comment about checking the modification time. My understanding
is
> that when there are two modifications in less than a second, we may miss one
of
> them. How about changing the condition from current_timestamp !=
saved_timestamp
> to current_timestamp != saved_timestamp || Time::Now() - saved_timestamp < 2s
> (pseudocode-like)? This way we wouldn't miss any notification, and the
> performance also shouldn't suffer since after two seconds the situation is
"back
> to normal".

That's a good idea.  I updated the code to do this and updated the warning in
the header.  I had a few runs pass without the sleep in the unittest, so this
seems to work.

[Paweł Hajdan Jr., 2010-03-08 11:51:13]

Thanks, LGTM!