Move FilePathWatcher class from browser/... to common/...

Move FilePathWatcher class from chrome/browser/... to content/common...

The service process needs FilePathWatcher, and with this change I
got rid of the dependency on BrowserThread which allows it to be moved to common/...
I also tried my hand at a kqueue impl on the Mac, but failed.
See http://code.google.com/p/chromium/issues/detail?id=54822#c13 for details.

BUG=74983
TEST=BUILD

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

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

[dmac, 2011-03-17 06:08:49]

Mattias, would you mind taking a look?

I did delete the other files in .../browser/file_path_watcher/*, but for some
reason the git-svn gateway isn't noticing it. I'll try and figure out what's
going wrong tomorrow with it. In  general though, the rest of this should be
good I think.

[jam, 2011-03-17 06:20:23]

Since content\browser uses this, this class will eventually have to end up in
content\common (since content can't depend on chrome, once all the dependencies
are removed).  If you can move it there, that'll save another move.

[Mattias Nissler (ping if slow), 2011-03-17 10:37:56]

Here's a bunch of comments.

In general, due to hard-to-track-down flakiness we've seen with the OS APIs in
the tests before, I recommend running the tests in a loop over night (in
particular on Mac), to be sure to not reintroduce flakiness.

Please also coordinate with joaodasilva, who has a pending cleanup CL for the
FilePathWatcher code at http://codereview.chromium.org/6697020/

Out of curiousity: What does the service process watch for?

http://codereview.chromium.org/6670081/diff/1/chrome/browser/user_style_sheet...
File chrome/browser/user_style_sheet_watcher_unittest.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/browser/user_style_sheet...
chrome/browser/user_style_sheet_watcher_unittest.cc:32: base::Thread
io_thread("UserStyleSheetWatcherTestIOThread");
Why do we need two loops here? Can't we use the same message loop proxy for both
UI and FILE?

http://codereview.chromium.org/6670081/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6670081/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:1732:
'common/file_path_watcher/file_path_watcher_unittest.cc',
There is a reason we had the tests in browser tests: The underlying OS API (in
particular on Mac) is flaky, so we need a reliable timeout mechanism. Running as
part of unit_tests will result in a global timeout, preventing all subsequent
tests to run.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher.cc:21:
impl_->set_message_loop(base::MessageLoopProxy::CreateForCurrentThread());
This effectively allows you to instantiate a watcher on any thread, whereas
before it made sure that you can only run it on the FILE thread. Can we improve
this?

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_inotify.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_inotify.cc:350:
DCHECK(MessageLoopForIO::current());
Why do you have MessageLoopForIO::current() here while you use
message_loop()->BelongsToCurrentThread() below? Can't we just use
message_loop()->BelongsToCurrentThread() everywhere?

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:170:
DCHECK(MessageLoopForIO::current());
Same question as for the inotify implementation: Why can't we be consistently
using message_loop()->BelongsToCurrentThread()?

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:193: if
(run_loop_message_loop().get() &&
I guess you need the NULL check since Cancel() can be called even if Watch() was
never called, correct? But shouldn't we just bail out in that case, since there
is no reason to run the cleanup? In any case, a comment explaining the situation
would probably be helpful.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:207:
DCHECK(MessageLoopForUI::current());
It seems that we can do the message loop type check at initialization time.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_unittest.cc:117:
scoped_refptr<base::MessageLoopProxy> mac_run_loop)
I think most of our code passes plain pointers in this situation.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_unittest.cc:189:
TestTimeouts::action_timeout_ms());
This is exactly the hang we saw on the Mac test bots, which is why we made this
a browser test. Also, there is the problem that we have sequences in the tests
that WaitForEvents() multiple times. In that case, there are stale Quit tasks on
the loop, which may lead to additional flakiness (since they can abort
subsequent WaitForEvents() early).

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_win.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_win.cc:86: if
(message_loop().get() &&
Same comment as for Mac.

[dmac, 2011-03-17 17:16:48]

I think I addressed all comments. joaodasilva, I'm afraid this may be disruptive
to your CL, but I'm trying to address a release blocker right now. Any
suggestions?

http://codereview.chromium.org/6670081/diff/1/chrome/browser/user_style_sheet...
File chrome/browser/user_style_sheet_watcher_unittest.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/browser/user_style_sheet...
chrome/browser/user_style_sheet_watcher_unittest.cc:32: base::Thread
io_thread("UserStyleSheetWatcherTestIOThread");
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> Why do we need two loops here? Can't we use the same message loop proxy for
both
> UI and FILE?

No, because the Mac and Linux impl's check to see if they are running on a
TYPE_IO loop, and the Mac requires a CFRunLoop based UI loop for the FSEvents
code to hang off of.

http://codereview.chromium.org/6670081/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6670081/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:1732:
'common/file_path_watcher/file_path_watcher_unittest.cc',
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> There is a reason we had the tests in browser tests: The underlying OS API (in
> particular on Mac) is flaky, so we need a reliable timeout mechanism. Running
as
> part of unit_tests will result in a global timeout, preventing all subsequent
> tests to run.

Moved back to browsertest

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher.cc:21:
impl_->set_message_loop(base::MessageLoopProxy::CreateForCurrentThread());
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> This effectively allows you to instantiate a watcher on any thread, whereas
> before it made sure that you can only run it on the FILE thread. Can we
improve
> this?

Down in the implementations we check to make sure that they are running on the
proper type of thread. Service process doesn't have the concept of browser
threads, so I'm not sure how we can enforce this any more than just checking the
type of the threads down in the implementation.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_inotify.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_inotify.cc:350:
DCHECK(MessageLoopForIO::current());
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> Why do you have MessageLoopForIO::current() here while you use
> message_loop()->BelongsToCurrentThread() below? Can't we just use
> message_loop()->BelongsToCurrentThread() everywhere?

This is the check to make sure that Watch is being called on at least the proper
kind of thread (as opposed to what you had earlier where we could check to make
sure it was being run explicitly on the FILE thread).

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:27: //
http://code.google.com/p/chromium/issues/detail?id=54822#c13
out of curiosity, had you already reached this same conclusion?

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:170:
DCHECK(MessageLoopForIO::current());
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> Same question as for the inotify implementation: Why can't we be consistently
> using message_loop()->BelongsToCurrentThread()?

Same response ;-)

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:193: if
(run_loop_message_loop().get() &&
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> I guess you need the NULL check since Cancel() can be called even if Watch()
was
> never called, correct? But shouldn't we just bail out in that case, since
there
> is no reason to run the cleanup? In any case, a comment explaining the
situation
> would probably be helpful.

Done.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:207:
DCHECK(MessageLoopForUI::current());
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> It seems that we can do the message loop type check at initialization time.

We can't actually (unless I'm missing something) because there doesn't appear to
be anyway of getting the type of a MessageLoopProxy.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_unittest.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_unittest.cc:117:
scoped_refptr<base::MessageLoopProxy> mac_run_loop)
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> I think most of our code passes plain pointers in this situation.

Done.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_unittest.cc:189:
TestTimeouts::action_timeout_ms());
Fixed by going back to browsertest

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_win.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_win.cc:86: if
(message_loop().get() &&
On 2011/03/17 10:37:56, Mattias Nissler wrote:
> Same comment as for Mac.

Done.

[dmac, 2011-03-17 18:03:44]

On 2011/03/17 06:08:49, dmac wrote:
> Mattias, would you mind taking a look?
> 
> I did delete the other files in .../browser/file_path_watcher/*, but for some
> reason the git-svn gateway isn't noticing it. I'll try and figure out what's
> going wrong tomorrow with it. In  general though, the rest of this should be
> good I think.

BTW turns out the deleted file problem is a known git-cl issue according to
maruel. The files should be removed correctly when I dcommit.

[Mattias Nissler (ping if slow), 2011-03-17 18:14:26]

Getting close, mostly nits. I'll be out of office till Monday and I don't see
any big issues left, so feel free to commit once John and the trybots are happy.

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
File chrome/common/file_path_watcher/file_path_watcher_mac.cc (right):

http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
chrome/common/file_path_watcher/file_path_watcher_mac.cc:27: //
http://code.google.com/p/chromium/issues/detail?id=54822#c13
On 2011/03/17 17:16:48, dmac wrote:
> out of curiosity, had you already reached this same conclusion?

Yes. Sorry I didn't update the bug. IIRC, there was some email thread in which I
mentioned my findings.

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher.h (right):

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher.h:45:
scoped_refptr<base::MessageLoopProxy> loop) WARN_UNUSED_RESULT;
Plain pointer here, too?

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher.h:67:
scoped_refptr<base::MessageLoopProxy> loop) WARN_UNUSED_RESULT = 0;
and here?

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher.h:73:
scoped_refptr<base::MessageLoopProxy> message_loop() const {
Why is this public? The only reason I can see is that the inotify implementation
can fetch the message loop and post something on the watcher impl object (this
could be easily resolved by moving the thread switch into the impl class, where
it belongs).

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher.h:76: void
set_message_loop(scoped_refptr<base::MessageLoopProxy> loop);
It seems odd that the message_loop_ accessors are public. Moreover, why do you
pass in one of the message loops in Watch() and install the other at the same
time in set_message_loop()?

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_inotify.cc (right):

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_inotify.cc:286:
(*watcher)->message_loop()->PostTask(FROM_HERE,
This is what I was referring to in my other comment. It seems strange to get the
message loop from the watcher and then use it to post a task to that exact
watcher. I think we should just tell the watcher to its thing and it'll do the
thread switching internally.

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_inotify.cc:371: // Switch to
the file thread if necessary so we can access |watches_|.
indentation

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

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_mac.cc:197: // Switch to the
UI thread if necessary, so we can tear down the event stream.
nit: update comment.

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
File content/common/file_path_watcher/file_path_watcher_win.cc (right):

http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
content/common/file_path_watcher/file_path_watcher_win.cc:89: // Switch to the
file thread if necessary so we can stop |watcher_|.
indentation

[Joao da Silva, 2011-03-17 20:31:33]

On 2011/03/17 17:16:48, dmac wrote:
> I think I addressed all comments. joaodasilva, I'm afraid this may be
disruptive
> to your CL, but I'm trying to address a release blocker right now. Any
> suggestions?
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/browser/user_style_sheet...
> File chrome/browser/user_style_sheet_watcher_unittest.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/browser/user_style_sheet...
> chrome/browser/user_style_sheet_watcher_unittest.cc:32: base::Thread
> io_thread("UserStyleSheetWatcherTestIOThread");
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > Why do we need two loops here? Can't we use the same message loop proxy for
> both
> > UI and FILE?
> 
> No, because the Mac and Linux impl's check to see if they are running on a
> TYPE_IO loop, and the Mac requires a CFRunLoop based UI loop for the FSEvents
> code to hang off of.
> 
> http://codereview.chromium.org/6670081/diff/1/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/chrome_tests.gypi#newcod...
> chrome/chrome_tests.gypi:1732:
> 'common/file_path_watcher/file_path_watcher_unittest.cc',
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > There is a reason we had the tests in browser tests: The underlying OS API
(in
> > particular on Mac) is flaky, so we need a reliable timeout mechanism.
Running
> as
> > part of unit_tests will result in a global timeout, preventing all
subsequent
> > tests to run.
> 
> Moved back to browsertest
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> File chrome/common/file_path_watcher/file_path_watcher.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher.cc:21:
> impl_->set_message_loop(base::MessageLoopProxy::CreateForCurrentThread());
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > This effectively allows you to instantiate a watcher on any thread, whereas
> > before it made sure that you can only run it on the FILE thread. Can we
> improve
> > this?
> 
> Down in the implementations we check to make sure that they are running on the
> proper type of thread. Service process doesn't have the concept of browser
> threads, so I'm not sure how we can enforce this any more than just checking
the
> type of the threads down in the implementation.
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> File chrome/common/file_path_watcher/file_path_watcher_inotify.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_inotify.cc:350:
> DCHECK(MessageLoopForIO::current());
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > Why do you have MessageLoopForIO::current() here while you use
> > message_loop()->BelongsToCurrentThread() below? Can't we just use
> > message_loop()->BelongsToCurrentThread() everywhere?
> 
> This is the check to make sure that Watch is being called on at least the
proper
> kind of thread (as opposed to what you had earlier where we could check to
make
> sure it was being run explicitly on the FILE thread).
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> File chrome/common/file_path_watcher/file_path_watcher_mac.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_mac.cc:27: //
> http://code.google.com/p/chromium/issues/detail?id=54822#c13
> out of curiosity, had you already reached this same conclusion?
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_mac.cc:170:
> DCHECK(MessageLoopForIO::current());
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > Same question as for the inotify implementation: Why can't we be
consistently
> > using message_loop()->BelongsToCurrentThread()?
> 
> Same response ;-)
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_mac.cc:193: if
> (run_loop_message_loop().get() &&
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > I guess you need the NULL check since Cancel() can be called even if Watch()
> was
> > never called, correct? But shouldn't we just bail out in that case, since
> there
> > is no reason to run the cleanup? In any case, a comment explaining the
> situation
> > would probably be helpful.
> 
> Done.
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_mac.cc:207:
> DCHECK(MessageLoopForUI::current());
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > It seems that we can do the message loop type check at initialization time.
> 
> We can't actually (unless I'm missing something) because there doesn't appear
to
> be anyway of getting the type of a MessageLoopProxy.
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> File chrome/common/file_path_watcher/file_path_watcher_unittest.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_unittest.cc:117:
> scoped_refptr<base::MessageLoopProxy> mac_run_loop)
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > I think most of our code passes plain pointers in this situation.
> 
> Done.
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_unittest.cc:189:
> TestTimeouts::action_timeout_ms());
> Fixed by going back to browsertest
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> File chrome/common/file_path_watcher/file_path_watcher_win.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_win.cc:86: if
> (message_loop().get() &&
> On 2011/03/17 10:37:56, Mattias Nissler wrote:
> > Same comment as for Mac.
> 
> Done.
No problem with my CL, I'll take care of it. This might even make it easier. I'm
not reviewing since I'm not a full committer yet, but LGTM!

[dmac, 2011-03-17 22:09:01]

On 2011/03/17 18:14:26, Mattias Nissler wrote:
> Getting close, mostly nits. I'll be out of office till Monday and I don't see
> any big issues left, so feel free to commit once John and the trybots are
happy.
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> File chrome/common/file_path_watcher/file_path_watcher_mac.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/1/chrome/common/file_path_watcher...
> chrome/common/file_path_watcher/file_path_watcher_mac.cc:27: //
> http://code.google.com/p/chromium/issues/detail?id=54822#c13
> On 2011/03/17 17:16:48, dmac wrote:
> > out of curiosity, had you already reached this same conclusion?
> 
> Yes. Sorry I didn't update the bug. IIRC, there was some email thread in which
I
> mentioned my findings.

Hmmm.. searched but didn't find it.

> 
>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> File content/common/file_path_watcher/file_path_watcher.h (right):
> 
>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher.h:45:
> scoped_refptr<base::MessageLoopProxy> loop) WARN_UNUSED_RESULT;
> Plain pointer here, too?

fixed.

>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher.h:67:
> scoped_refptr<base::MessageLoopProxy> loop) WARN_UNUSED_RESULT = 0;
> and here?

fixed.

>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher.h:73:
> scoped_refptr<base::MessageLoopProxy> message_loop() const {
> Why is this public? The only reason I can see is that the inotify
implementation
> can fetch the message loop and post something on the watcher impl object (this
> could be easily resolved by moving the thread switch into the impl class,
where
> it belongs).

fixed.

>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher.h:76: void
> set_message_loop(scoped_refptr<base::MessageLoopProxy> loop);
> It seems odd that the message_loop_ accessors are public. Moreover, why do you
> pass in one of the message loops in Watch() and install the other at the same
> time in set_message_loop()?

fixed.

>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> File content/common/file_path_watcher/file_path_watcher_inotify.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher_inotify.cc:286:
> (*watcher)->message_loop()->PostTask(FROM_HERE,
> This is what I was referring to in my other comment. It seems strange to get
the
> message loop from the watcher and then use it to post a task to that exact
> watcher. I think we should just tell the watcher to its thing and it'll do the
> thread switching internally.
> 
fixed


http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher_inotify.cc:371: // Switch
to
> the file thread if necessary so we can access |watches_|.
> indentation

fixed

>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> File content/common/file_path_watcher/file_path_watcher_mac.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher_mac.cc:197: // Switch to
the
> UI thread if necessary, so we can tear down the event stream.
> nit: update comment.

fixed

>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> File content/common/file_path_watcher/file_path_watcher_win.cc (right):
> 
>
http://codereview.chromium.org/6670081/diff/6001/content/common/file_path_wat...
> content/common/file_path_watcher/file_path_watcher_win.cc:89: // Switch to the
> file thread if necessary so we can stop |watcher_|.
> indentation
fixed

[dmac, 2011-03-17 22:11:23]

On 2011/03/17 20:31:33, joaodasilva wrote:
> No problem with my CL, I'll take care of it. This might even make it easier.
I'm
> not reviewing since I'm not a full committer yet, but LGTM!

Excellent. Thanks.