kqueue implementation of FilePathWatcher on Mac.

kqueue implementation of FilePathWatcher on Mac.


BUG=54822, 77391, 77217, 77064, 69641
TEST=BUILD

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

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

[dmac, 2011-03-29 21:02:10]

OK, so back to kqueues we go. PTAL.

[Mark Mentovai, 2011-03-29 21:27:24]

FD leak concern. Also, that long OnCanRead function is a little confusing.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
File content/common/file_path_watcher/file_path_watcher.h (right):

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher.h:24: // details.
Thanks for revising this comment to match the new reality.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:65: virtual void
CancelOnMessageLoopThread() OVERRIDE;
This deserves a comment. Does it mean that it can be called fro any thread and
asks for work to be done on MessageLoopThread, or that it must be called from
MessageLoopThread?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:73: static void
ReleaseEvent(struct kevent& event);
It’s unorthodox in our style to pass a non-const reference. Can this be a
pointer?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:122: fd =
FileDescriptorForPath(built_path);
Do you have to worry about closing these ever? You only seem to close them in
some cases.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:134: | NOTE_RENAME |
NOTE_REVOKE | NOTE_EXTEND), 0, data);
Put the operator at the end of the preceding line instead of the beginning of
this one. This is a Chromium-only requirement, not part of standard Google
style.

http://www.chromium.org/developers/coding-style#Code_Formatting

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:160: int count =
kevent(kqueue_, NULL, 0, &updates[0], updates.size(), &timeout);
HANDLE_EINTR.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:196:
reinterpret_cast<EventData*>(next_event->udata);
Continuation lines are indented by 4.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:222: count =
kevent(kqueue_, &events_[0], valid, NULL, 0, NULL);
HANDLE_EINTR (mostly just paranoia in this case).

[Mattias Nissler (ping if slow), 2011-03-30 11:30:52]

First round of comments.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:35: // that the
FSEvents impl would miss.
Maybe also mention that FSEvents needs a runloop backed MessageLoop, so it adds
additional complexity.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:63: typedef
std::vector<struct kevent> EventVector;
Ah, does this mean we consume one file descriptor per path component? Is this
acceptable?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:80: static void
CloseFileDescriptor(int *fd);
use int* instead of int *

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:98: int
FilePathWatcherImpl::EventsForPath(FilePath path, EventVector *events) {
Asterisk by the type.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:147: }
No need for curlies for single-line conditionals (also at other places), but if
you like them, keep them :)

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:155: void
FilePathWatcherImpl::OnFileCanReadWithoutBlocking(int fd) {
This is a very long function that handles separate aspects, i.e. detecting
events and updating watches. Maybe break it up a bit?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:158:
std::vector<struct kevent> updates(events_.size());
Maybe a scoped_array is more appropriate?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:176: CHECK(event !=
events_.end());
trusting the kernel to only pass valid fds? We've been bitten quite a few times
on windows because of this, so you may want to switch this over to
check-NOTREACHED()-continue.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:222: count =
kevent(kqueue_, &events_[0], valid, NULL, 0, NULL);
Don't we need EV_ERROR checking for the responses?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:230: for(; valid <
events_.size(); ++valid) {
space after for

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:278:
std::vector<struct kevent> responses(last_entry);
a scoped_array might be more appropriate here?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:283: return false;
Do we need to close the directory FDs when we hit an error here and below?

[Mark Mentovai, 2011-03-30 13:59:08]

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:158:
std::vector<struct kevent> updates(events_.size());
Mattias Nissler wrote:
> Maybe a scoped_array is more appropriate?

No, std::vector is. :)

[Mattias Nissler (ping if slow), 2011-03-30 14:07:52]

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:158:
std::vector<struct kevent> updates(events_.size());
On 2011/03/30 13:59:08, Mark Mentovai wrote:
> Mattias Nissler wrote:
> > Maybe a scoped_array is more appropriate?
> 
> No, std::vector is. :)

Can you give reasons? He doesn't use any vector-specific goodness, so what's bad
about a simple heap-allocated array (which is what vector alos does behind the
scenes)?

[Mark Mentovai, 2011-03-30 14:14:47]

mnissler@chromium.org wrote:
> Can you give reasons? He doesn't use any vector-specific goodness, so
> what's bad about a simple heap-allocated array (which is what vector
> alos does behind the scenes)?

Right, so prefer the standard solution to some made-up equivalent.

[Mattias Nissler (ping if slow), 2011-03-30 14:32:03]

On Wed, Mar 30, 2011 at 4:14 PM, Mark Mentovai <mark@chromium.org> wrote:

> mnissler@chromium.org wrote:
> > Can you give reasons? He doesn't use any vector-specific goodness, so
> > what's bad about a simple heap-allocated array (which is what vector
> > alos does behind the scenes)?
>
> Right, so prefer the standard solution to some made-up equivalent.
>

I wouldn't consider a standard plain array and the usual scoped_array from
base some made-up equivalent.

[dmaclach1, 2011-03-30 16:41:35]

My reason for using vector over scoped_array was that I use a vector
elsewhere (and use the vector specific goodness) so it seemed to make
sense to only generate code for one template. Is there any advantage
to scoped_array I am unaware of?

Cheers,
Dave

On Wed, Mar 30, 2011 at 7:31 AM, Mattias Nissler <mnissler@chromium.org> wrote:
>
>
> On Wed, Mar 30, 2011 at 4:14 PM, Mark Mentovai <mark@chromium.org> wrote:
>>
>> mnissler@chromium.org wrote:
>> > Can you give reasons? He doesn't use any vector-specific goodness, so
>> > what's bad about a simple heap-allocated array (which is what vector
>> > alos does behind the scenes)?
>>
>> Right, so prefer the standard solution to some made-up equivalent.
>
> I wouldn't consider a standard plain array and the usual scoped_array from
> base some made-up equivalent.

[Mark Mentovai, 2011-03-30 16:53:54]

Mattias Nissler wrote:
> I wouldn't consider a standard plain array and the usual scoped_array from
> base some made-up equivalent.

Standard plain arrays certainly aren’t made-up.

David Maclachlan wrote:
> My reason for using vector over scoped_array was that I use a vector
> elsewhere (and use the vector specific goodness) so it seemed to make
> sense to only generate code for one template. Is there any advantage
> to scoped_array I am unaware of?

The only real advantage to scoped_array that I see is when you’re
interfacing with some other code that you don’t control that adheres
to new[]/delete[] semantics. For example, if a function you can’t
change returns something to you that’s been allocated with new[], or
if you need to build up your own structures in memory and then pass
them along to other code that will assume ownership and delete it with
delete[], scoped_array is a winner. For cases where you control all of
the code yourself (like this one), you can mandate your own allocation
and deallocation policy, and there’s no reason to not delegate these
operations to std::vector.

[Mattias Nissler (ping if slow), 2011-03-30 16:57:13]

I don't see any functional difference between the two in this case, so I
guess I'm happy with either. The comment I just put because when I read it I
thought that since all you need an array you could actual directly use one.
Please go with whatever you like.

On Wed, Mar 30, 2011 at 6:53 PM, Mark Mentovai <mark@chromium.org> wrote:

> Mattias Nissler wrote:
> > I wouldn't consider a standard plain array and the usual scoped_array
> from
> > base some made-up equivalent.
>
> Standard plain arrays certainly aren’t made-up.
>
> David Maclachlan wrote:
> > My reason for using vector over scoped_array was that I use a vector
> > elsewhere (and use the vector specific goodness) so it seemed to make
> > sense to only generate code for one template. Is there any advantage
> > to scoped_array I am unaware of?
>
> The only real advantage to scoped_array that I see is when you’re
> interfacing with some other code that you don’t control that adheres
> to new[]/delete[] semantics. For example, if a function you can’t
> change returns something to you that’s been allocated with new[], or
> if you need to build up your own structures in memory and then pass
> them along to other code that will assume ownership and delete it with
> delete[], scoped_array is a winner. For cases where you control all of
> the code yourself (like this one), you can mandate your own allocation
> and deallocation policy, and there’s no reason to not delegate these
> operations to std::vector.
>

[dmac, 2011-03-30 17:18:47]

Thanks guys. Ball is back in your court(s)

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:35: // that the
FSEvents impl would miss.
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> Maybe also mention that FSEvents needs a runloop backed MessageLoop, so it
adds
> additional complexity.

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:63: typedef
std::vector<struct kevent> EventVector;
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> Ah, does this mean we consume one file descriptor per path component? Is this
> acceptable?

That's kind of a requirement for the kqueue implementation, unless you have a
solution to it?

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:65: virtual void
CancelOnMessageLoopThread() OVERRIDE;
On 2011/03/29 21:27:25, Mark Mentovai wrote:
> This deserves a comment. Does it mean that it can be called fro any thread and
> asks for work to be done on MessageLoopThread, or that it must be called from
> MessageLoopThread?

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:73: static void
ReleaseEvent(struct kevent& event);
On 2011/03/29 21:27:25, Mark Mentovai wrote:
> It’s unorthodox in our style to pass a non-const reference. Can this be a
> pointer?

I'm only doing it this way because I use foreach below to close things and that
passes struct kevent& in. If you feel strongly about it, I can get rid of the
for each and pass them in by pointer.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:80: static void
CloseFileDescriptor(int *fd);
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> use int* instead of int *

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:98: int
FilePathWatcherImpl::EventsForPath(FilePath path, EventVector *events) {
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> Asterisk by the type.

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:122: fd =
FileDescriptorForPath(built_path);
On 2011/03/29 21:27:25, Mark Mentovai wrote:
> Do you have to worry about closing these ever? You only seem to close them in
> some cases.

Good catch. ReleaseEvent now handles closing them.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:134: | NOTE_RENAME |
NOTE_REVOKE | NOTE_EXTEND), 0, data);
On 2011/03/29 21:27:25, Mark Mentovai wrote:
> Put the operator at the end of the preceding line instead of the beginning of
> this one. This is a Chromium-only requirement, not part of standard Google
> style.
> 
> http://www.chromium.org/developers/coding-style#Code_Formatting

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:147: }
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> No need for curlies for single-line conditionals (also at other places), but
if
> you like them, keep them :)

I like them. With all the python programmers we have around who think that
indentation is going to save them, I prefer to be explicit about my blocks.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:155: void
FilePathWatcherImpl::OnFileCanReadWithoutBlocking(int fd) {
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> This is a very long function that handles separate aspects, i.e. detecting
> events and updating watches. Maybe break it up a bit?

I tried breaking it up a bit, and it didn't really break cleanly. Hopefully the
added comments will help.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:158:
std::vector<struct kevent> updates(events_.size());
On 2011/03/30 14:07:52, Mattias Nissler wrote:
> On 2011/03/30 13:59:08, Mark Mentovai wrote:
> > Mattias Nissler wrote:
> > > Maybe a scoped_array is more appropriate?
> > 
> > No, std::vector is. :)
> 
> Can you give reasons? He doesn't use any vector-specific goodness, so what's
bad
> about a simple heap-allocated array (which is what vector alos does behind the
> scenes)?

This discussion was taken to email ;-) Left as a vector for now.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:160: int count =
kevent(kqueue_, NULL, 0, &updates[0], updates.size(), &timeout);
On 2011/03/29 21:27:25, Mark Mentovai wrote:
> HANDLE_EINTR.

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:176: CHECK(event !=
events_.end());
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> trusting the kernel to only pass valid fds? We've been bitten quite a few
times
> on windows because of this, so you may want to switch this over to
> check-NOTREACHED()-continue.

I've never seen this on the Mac (not to say it can't happen). I'd feel
uncomfortable attempting to continue in that case, so I'm going to chicken out,
and error on it. I can't think of a way to unittest this case though.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:196:
reinterpret_cast<EventData*>(next_event->udata);
On 2011/03/29 21:27:25, Mark Mentovai wrote:
> Continuation lines are indented by 4.

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:222: count =
kevent(kqueue_, &events_[0], valid, NULL, 0, NULL);
On 2011/03/29 21:27:25, Mark Mentovai wrote:
> HANDLE_EINTR (mostly just paranoia in this case).

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:222: count =
kevent(kqueue_, &events_[0], valid, NULL, 0, NULL);
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> Don't we need EV_ERROR checking for the responses?

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:230: for(; valid <
events_.size(); ++valid) {
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> space after for

Done.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:278:
std::vector<struct kevent> responses(last_entry);
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> a scoped_array might be more appropriate here?

comment about vector vs scoped_array above.

http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
content/common/file_path_watcher/file_path_watcher_mac.cc:283: return false;
On 2011/03/30 11:30:52, Mattias Nissler wrote:
> Do we need to close the directory FDs when we hit an error here and below?

They would be closed off in the dtor, but I can see that closing them off
earlier may make sense here.

[Mattias Nissler (ping if slow), 2011-03-30 18:07:54]

I'm still a bit unhappy about the long function. But since I don't have
constructive comments, LGTM.

On 2011/03/30 17:18:47, dmac wrote:
> Thanks guys. Ball is back in your court(s)
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> File content/common/file_path_watcher/file_path_watcher_mac.cc (right):
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:35: // that the
> FSEvents impl would miss.
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > Maybe also mention that FSEvents needs a runloop backed MessageLoop, so it
> adds
> > additional complexity.
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:63: typedef
> std::vector<struct kevent> EventVector;
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > Ah, does this mean we consume one file descriptor per path component? Is
this
> > acceptable?
> 
> That's kind of a requirement for the kqueue implementation, unless you have a
> solution to it?
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:65: virtual void
> CancelOnMessageLoopThread() OVERRIDE;
> On 2011/03/29 21:27:25, Mark Mentovai wrote:
> > This deserves a comment. Does it mean that it can be called fro any thread
and
> > asks for work to be done on MessageLoopThread, or that it must be called
from
> > MessageLoopThread?
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:73: static void
> ReleaseEvent(struct kevent& event);
> On 2011/03/29 21:27:25, Mark Mentovai wrote:
> > It’s unorthodox in our style to pass a non-const reference. Can this be a
> > pointer?
> 
> I'm only doing it this way because I use foreach below to close things and
that
> passes struct kevent& in. If you feel strongly about it, I can get rid of the
> for each and pass them in by pointer.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:80: static void
> CloseFileDescriptor(int *fd);
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > use int* instead of int *
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:98: int
> FilePathWatcherImpl::EventsForPath(FilePath path, EventVector *events) {
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > Asterisk by the type.
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:122: fd =
> FileDescriptorForPath(built_path);
> On 2011/03/29 21:27:25, Mark Mentovai wrote:
> > Do you have to worry about closing these ever? You only seem to close them
in
> > some cases.
> 
> Good catch. ReleaseEvent now handles closing them.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:134: | NOTE_RENAME |
> NOTE_REVOKE | NOTE_EXTEND), 0, data);
> On 2011/03/29 21:27:25, Mark Mentovai wrote:
> > Put the operator at the end of the preceding line instead of the beginning
of
> > this one. This is a Chromium-only requirement, not part of standard Google
> > style.
> > 
> > http://www.chromium.org/developers/coding-style#Code_Formatting
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:147: }
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > No need for curlies for single-line conditionals (also at other places), but
> if
> > you like them, keep them :)
> 
> I like them. With all the python programmers we have around who think that
> indentation is going to save them, I prefer to be explicit about my blocks.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:155: void
> FilePathWatcherImpl::OnFileCanReadWithoutBlocking(int fd) {
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > This is a very long function that handles separate aspects, i.e. detecting
> > events and updating watches. Maybe break it up a bit?
> 
> I tried breaking it up a bit, and it didn't really break cleanly. Hopefully
the
> added comments will help.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:158:
> std::vector<struct kevent> updates(events_.size());
> On 2011/03/30 14:07:52, Mattias Nissler wrote:
> > On 2011/03/30 13:59:08, Mark Mentovai wrote:
> > > Mattias Nissler wrote:
> > > > Maybe a scoped_array is more appropriate?
> > > 
> > > No, std::vector is. :)
> > 
> > Can you give reasons? He doesn't use any vector-specific goodness, so what's
> bad
> > about a simple heap-allocated array (which is what vector alos does behind
the
> > scenes)?
> 
> This discussion was taken to email ;-) Left as a vector for now.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:160: int count =
> kevent(kqueue_, NULL, 0, &updates[0], updates.size(), &timeout);
> On 2011/03/29 21:27:25, Mark Mentovai wrote:
> > HANDLE_EINTR.
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:176: CHECK(event !=
> events_.end());
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > trusting the kernel to only pass valid fds? We've been bitten quite a few
> times
> > on windows because of this, so you may want to switch this over to
> > check-NOTREACHED()-continue.
> 
> I've never seen this on the Mac (not to say it can't happen). I'd feel
> uncomfortable attempting to continue in that case, so I'm going to chicken
out,
> and error on it. I can't think of a way to unittest this case though.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:196:
> reinterpret_cast<EventData*>(next_event->udata);
> On 2011/03/29 21:27:25, Mark Mentovai wrote:
> > Continuation lines are indented by 4.
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:222: count =
> kevent(kqueue_, &events_[0], valid, NULL, 0, NULL);
> On 2011/03/29 21:27:25, Mark Mentovai wrote:
> > HANDLE_EINTR (mostly just paranoia in this case).
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:222: count =
> kevent(kqueue_, &events_[0], valid, NULL, 0, NULL);
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > Don't we need EV_ERROR checking for the responses?
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:230: for(; valid <
> events_.size(); ++valid) {
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > space after for
> 
> Done.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:278:
> std::vector<struct kevent> responses(last_entry);
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > a scoped_array might be more appropriate here?
> 
> comment about vector vs scoped_array above.
> 
>
http://codereview.chromium.org/6731064/diff/1/content/common/file_path_watche...
> content/common/file_path_watcher/file_path_watcher_mac.cc:283: return false;
> On 2011/03/30 11:30:52, Mattias Nissler wrote:
> > Do we need to close the directory FDs when we hit an error here and below?
> 
> They would be closed off in the dtor, but I can see that closing them off
> earlier may make sense here.

[Mark Mentovai, 2011-03-30 18:20:39]

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:172: LOG(ERROR) <<
"Error: " << events[i].data;
Will these messages provide enough context to be able to diagnose problems? Is a
path also needed here?

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:182: 
CHECK(events_.size()) or bail if it’s zero.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:200: if (event->ident
== updates[i].ident)
After your anti-Python pro-brace rant, you left them off here?! :)

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:216:
CloseFileDescriptor(reinterpret_cast<int*>(&event->ident));
Do you need to remove it from the kqueue too? (Line 233 also?)

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:258:
std::vector<struct kevent> updates(valid);
If valid is 0, you need to stop right here.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:259: count =
HANDLE_EINTR(kevent(kqueue_, &events_[0], valid, &updates[0], valid,
The second argument to kevent is a list of changes that you want applied to the
kqueue. It looks like this just gives the kqueue the same set of changes that
were obtained in Watch(), possibly limited to fewer items. It doesn’t look like
you ever remove anything from the kqueue (EV_DELETE).

I’m worried about two things.

1. On its face, it doesn’t seem right, once you detect this condition, to not
EV_DELETE the events from the kqueue.

2. Similarly, do you even need to keep the invalid events in events_?

This is the kind of thing that I thought might have been a problem for sure but
it was difficult to read this code for correctness because this function is so
long. I’ve taken a bit more time to digest it on this pass, but I still agree
with Mattias (and I think I mentioned in the last round as a whole-CL comment)
that in its current long-winded state, it’s the least understandable part of
your change.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:314: int last_entry =
EventsForPath(target_, &events_);
CHECK(last_entry) or bail if it’s zero.

[Mark Mentovai, 2011-03-30 19:49:30]

Oh, I guess the close already does the EV_DELETE. Some of my other
concerns remain, but I’m willing to be talked out of them if you’ve
got good reasons for things being the way they are.

[dmaclach1, 2011-03-30 19:51:33]

On Wed, Mar 30, 2011 at 12:49 PM, Mark Mentovai <mark@chromium.org> wrote:
> Oh, I guess the close already does the EV_DELETE. Some of my other
> concerns remain, but I’m willing to be talked out of them if you’ve
> got good reasons for things being the way they are.
>

Yep... I'm adding a comment though for that since it's not clear. Also
fixing up your other concerns.

[dmac, 2011-03-30 22:39:28]

Changes were substantial to refactor long function. PTAL. I really appreciate
the feedback.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:172: LOG(ERROR) <<
"Error: " << events[i].data;
On 2011/03/30 18:20:39, Mark Mentovai wrote:
> Will these messages provide enough context to be able to diagnose problems? Is
a
> path also needed here?

Done.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:182: 
On 2011/03/30 18:20:39, Mark Mentovai wrote:
> CHECK(events_.size()) or bail if it’s zero.

Done.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:200: if (event->ident
== updates[i].ident)
On 2011/03/30 18:20:39, Mark Mentovai wrote:
> After your anti-Python pro-brace rant, you left them off here?! :)

Always fun being caught being a hypocrite. :) Fixed.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:216:
CloseFileDescriptor(reinterpret_cast<int*>(&event->ident));
On 2011/03/30 18:20:39, Mark Mentovai wrote:
> Do you need to remove it from the kqueue too? (Line 233 also?)

Added comments explaining why this isn't necessary.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:258:
std::vector<struct kevent> updates(valid);
On 2011/03/30 18:20:39, Mark Mentovai wrote:
> If valid is 0, you need to stop right here.

Done.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:259: count =
HANDLE_EINTR(kevent(kqueue_, &events_[0], valid, &updates[0], valid,
On 2011/03/30 18:20:39, Mark Mentovai wrote:
> The second argument to kevent is a list of changes that you want applied to
the
> kqueue. It looks like this just gives the kqueue the same set of changes that
> were obtained in Watch(), possibly limited to fewer items. It doesn’t look
like
> you ever remove anything from the kqueue (EV_DELETE).
> 
> I’m worried about two things.
> 
> 1. On its face, it doesn’t seem right, once you detect this condition, to not
> EV_DELETE the events from the kqueue.
> 
> 2. Similarly, do you even need to keep the invalid events in events_?
> 
> This is the kind of thing that I thought might have been a problem for sure
but
> it was difficult to read this code for correctness because this function is so
> long. I’ve taken a bit more time to digest it on this pass, but I still agree
> with Mattias (and I think I mentioned in the last round as a whole-CL comment)
> that in its current long-winded state, it’s the least understandable part of
> your change.

Hopefully this has been dealt with adequately.

http://codereview.chromium.org/6731064/diff/4011/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:314: int last_entry =
EventsForPath(target_, &events_);
On 2011/03/30 18:20:39, Mark Mentovai wrote:
> CHECK(last_entry) or bail if it’s zero.

Done.

[Mattias Nissler (ping if slow), 2011-03-30 22:50:10]

My brain is incapable of doing a thorough review at this time, but I definitely
like the way you broke up the logic.

http://codereview.chromium.org/6731064/diff/14001/content/common/file_path_wa...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/14001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_mac.cc:247: // potentially
rendering other events in |updates| to be rendered
grammar.

http://codereview.chromium.org/6731064/diff/14001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_mac.cc:266: // potentially
rendering other events in |updates| to be rendered
grammar

[dmac, 2011-03-31 00:15:31]

On 2011/03/30 22:50:10, Mattias Nissler wrote:
> My brain is incapable of doing a thorough review at this time, but I
definitely
> like the way you broke up the logic.
> 
>
http://codereview.chromium.org/6731064/diff/14001/content/common/file_path_wa...
> File content/common/file_path_watcher/file_path_watcher_mac.cc (right):
> 
>
http://codereview.chromium.org/6731064/diff/14001/content/common/file_path_wa...
> content/common/file_path_watcher/file_path_watcher_mac.cc:247: // potentially
> rendering other events in |updates| to be rendered
> grammar.
> 
>
http://codereview.chromium.org/6731064/diff/14001/content/common/file_path_wa...
> content/common/file_path_watcher/file_path_watcher_mac.cc:266: // potentially
> rendering other events in |updates| to be rendered
> grammar

Fixed and Fixed.

[Mark Mentovai, 2011-03-31 01:50:35]

This is in good shape now. Let’s go with it.

LGTM

http://codereview.chromium.org/6731064/diff/16001/content/common/file_path_wa...
File content/common/file_path_watcher/file_path_watcher_browsertest.cc (right):

http://codereview.chromium.org/6731064/diff/16001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_browsertest.cc:8: #if
defined(OS_WIN)
These should be moved down.

http://www.chromium.org/developers/coding-style#Code_Formatting

#includes should be ordered per Google style, except platform-specific (#if
defined(..)) headers to follow the cross platform headers in a separate section

Discussion at
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

http://codereview.chromium.org/6731064/diff/16001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_browsertest.cc:493: mode =
(S_IRUSR | S_IRGRP | S_IROTH);
You don’t need the (parentheses) in any of these.

http://codereview.chromium.org/6731064/diff/16001/content/common/file_path_wa...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/16001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_mac.cc:36: // items in the
list above except for detecting modifications to files in a
You broke this comment into a new line here for no apparent reason. Reflow.

http://codereview.chromium.org/6731064/diff/16001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_mac.cc:99: bool
UpdateWatches(bool* target_file_affected);
I already like where this refactoring has gone.

http://codereview.chromium.org/6731064/diff/16001/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_mac.cc:224: "fd: %d",
*reinterpret_cast<int*>(&kevents[i].ident));
Minor nit: you don’t need the colon, the error will read better without it.

[dmac, 2011-03-31 18:28:26]

Brett/John/Darin

As owners, could I get a review so I can check in? This is a M11 release blocker
CL.

[brettw, 2011-03-31 21:00:43]

LGTM (I didn't do a real code review, and I certainly didn't look at the new
scary _mac.cc file).

[Mark Mentovai, 2011-03-31 21:03:02]

brettw@chromium.org wrote:
> LGTM (I didn't do a real code review, and I certainly didn't look at the new
> scary _mac.cc file).

Don’t worry, I was scared enough for all of us.

[dmac, 2011-04-01 18:09:46]

Turns out that this failed on 10.5 because kqueue on 10.5 returns both
NOTE_ATTRIB and NOTE_WRITE when a file is created in a directory. Fixed this by
changing the if-elif-else for handling changes into an if-if-if. 

Also in trying to track down the problem noted that OnError was a very generic
name for an interface and could easily clash, and renamed it to OnFilePathError.

Mark, do you mind taking another look?

[Mark Mentovai, 2011-04-01 18:13:37]

LGTM

http://codereview.chromium.org/6731064/diff/18002/content/common/file_path_wa...
File content/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6731064/diff/18002/content/common/file_path_wa...
content/common/file_path_watcher/file_path_watcher_mac.cc:384: if
((updates[i].fflags & NOTE_ATTRIB) && !target_file_affected) {
Rationalize your style here so everything is consistent. You don’t strictly need
parentheses around |updates[i].fflags & NOTE_ATTRIB| but you’ve used them here;
but then you didn’t use them in the same situation on line 390. Personally, I
think they improve readability. I don’t mind if you choose to not use them,
though, as long as you do the same thing here and on 390.