Drive: Mount/Unmount GoogleDrive on Files App when the file system is mounted/unmounted.

Drive: Mount/Unmount GoogleDrive on Files App when the file system is mounted/unmounted. 

This cl is necessary to refresh the list on file manager on unmounting and mounting the drive file system. 

This patch does the following:
- Move the code to raise the mount/unmount events of file manager from file_browser_private_api.cc to file_browser_event_router.cc 
- Add handlers' interfaces to notify mount/unmount events in gdata_file_system_interface.h. 
- Call the mount/unmount events to FilesApp when handler is called. 
- Call the handlers when the file system is mounted/unmounted

BUG=135197
TEST=out/Debug/unit_tests:GData* and out/Debug/browser_tests:GData* passes.

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

[yoshiki, 2012-08-01 20:28:36]

satorux: could you take a look? This CL is one of 4 patches (maybe) to implement
crbug.com/135197.

[satorux1, 2012-08-01 20:59:00]

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:230: const
base::Callback<void(bool)>& callback) {
add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ?

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:244: const
std::string& token) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ?

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.h:55: // GData
mount routines.
oh no, please write better function comments. Don't follow bad examples nearby.
:)

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.h:56: void
MountGData(const base::Callback<void(bool)>& callback);
what's bool?

MountGData -> MountDrive(). Historically we've been abusing "gdata" to mean
Drive, but we should stop doing that.

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2909: new
GDataFileSystemProxy(this));
Hmm, why do we need to move the code to GDataFileSystem? We've been trying to
reduce code in GDataFileSystem.

[yoshiki, 2012-08-01 22:23:39]

Thanks, PTAL?

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:230: const
base::Callback<void(bool)>& callback) {
On 2012/08/01 20:59:01, satorux1 wrote:
> add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ?

Done.

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:244: const
std::string& token) {
On 2012/08/01 20:59:01, satorux1 wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); ?

Done.

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.h:55: // GData
mount routines.
On 2012/08/01 20:59:01, satorux1 wrote:
> oh no, please write better function comments. Don't follow bad examples
nearby.
> :)

Done.

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_browser_event_router.h:56: void
MountGData(const base::Callback<void(bool)>& callback);
On 2012/08/01 20:59:01, satorux1 wrote:
> what's bool?
It is intended to return the result of mount. But it currently always be true.

> MountGData -> MountDrive(). Historically we've been abusing "gdata" to mean
> Drive, but we should stop doing that.

Done.

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2909: new
GDataFileSystemProxy(this));
I moved this code in order to call NotifyFileSystemMounted() and to keep it
private. Do you think it is better to make it public or have good idea?

On 2012/08/01 20:59:01, satorux1 wrote:
> Hmm, why do we need to move the code to GDataFileSystem? We've been trying to
> reduce code in GDataFileSystem.

[satorux1, 2012-08-02 01:13:05]

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:237:
base::Bind(&FileBrowserEventRouter::OnDriveAuthentication,
OnAuthenticated

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1058: :
ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this))

[satorux1, 2012-08-02 01:17:12]

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2909: new
GDataFileSystemProxy(this));
What classes are interested in NotifyFileSystemMounted() event? Should this be
an event of GDataFileSystem::Observer? Can we emit this event from eleswhere?

[yoshiki, 2012-08-02 05:53:42]

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2909: new
GDataFileSystemProxy(this));
Sorry for my confusing explanation.

This method (GDataFileSystem::AddDriveMountPoint()) calls
GDataFileSystem::NotifyFileSystemMounted() at L2911, to raise an event to
observers.

If we move this method back to GDataSystemService, we have to change
GDataFileSystem::NotifyFileSystemMounted() a public method. Is it right for you?

On 2012/08/02 01:17:12, satorux1 wrote:
> What classes are interested in NotifyFileSystemMounted() event? Should this be
> an event of GDataFileSystem::Observer? Can we emit this event from eleswhere?

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:237:
base::Bind(&FileBrowserEventRouter::OnDriveAuthentication,
On 2012/08/02 01:13:05, satorux1 wrote:
> OnAuthenticated

Done.

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10834115/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1058: :
ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
The example usage in base/compiler_specific.h suggests that we should use
ALLOW_THIS_IN_INITIALIZER_LIST(y(this)).

On 2012/08/02 01:13:05, satorux1 wrote:
> weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this))

[satorux1, 2012-08-02 06:19:44]

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2909: new
GDataFileSystemProxy(this));
On 2012/08/02 05:53:42, yoshiki wrote:
> Sorry for my confusing explanation.
> 
> This method (GDataFileSystem::AddDriveMountPoint()) calls
> GDataFileSystem::NotifyFileSystemMounted() at L2911, to raise an event to
> observers.
> 
> If we move this method back to GDataSystemService, we have to change
> GDataFileSystem::NotifyFileSystemMounted() a public method. Is it right for
you?
> 
> On 2012/08/02 01:17:12, satorux1 wrote:
> > What classes are interested in NotifyFileSystemMounted() event? Should this
be
> > an event of GDataFileSystem::Observer? Can we emit this event from
eleswhere?
> 

My question was, why NotifyFileSystemMounted() has to be part of
GDataFileSystem::Observer? Who's interested in this event?

[yoshiki, 2012-08-02 06:37:01]

On 2012/08/02 06:19:44, satorux1 wrote:
> My question was, why NotifyFileSystemMounted() has to be part of
> GDataFileSystem::Observer? Who's interested in this event?

You mean OnFileSystemMounted() in GDataFileSystemInterface::Observer class,
right?

Though I'm not sure this is correct answer. This event is caught by
FileBrowserEventRouter::OnFileSystemMounted(). I think
GDataFileSystemInterface::Observer is the best place to notify events to file
manager from file system.

[satorux1, 2012-08-02 06:38:31]

forgot to send this out...

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10834115/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2909: new
GDataFileSystemProxy(this));
On 2012/08/02 06:19:44, satorux1 wrote:
> On 2012/08/02 05:53:42, yoshiki wrote:
> > Sorry for my confusing explanation.
> > 
> > This method (GDataFileSystem::AddDriveMountPoint()) calls
> > GDataFileSystem::NotifyFileSystemMounted() at L2911, to raise an event to
> > observers.
> > 
> > If we move this method back to GDataSystemService, we have to change
> > GDataFileSystem::NotifyFileSystemMounted() a public method. Is it right for
> you?
> > 
> > On 2012/08/02 01:17:12, satorux1 wrote:
> > > What classes are interested in NotifyFileSystemMounted() event? Should
this
> be
> > > an event of GDataFileSystem::Observer? Can we emit this event from
> eleswhere?
> > 
> 
> My question was, why NotifyFileSystemMounted() has to be part of
> GDataFileSystem::Observer? Who's interested in this event?

I meant:

why OnFileSystemMounted() has to be part of GDataFileSystem::Observer? Who's
interested in this event?


I'm guessing that this event should not belong to GDataFileSystem::Observer. The
class that mounts GDataFileSystem should emit the event?

[satorux1, 2012-08-02 06:40:05]

On 2012/08/02 06:37:01, yoshiki wrote:
> On 2012/08/02 06:19:44, satorux1 wrote:
> > My question was, why NotifyFileSystemMounted() has to be part of
> > GDataFileSystem::Observer? Who's interested in this event?
> 
> You mean OnFileSystemMounted() in GDataFileSystemInterface::Observer class,
> right?
> 
> Though I'm not sure this is correct answer. This event is caught by
> FileBrowserEventRouter::OnFileSystemMounted(). I think
> GDataFileSystemInterface::Observer is the best place to notify events to file
> manager from file system.

I think the class that does mount/unmount the file system should raise this
event.

[yoshiki, 2012-08-03 07:15:03]

satorux: As discussed offline, moved mount- & unmount-related codes from
file_system.cc to system_service.cc. PTAL

On 2012/08/02 06:40:05, satorux1 wrote:
> On 2012/08/02 06:37:01, yoshiki wrote:
> > On 2012/08/02 06:19:44, satorux1 wrote:
> > > My question was, why NotifyFileSystemMounted() has to be part of
> > > GDataFileSystem::Observer? Who's interested in this event?
> > 
> > You mean OnFileSystemMounted() in GDataFileSystemInterface::Observer class,
> > right?
> > 
> > Though I'm not sure this is correct answer. This event is caught by
> > FileBrowserEventRouter::OnFileSystemMounted(). I think
> > GDataFileSystemInterface::Observer is the best place to notify events to
file
> > manager from file system.
> 
> I think the class that does mount/unmount the file system should raise this
> event.

[satorux1, 2012-08-03 17:28:55]

I think the patch description is not up-to-date. Please fix

http://codereview.chromium.org/10834115/diff/10017/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.h (right):

http://codereview.chromium.org/10834115/diff/10017/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.h:289:
base::WeakPtrFactory<AddMountFunction> weak_ptr_factory_;
I think we don't need this. We don't need this. Extension functions are
ref-counted so base::Bind(&Foo::Func, this) works.

http://codereview.chromium.org/10834115/diff/10017/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

http://codereview.chromium.org/10834115/diff/10017/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:131: // Notifies the
file system is mounted or getting unmounted.
Add separate comments

// Notifies the file system was just mounted.

// Notifies the file system is going to be unmounted.

http://codereview.chromium.org/10834115/diff/10017/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:133: virtual void
NotifyFileSystemUnmounting() = 0;
NotifyFileSystemToBeUnmounted

[yoshiki, 2012-08-03 18:15:08]

PTAL

https://chromiumcodereview.appspot.com/10834115/diff/10017/chrome/browser/chr...
File chrome/browser/chromeos/extensions/file_browser_private_api.h (right):

https://chromiumcodereview.appspot.com/10834115/diff/10017/chrome/browser/chr...
chrome/browser/chromeos/extensions/file_browser_private_api.h:289:
base::WeakPtrFactory<AddMountFunction> weak_ptr_factory_;
On 2012/08/03 17:28:56, satorux1 wrote:
> I think we don't need this. We don't need this. Extension functions are
> ref-counted so base::Bind(&Foo::Func, this) works.

Done.

https://chromiumcodereview.appspot.com/10834115/diff/10017/chrome/browser/chr...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

https://chromiumcodereview.appspot.com/10834115/diff/10017/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:131: // Notifies the
file system is mounted or getting unmounted.
On 2012/08/03 17:28:56, satorux1 wrote:
> Add separate comments
> 
> // Notifies the file system was just mounted.
> 
> // Notifies the file system is going to be unmounted.

Done.

https://chromiumcodereview.appspot.com/10834115/diff/10017/chrome/browser/chr...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:133: virtual void
NotifyFileSystemUnmounting() = 0;
On 2012/08/03 17:28:56, satorux1 wrote:
> NotifyFileSystemToBeUnmounted

Done.

[satorux1, 2012-08-03 18:21:14]

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3404: DVLOG(1) << "File
System is getting unmounted";
is to be unmounted

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:112: // Triggered
when the file system is getting unmounted.
is being unmounted

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:113: virtual void
OnFileSystemUnmounting() {}
OnFileSystemBeingUnmounted

[yoshiki, 2012-08-03 20:06:36]

PTAL

[yoshiki, 2012-08-03 20:10:31]

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3404: DVLOG(1) << "File
System is getting unmounted";
On 2012/08/03 18:21:14, satorux1 wrote:
> is to be unmounted

Done.

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_file_system_interface.h (right):

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:112: // Triggered
when the file system is getting unmounted.
On 2012/08/03 18:21:14, satorux1 wrote:
> is being unmounted

Done.

https://chromiumcodereview.appspot.com/10834115/diff/3023/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system_interface.h:113: virtual void
OnFileSystemUnmounting() {}
On 2012/08/03 18:21:14, satorux1 wrote:
> OnFileSystemBeingUnmounted

Done.

[satorux1, 2012-08-03 20:11:18]

LGTM

http://codereview.chromium.org/10834115/diff/3023/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/mock_gdata_file_system.h (right):

http://codereview.chromium.org/10834115/diff/3023/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/mock_gdata_file_system.h:86:
MOCK_METHOD0(RemoveDriveMountPoint, void());
thank you for removing them. :)