Move FileWatcher from src/base/ to src/chrome/browser/ and switch

Move FileWatcher from src/base/ to src/chrome/browser/ and switch
it from using MessageLoop to post tasks to using
ChromeThread::PostTask, which is safer.

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

[Paweł Hajdan Jr., 2010-03-11 07:33:37]

After fixing the minor comments LGTM.

http://codereview.chromium.org/864001/diff/1/4
File chrome/browser/file_watcher.h (right):

http://codereview.chromium.org/864001/diff/1/4#newcode41
chrome/browser/file_watcher.h:41: ChromeThread::ID thread_id) {
nit: I wonder if we can just drop the parameter here and hardcode
ChromeThread::FILE.

http://codereview.chromium.org/864001/diff/1/8
File chrome/browser/file_watcher_unittest.cc (right):

http://codereview.chromium.org/864001/diff/1/8#newcode226
chrome/browser/file_watcher_unittest.cc:226: /*
Please make sure to remove this test before committing.

[tony, 2010-03-11 09:25:54]

I made the following changes:
(1) Drop the thread_id parameter.  It was only used on Linux, so I'm just
hardcoding the use of file thread.
(2) Add a DCHECK to FileWatcher::Watch to make sure watchs happen on the file
thread.  This is important for mac and win which touch disk when notified of
changes.
(3) Make sure that PlatformDelegate is deleted on the file thread, since watches
happen on the file thread.  It's safe to create FileWatchers on other threads
since the ctor doesn't do anything significant.

I tried to make InotifyReader not be a singleton, but failed.  I'm not sure who
would own it.  I suppose that BrowserProcessImpl could own it, but it seems
weird to have do that for a class only used on Linux.

It would also be possible to move this back to base, but we would lose points
(2) and (3) above.  I'm not convinced there will be uses for this outside of
Chrome to make me want to do that.

[Paweł Hajdan Jr., 2010-03-11 20:48:50]

Just one issue on Mac.

http://codereview.chromium.org/864001/diff/6001/7005
File chrome/browser/file_watcher_mac.cc (left):

http://codereview.chromium.org/864001/diff/6001/7005#oldcode91
chrome/browser/file_watcher_mac.cc:91: DCHECK(MessageLoop::current()->type() ==
MessageLoop::TYPE_UI);
Hmm, this checks has now disappeared. The tests are fine since they use TYPE_UI
loop for everything, but in reality, is the file thread of TYPE_UI? If not, it's
going to break on Mac, at least the last time I checked.

[tony, 2010-03-12 08:59:01]

On 2010/03/11 20:48:50, Paweł Hajdan Jr. wrote:
> Just one issue on Mac.
> 
> http://codereview.chromium.org/864001/diff/6001/7005
> File chrome/browser/file_watcher_mac.cc (left):
> 
> http://codereview.chromium.org/864001/diff/6001/7005#oldcode91
> chrome/browser/file_watcher_mac.cc:91: DCHECK(MessageLoop::current()->type()
==
> MessageLoop::TYPE_UI);
> Hmm, this checks has now disappeared. The tests are fine since they use
TYPE_UI
> loop for everything, but in reality, is the file thread of TYPE_UI? If not,
it's
> going to break on Mac, at least the last time I checked.

Oh man, you're right.  Ok, I've updated the mac version so that Watch posts a
task back to the UI thread for the system calls.  The callback posts back to the
File thread so we can do the timestamp checks on the file thread.

I tested this by merging into the user style sheet change, and it works fine on
mac.

Take another look?

[Paweł Hajdan Jr., 2010-03-12 09:32:04]

I think this CL misses the change that removes file_watcher_unittest.cc from
base.gyp.

After fixing the issues LGTM.

http://codereview.chromium.org/864001/diff/10001/11003
File chrome/browser/file_watcher_inotify.cc (right):

http://codereview.chromium.org/864001/diff/10001/11003#newcode38
chrome/browser/file_watcher_inotify.cc:38: // http://crbug.com/
nit: Did you mean to put an actual bug reference here?

[tony, 2010-03-15 01:56:33]

On 2010/03/12 09:32:04, Paweł Hajdan Jr. wrote:
> I think this CL misses the change that removes file_watcher_unittest.cc from
> base.gyp.

Fixed.

> http://codereview.chromium.org/864001/diff/10001/11003
> File chrome/browser/file_watcher_inotify.cc (right):
> 
> http://codereview.chromium.org/864001/diff/10001/11003#newcode38
> chrome/browser/file_watcher_inotify.cc:38: // http://crbug.com/
> nit: Did you mean to put an actual bug reference here?

Fixed.

> After fixing the issues LGTM.

Passed on the try bots, so submitting.