drive: Fix two instances of madness in FileBrowserEventRouter

drive: Fix two instances of madness in FileBrowserEventRouter

1) Being a ref counted class
Previously, FileBrowserEventRouter was a ref counted class, but the weak
pointer was also used, which doesn't make much sense. Being a ref counted
class was bad, as lifetime of FileBrowserEventRouter was unclear, and 
FileBrowserEventRouter could outlive things like DBusThreadManager and
PrefService in rare cases, which caused crash at shutdown.

2) Use of a mutex lock
The lock was used to protect |file_watchers_|, which was accessed from FILE
and UI threads, which should be avoided. Interaction between FILE and UI
threads were also hard to read.

BUG=227477, 227483
TEST=Open Downloads in Files.app. Download a file to Downloads. Confirm that the new file appears in Files.app immediatley.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192993

[hashimoto, 2013-04-08 11:10:26]

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_event_router.h:74: bool
AddFileWatch(const base::FilePath& local_path,
A method returning bool while taking BoolCallback as its argument seems
confusing.
Can we change this method's return type to 'void'?

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:748:
event_router->RemoveFileWatch(
nit: Can't this method call be one line?

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_browser_private_api.h (right):

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.h:110: virtual void
PerformFileWatchOperation(
nit: Could you add a comment for this method while you are here?

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.h:131: virtual void
PerformFileWatchOperation(
nit: Could you add a "FileWatchBrowserFunctionBase override" comment while you
are here?

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.h:147: virtual void
PerformFileWatchOperation(
ditto.

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:409:
BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, iter->second);
Why do you delete FileWatcherExtensions on FILE thread while having
"DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));" in the dtor of the
class?

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:667:
base::AutoLock lock(lock_);
Can't we remove this AutoLock to eliminate |lock_|?

[satorux1, 2013-04-09 01:24:38]

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_event_router.h:74: bool
AddFileWatch(const base::FilePath& local_path,
On 2013/04/08 11:10:26, hashimoto wrote:
> A method returning bool while taking BoolCallback as its argument seems
> confusing.
> Can we change this method's return type to 'void'?

Done.

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:748:
event_router->RemoveFileWatch(
On 2013/04/08 11:10:26, hashimoto wrote:
> nit: Can't this method call be one line?

Done.

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_browser_private_api.h (right):

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.h:110: virtual void
PerformFileWatchOperation(
On 2013/04/08 11:10:26, hashimoto wrote:
> nit: Could you add a comment for this method while you are here?

Done.

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.h:131: virtual void
PerformFileWatchOperation(
On 2013/04/08 11:10:26, hashimoto wrote:
> nit: Could you add a "FileWatchBrowserFunctionBase override" comment while you
> are here?

Done.

https://codereview.chromium.org/13776005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_browser_private_api.h:147: virtual void
PerformFileWatchOperation(
On 2013/04/08 11:10:26, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:341:
base::Unretained(this)));
replaced this with a weak ptr.

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:345:
base::Unretained(this));
replaced this with a weak ptr.

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:409:
BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, iter->second);
On 2013/04/08 11:10:26, hashimoto wrote:
> Why do you delete FileWatcherExtensions on FILE thread while having
> "DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));" in the dtor of the
> class?

oops. Good catch! I was confused. FilePathWatcher needs to be deleted on FILE
thread, but FileWatcherExtensions does not. Fixed.

https://codereview.chromium.org/13776005/diff/4001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:667:
base::AutoLock lock(lock_);
On 2013/04/08 11:10:26, hashimoto wrote:
> Can't we remove this AutoLock to eliminate |lock_|?

oops. I thought I removed lock_ completely. Removed.

[hashimoto, 2013-04-09 01:34:09]

lgtm

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.h:144: // Starts a
file watch at |local_path|. |file_path_watcher| will be
nit: s/file_path_watcher/file_watcher_callback/?

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.h:148: //
|successfully, or false if failed. |callback| must not be null.
nit: Is '|' in "|successfully" necessary?

[satorux1, 2013-04-09 01:36:25]

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.h:148: //
|successfully, or false if failed. |callback| must not be null.
On 2013/04/09 01:34:09, hashimoto wrote:
> nit: Is '|' in "|successfully" necessary?

Done.

[hashimoto, 2013-04-09 01:38:59]

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.h:144: // Starts a
file watch at |local_path|. |file_path_watcher| will be
On 2013/04/09 01:34:09, hashimoto wrote:
> nit: s/file_path_watcher/file_watcher_callback/?
ping?

[satorux1, 2013-04-09 01:41:21]

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

https://codereview.chromium.org/13776005/diff/16001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.h:144: // Starts a
file watch at |local_path|. |file_path_watcher| will be
On 2013/04/09 01:38:59, hashimoto wrote:
> On 2013/04/09 01:34:09, hashimoto wrote:
> > nit: s/file_path_watcher/file_watcher_callback/?
> ping?

oops. missed it. Done.

[hashimoto, 2013-04-09 01:47:16]

lgtm

[commit-bot: I haz the power, 2013-04-09 01:51:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/13776005/28018

[satorux1, 2013-04-09 03:33:47]

Committed patchset #6 manually as r192993 (presubmit successful).