gdata: Support opening zip file on Google Docs.

gdata: Support opening zip file on Google Docs.

Pass the file extension in the 2nd argument to DiskMountManager::MountPath
to indicate the archive format (e.g.: .zip, .rar, etc.)

When mounting and unmounting archive files in GData cache, call
GDataFileSystem::SetMountedState to set or clear the "mounted" state
accordingly.

BUG=chromium-os:28678
TEST=Verify that mounting/unmounting archive files on GData works correctly.

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

[hshi, 2012-04-18 23:38:12]

This is part#2 CL (originally from (http://codereview.chromium.org/10008100/), I
split it into two CLs to make things more manageable).

This change modifies dbus, disks, and extensions. Mainly (a) pass file extension
to the "source_format" argument, and (b) set or clear the "mounted" state
accordingly when mounting/unmounting archive files.

[satorux1, 2012-04-19 00:14:55]

This patch and http://codereview.chromium.org/10116044/ have the same patch
title:

gdata: Support mounting archive files in GData cache

Can you give them separate patch titles?

[hshi, 2012-04-19 00:27:03]

Does the new title look ok? It is not very descriptive but I don't know if I can
fit more info in one sentence. Thanks!

On 2012/04/19 00:14:55, satorux1 wrote:
> This patch and http://codereview.chromium.org/10116044/ have the same patch
> title:
> 
> gdata: Support mounting archive files in GData cache
> 
> Can you give them separate patch titles?

[satorux1, 2012-04-19 00:31:32]

sounds good!

[hshi, 2012-04-24 01:27:51]

This is the second part change for bug 28678. It deals with
extensions/dbus/disks changes. Please review, thanks!

First part got submitted this afternoon (rev. 133571).

[hshi, 2012-04-24 19:49:33]

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

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1104: }
Please review this patch set #5. My previous patch set #4 causes the browser
tests (ExtensionFileBroswerPrivateApiTest) to fail because it alters the
RemoveMount behavior.

Here we should not bail out prematurally if the source_path associated with the
mount_path does not exist. The test still expects a call to
disk_mount_manager->UnmountPath as long as the mount_path itself is non-empty.

[satorux1, 2012-04-24 20:46:53]

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

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1032:
OnMountedStateSet(mount_type_str, FilePath::StringType(),
can you add some comment why we are passing  FilePath::StringType()?

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1102: if (it !=
mount_points.end()) {
nit: remove {} per the chromium style.

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1106:
disk_mount_manager->UnmountPath(mount_path);
Please move this to right after mount_path is computed:

const std::string& mount_path = files[0].path.value();

In other words, we don't need to compute|source_path| to unmount this.

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1128: // Ignore
the file_path argument for now.
remove this comment and change the parameter to:

const FilePath& /* file_path */) {

http://codereview.chromium.org/10083067/diff/45001/chromeos/dbus/cros_disks_c...
File chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/10083067/diff/45001/chromeos/dbus/cros_disks_c...
chromeos/dbus/cros_disks_client.h:182: const std::string& source_format,
We allow source_format to be empty, and in that case, the format is
auto-detected, right? please explaint it in the comment.

[hshi, 2012-04-24 21:18:22]

Please review Patch Set #6. Toni has pointed out that it is in fact indeed an
error condition if we fail to find the source_path corresponding to the
mount_path in the MountPointMap. The unit test is modified to pass in a valid
path to unmount. Thanks!

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

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1032:
OnMountedStateSet(mount_type_str, FilePath::StringType(),
On 2012/04/24 20:46:53, satorux1 wrote:
> can you add some comment why we are passing  FilePath::StringType()?

I think I should actually pass the display_name here. The OnMountedStateSet
expects to get the filename (base name) for this argument.

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1104: }
Toni told me that it is necessary after all to bail out if the corresponding
source_path does not exist. He suggested me to modify the unit test instead.
(Previously the unit test was incorrectly unmounting a non-existing mount path).

On 2012/04/24 19:49:33, hshi wrote:
> Please review this patch set #5. My previous patch set #4 causes the browser
> tests (ExtensionFileBroswerPrivateApiTest) to fail because it alters the
> RemoveMount behavior.
> 
> Here we should not bail out prematurally if the source_path associated with
the
> mount_path does not exist. The test still expects a call to
> disk_mount_manager->UnmountPath as long as the mount_path itself is non-empty.

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1106:
disk_mount_manager->UnmountPath(mount_path);
I've added comments to explain this.

On 2012/04/24 20:46:53, satorux1 wrote:
> Please move this to right after mount_path is computed:
> 
> const std::string& mount_path = files[0].path.value();
> 
> In other words, we don't need to compute|source_path| to unmount this.

[satorux1, 2012-04-24 22:54:15]

LGTM, but please wait for LGTM from Toni.

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

http://codereview.chromium.org/10083067/diff/45001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1032:
OnMountedStateSet(mount_type_str, FilePath::StringType(),
On 2012/04/24 21:18:22, hshi wrote:
> On 2012/04/24 20:46:53, satorux1 wrote:
> > can you add some comment why we are passing  FilePath::StringType()?
> 
> I think I should actually pass the display_name here. The OnMountedStateSet
> expects to get the filename (base name) for this argument.

Nice. i caught something. :)

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
File chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
chromeos/dbus/cros_disks_client.h:182: // is auto-detected.
please also document valid values for this. maybe ".zip" is valid?

[hshi, 2012-04-24 23:00:04]

Toni, are you fine with my changes to the unit tests? I have built and run the
test locally and verified that the unmount test is passing.

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
File chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
chromeos/dbus/cros_disks_client.h:182: // is auto-detected.
The list of valid values is not small. According to
chrome/browser/resources/file_manager/manifest.json, there are currently 8
extensions (.rar, .tar, .tar.bz2, .tar.gz, .tgz, .tgz2, .tgz, .zip) registered
under the file browser handler "mount-archive". I don't know if these 8 types
are really all supported by CrOS or not (it may vary on different platforms
also)

On 2012/04/24 22:54:15, satorux1 wrote:
> please also document valid values for this. maybe ".zip" is valid?

[satorux1, 2012-04-24 23:01:29]

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
File chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
chromeos/dbus/cros_disks_client.h:182: // is auto-detected.
On 2012/04/24 23:00:04, hshi wrote:
> The list of valid values is not small. According to
> chrome/browser/resources/file_manager/manifest.json, there are currently 8
> extensions (.rar, .tar, .tar.bz2, .tar.gz, .tgz, .tgz2, .tgz, .zip) registered
> under the file browser handler "mount-archive". I don't know if these 8 types
> are really all supported by CrOS or not (it may vary on different platforms
> also)

Then, I'd use ".zip" as an example, and add a reference to the file.
> 
> On 2012/04/24 22:54:15, satorux1 wrote:
> > please also document valid values for this. maybe ".zip" is valid?
>

[Ben Chan, 2012-04-24 23:04:16]

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
File chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
chromeos/dbus/cros_disks_client.h:182: // is auto-detected.
On 2012/04/24 23:00:04, hshi wrote:
> The list of valid values is not small. According to
> chrome/browser/resources/file_manager/manifest.json, there are currently 8
> extensions (.rar, .tar, .tar.bz2, .tar.gz, .tgz, .tgz2, .tgz, .zip) registered
> under the file browser handler "mount-archive". I don't know if these 8 types
> are really all supported by CrOS or not (it may vary on different platforms
> also)
> 
> On 2012/04/24 22:54:15, satorux1 wrote:
> > please also document valid values for this. maybe ".zip" is valid?
> 

They are all supported by CrOS regardless of platform.

But I think the emphasis here should be that, when mounting an archive, we pass
the file extension (with the leading dot) as |source_format|.

[hshi, 2012-04-24 23:10:14]

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
File chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/10083067/diff/52004/chromeos/dbus/cros_disks_c...
chromeos/dbus/cros_disks_client.h:182: // is auto-detected.
On 2012/04/24 23:04:16, Ben Chan wrote:
> On 2012/04/24 23:00:04, hshi wrote:
> > The list of valid values is not small. According to
> > chrome/browser/resources/file_manager/manifest.json, there are currently 8
> > extensions (.rar, .tar, .tar.bz2, .tar.gz, .tgz, .tgz2, .tgz, .zip)
registered
> > under the file browser handler "mount-archive". I don't know if these 8
types
> > are really all supported by CrOS or not (it may vary on different platforms
> > also)
> > 
> > On 2012/04/24 22:54:15, satorux1 wrote:
> > > please also document valid values for this. maybe ".zip" is valid?
> > 
> 
> They are all supported by CrOS regardless of platform.
> 
> But I think the emphasis here should be that, when mounting an archive, we
pass
> the file extension (with the leading dot) as |source_format|.

Done with emphasis on the fact that extension should contain the leading dot.

[hshi, 2012-04-24 23:49:09]

Toni suggested to add one more unit test for removeMount of invalid mount path
(it is expected that UnmountPath be NOT called for this case). I have verified
that the test is working, so uploading Patch Set #8 with the added test.
Otherwise this patch set is identical to Patch Set #7.

[tbarzic, 2012-04-25 00:09:20]

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

http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:417: // Initiate
disk mount operation. Here MountPath auto-detects the
Lose "Here"

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

http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
disk_mount_manager->MountPath(file_path.AsUTF8Unsafe(),
what if MountPath fails?

http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1110:
gdata::GDataSystemService* system_service =
Hmm.. maybe it would be better if we did this in file_browser_event_router.cc
when we get MountCompleted event (and the archive really gets unmounted).
MountPath failure (previous comment) could be handled in the same place

at this point the archive is still mounted, and if Unmount fails, we'll be in an
inconsistent state.

[hshi, 2012-04-25 00:44:17]

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

http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1110:
gdata::GDataSystemService* system_service =
The problem is that the actual path of the archive file to be mounted is
determined by file_system->SetMountedState (this call moves the cached blob from
tmp/<resource_id>.<md5> to persistent/<resouce_id>.<md5>.mounted and returns the
latter to the callback function, which passes it along to MountPath. Thus you've
got to call SetMountedState first, obtain the path, then call MountPath, then
get MountCompleted event. In any case, MountPath can only be invoked after the
cache blob is already moved to persistent/.

On 2012/04/25 00:09:21, tbarzic wrote:
> Hmm.. maybe it would be better if we did this in file_browser_event_router.cc
> when we get MountCompleted event (and the archive really gets unmounted).
> MountPath failure (previous comment) could be handled in the same place
> 
> at this point the archive is still mounted, and if Unmount fails, we'll be in
an
> inconsistent state.

[tbarzic, 2012-04-25 04:07:55]

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

http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1110:
gdata::GDataSystemService* system_service =
On 2012/04/25 00:44:17, hshi wrote:
> The problem is that the actual path of the archive file to be mounted is
> determined by file_system->SetMountedState (this call moves the cached blob
from
> tmp/<resource_id>.<md5> to persistent/<resouce_id>.<md5>.mounted and returns
the
> latter to the callback function, which passes it along to MountPath. Thus
you've
> got to call SetMountedState first, obtain the path, then call MountPath, then
> get MountCompleted event. In any case, MountPath can only be invoked after the
> cache blob is already moved to persistent/.
> 
> On 2012/04/25 00:09:21, tbarzic wrote:
> > Hmm.. maybe it would be better if we did this in
file_browser_event_router.cc
> > when we get MountCompleted event (and the archive really gets unmounted).
> > MountPath failure (previous comment) could be handled in the same place
> > 
> > at this point the archive is still mounted, and if Unmount fails, we'll be
in
> an
> > inconsistent state.
> 

I agree, but I'm talking about reseting mount state in cache after the archive
is unmounted. That part should probably be done when we are certain that the
archive has actually been unmounted.
Also, we should be able to handle MountPath failure using the same code.

p.s. Sorry I didn't catch this sooner.

[hshi1, 2012-04-25 04:32:29]

The unmounting part is straightforward because unmounting occurs before updating
cache state. On the other hand, the mounting part is tricky because the cache
state is updated at the same time the cached blob gets moved to
persistent/*.mounted; to do this properly would require to split the
SetMountedState logic in two parts (pre- and post- mount operations)

Can we do this in a separate CL? Thanks.

On 2012/04/25 04:07:55, tbarzic wrote:
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):
> 
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/file_browser_private_api.cc:1110:
> gdata::GDataSystemService* system_service =
> On 2012/04/25 00:44:17, hshi wrote:
> > The problem is that the actual path of the archive file to be mounted is
> > determined by file_system->SetMountedState (this call moves the cached blob
> from
> > tmp/<resource_id>.<md5> to persistent/<resouce_id>.<md5>.mounted and returns
> the
> > latter to the callback function, which passes it along to MountPath. Thus
> you've
> > got to call SetMountedState first, obtain the path, then call MountPath,
then
> > get MountCompleted event. In any case, MountPath can only be invoked after
the
> > cache blob is already moved to persistent/.
> > 
> > On 2012/04/25 00:09:21, tbarzic wrote:
> > > Hmm.. maybe it would be better if we did this in
> file_browser_event_router.cc
> > > when we get MountCompleted event (and the archive really gets unmounted).
> > > MountPath failure (previous comment) could be handled in the same place
> > > 
> > > at this point the archive is still mounted, and if Unmount fails, we'll be
> in
> > an
> > > inconsistent state.
> > 
> 
> I agree, but I'm talking about reseting mount state in cache after the archive
> is unmounted. That part should probably be done when we are certain that the
> archive has actually been unmounted.
> Also, we should be able to handle MountPath failure using the same code.
> 
> p.s. Sorry I didn't catch this sooner.

[hshi, 2012-04-25 17:13:04]

New patch set removes the "Here". But see my comments regarding the proper
handling of Mount/Unmount failures. Thanks!

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

http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
disk_mount_manager->MountPath(file_path.AsUTF8Unsafe(),
On 2012/04/25 00:09:21, tbarzic wrote:
> what if MountPath fails?

As I explained in a separate reply, the SetMountedState logic (including moving
the cached blob and setting the "mounted" cache state) need to occur before
MountPath is called. If it fails then we would have to "roll back" the move by
making another SetMountedState call (with to_mount == false). 

Question is, if MountPath fails, do we still get a MountCompleted event in
file_browser_event_router.cc? We need to insert the roll-back logic somewhere.

In any case I really hope I can get this CL checked in first. We can keep the
bug 28678 open (in any case it will not be completely fixed until Ben addresses
29439).

[hshi, 2012-04-25 17:33:20]

Toni, how about this?

(1) Remove the SetMountedState(false) call in RemoveMountFunction
(2) In FileBrowserEventRouter::MountCompleted, if mount_type is ARCHIVE:
(2.1) If event type is MOUNTING and error_code is != 0, or,
(2.2) If event type is UNMOUNTING and error_code is == 0, then call
SetMountedState(false) to clear the mounted state.

I was initially worried that I need to touch gdata_file_system.cc again. But if
the change can be limited to file_browser_event_rounter.cc and
file_browser_private_api.cc then it is ok.

On 2012/04/25 17:13:04, hshi wrote:
> New patch set removes the "Here". But see my comments regarding the proper
> handling of Mount/Unmount failures. Thanks!
> 
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):
> 
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
> disk_mount_manager->MountPath(file_path.AsUTF8Unsafe(),
> On 2012/04/25 00:09:21, tbarzic wrote:
> > what if MountPath fails?
> 
> As I explained in a separate reply, the SetMountedState logic (including
moving
> the cached blob and setting the "mounted" cache state) need to occur before
> MountPath is called. If it fails then we would have to "roll back" the move by
> making another SetMountedState call (with to_mount == false). 
> 
> Question is, if MountPath fails, do we still get a MountCompleted event in
> file_browser_event_router.cc? We need to insert the roll-back logic somewhere.
> 
> In any case I really hope I can get this CL checked in first. We can keep the
> bug 28678 open (in any case it will not be completely fixed until Ben
addresses
> 29439).

[tbarzic, 2012-04-25 17:37:00]

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

http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
disk_mount_manager->MountPath(file_path.AsUTF8Unsafe(),
On 2012/04/25 17:13:05, hshi wrote:
> On 2012/04/25 00:09:21, tbarzic wrote:
> > what if MountPath fails?
> 
> As I explained in a separate reply, the SetMountedState logic (including
moving
> the cached blob and setting the "mounted" cache state) need to occur before
> MountPath is called. If it fails then we would have to "roll back" the move by
> making another SetMountedState call (with to_mount == false). 
> 
> Question is, if MountPath fails, do we still get a MountCompleted event in
> file_browser_event_router.cc? We need to insert the roll-back logic somewhere.
> 
> In any case I really hope I can get this CL checked in first. We can keep the
> bug 28678 open (in any case it will not be completely fixed until Ben
addresses
> 29439).

Yes, we will get the MountCompleted event (with the error code). And yeah, we
should probably do the cache rollback there 

(basically, we'd do something like:
 if  (event == mounting && error_code != success ||
      event == unmounting && error_code == success) {
  // Reset cache mount state if the source path is under gdata cache   
  // (mostly, the same code you have in RemoveMount here)
}

I guess you could do this in another patch, but please add 
"TODO(hshi): Handle MountPath failure."  (same for RemoveMount)

[satorux1, 2012-04-25 17:52:09]

On 2012/04/25 17:37:00, tbarzic wrote:
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):
> 
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
> disk_mount_manager->MountPath(file_path.AsUTF8Unsafe(),
> On 2012/04/25 17:13:05, hshi wrote:
> > On 2012/04/25 00:09:21, tbarzic wrote:
> > > what if MountPath fails?
> > 
> > As I explained in a separate reply, the SetMountedState logic (including
> moving
> > the cached blob and setting the "mounted" cache state) need to occur before
> > MountPath is called. If it fails then we would have to "roll back" the move
by
> > making another SetMountedState call (with to_mount == false). 
> > 
> > Question is, if MountPath fails, do we still get a MountCompleted event in
> > file_browser_event_router.cc? We need to insert the roll-back logic
somewhere.
> > 
> > In any case I really hope I can get this CL checked in first. We can keep
the
> > bug 28678 open (in any case it will not be completely fixed until Ben
> addresses
> > 29439).
> 
> Yes, we will get the MountCompleted event (with the error code). And yeah, we
> should probably do the cache rollback there 
> 
> (basically, we'd do something like:
>  if  (event == mounting && error_code != success ||
>       event == unmounting && error_code == success) {
>   // Reset cache mount state if the source path is under gdata cache   
>   // (mostly, the same code you have in RemoveMount here)
> }
> 
> I guess you could do this in another patch, but please add 
> "TODO(hshi): Handle MountPath failure."  (same for RemoveMount)

You could also file a bug and leave a bug URL like crosbug.com/xxxxx in the TODO
comment.

[hshi, 2012-04-25 18:25:25]

Thanks Toni, Satoru. I talked to Toni just a moment ago and realized that this
is not very complicated. So please review patch set #10 -- I have moved to
handling of mounting failures and unmounting successes to
file_browser_event_router.cc where MountCompleted event is handled. Verified
that mount/unmount still works correctly on a Lumpy device. Thanks!

On 2012/04/25 17:37:00, tbarzic wrote:
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):
> 
>
http://codereview.chromium.org/10083067/diff/52007/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
> disk_mount_manager->MountPath(file_path.AsUTF8Unsafe(),
> On 2012/04/25 17:13:05, hshi wrote:
> > On 2012/04/25 00:09:21, tbarzic wrote:
> > > what if MountPath fails?
> > 
> > As I explained in a separate reply, the SetMountedState logic (including
> moving
> > the cached blob and setting the "mounted" cache state) need to occur before
> > MountPath is called. If it fails then we would have to "roll back" the move
by
> > making another SetMountedState call (with to_mount == false). 
> > 
> > Question is, if MountPath fails, do we still get a MountCompleted event in
> > file_browser_event_router.cc? We need to insert the roll-back logic
somewhere.
> > 
> > In any case I really hope I can get this CL checked in first. We can keep
the
> > bug 28678 open (in any case it will not be completely fixed until Ben
> addresses
> > 29439).
> 
> Yes, we will get the MountCompleted event (with the error code). And yeah, we
> should probably do the cache rollback there 
> 
> (basically, we'd do something like:
>  if  (event == mounting && error_code != success ||
>       event == unmounting && error_code == success) {
>   // Reset cache mount state if the source path is under gdata cache   
>   // (mostly, the same code you have in RemoveMount here)
> }
> 
> I guess you could do this in another patch, but please add 
> "TODO(hshi): Handle MountPath failure."  (same for RemoveMount)

[tonibarzic, 2012-04-25 18:39:09]

lgtm

http://codereview.chromium.org/10083067/diff/65001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/filebrowser_mount/test.js (right):

http://codereview.chromium.org/10083067/diff/65001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/filebrowser_mount/test.js:141: function
removeMountNonExisting() {
This will probably fail now :)

[hshi, 2012-04-25 18:48:08]

http://codereview.chromium.org/10083067/diff/65001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/filebrowser_mount/test.js (right):

http://codereview.chromium.org/10083067/diff/65001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/filebrowser_mount/test.js:141: function
removeMountNonExisting() {
Yes I agree that this test no longer makes sense after the previous patch set,
so removing it now.

On 2012/04/25 18:39:09, tonibarzic wrote:
> This will probably fail now :)

[commit-bot: I haz the power, 2012-04-25 18:49:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@google.com/10083067/65004

[commit-bot: I haz the power, 2012-04-25 18:49:59]

Presubmit check for 10083067-65004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
See the JavaScript style guide at
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
and if you have any feedback about the JavaScript PRESUBMIT check, contact
tbreisacher@chromium.org

** Presubmit Warnings **
Found JavaScript style violations in
chrome/test/data/extensions/api_test/filebrowser_mount/test.js:
  line 14: E0001: Extra space after ":"
  totalSize:  1073741824,
            ^^
  line 29: E0001: Extra space after ":"
  totalSize:  47723,
            ^^
  line 44: E0001: Extra space after ":"
  totalSize:  0,
            ^^
  line 95: E0012: Illegal semicolon after function declaration
};
 ^
  line 116: E0131: Single-quoted string preferred over double-quoted string.
    console.warn("Unexpected property found in returned volume");
                 ^
  line 120: E0012: Illegal semicolon after function declaration
};
 ^
  line 123: E0131: Single-quoted string preferred over double-quoted string.
  var testExtensionId = "ddammdhioacbehjngdmkjcjbnfginlla";
                        ^
  line 124: E0131: Single-quoted string preferred over double-quoted string.
  var fileUrl = "filesystem:chrome-extension://" + testExtensionId +
                ^
  line 125: E0131: Single-quoted string preferred over double-quoted string.
                "/external/" + fileName;
                ^
  line 127: E0012: Illegal semicolon after function declaration
};
 ^
  line 132: E0131: Single-quoted string preferred over double-quoted string.
    var fileUrl = createFileUrl("archive/archive_mount_path");
                                ^
  line 147: E0131: Single-quoted string preferred over double-quoted string.
              "getVolumeMetadata result for first volume not as expected");
              ^
  line 157: E0131: Single-quoted string preferred over double-quoted string.
              "getVolumeMetadata result for second volume not as expected");
              ^
  line 167: E0131: Single-quoted string preferred over double-quoted string.
              "getVolumeMetadata result for third volume not as expected");
              ^
  line 173: E0131: Single-quoted string preferred over double-quoted string.
        createFileUrl("removable/non_existent_device_path"),
                      ^
  line 182: E0131: Single-quoted string preferred over double-quoted string.
        createFileUrl("archive/archive_mount_path"),
                      ^
  line 190: E0131: Single-quoted string preferred over double-quoted string.
        "some path",
        ^
  line 191: E0131: Single-quoted string preferred over double-quoted string.
        chrome.test.callbackFail("Invalid mount path."));
                                 ^
  line 203: E0002: Missing space after "+"
                'getMountPoints result[' + i +'] not as expected');
                                             ^

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/resources/file_manager

Presubmit checks took 5.4s to calculate.

[hshi, 2012-04-25 18:53:47]

Adding Robert to review the change in file_manager.js, thanks!

[rginda, 2012-04-25 20:05:36]

file_manager.js lgtm

[commit-bot: I haz the power, 2012-04-25 20:07:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@google.com/10083067/65004

[commit-bot: I haz the power, 2012-04-25 20:07:49]

Presubmit check for 10083067-65004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
See the JavaScript style guide at
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
and if you have any feedback about the JavaScript PRESUBMIT check, contact
tbreisacher@chromium.org

** Presubmit Warnings **
Found JavaScript style violations in
chrome/test/data/extensions/api_test/filebrowser_mount/test.js:
  line 14: E0001: Extra space after ":"
  totalSize:  1073741824,
            ^^
  line 29: E0001: Extra space after ":"
  totalSize:  47723,
            ^^
  line 44: E0001: Extra space after ":"
  totalSize:  0,
            ^^
  line 95: E0012: Illegal semicolon after function declaration
};
 ^
  line 116: E0131: Single-quoted string preferred over double-quoted string.
    console.warn("Unexpected property found in returned volume");
                 ^
  line 120: E0012: Illegal semicolon after function declaration
};
 ^
  line 123: E0131: Single-quoted string preferred over double-quoted string.
  var testExtensionId = "ddammdhioacbehjngdmkjcjbnfginlla";
                        ^
  line 124: E0131: Single-quoted string preferred over double-quoted string.
  var fileUrl = "filesystem:chrome-extension://" + testExtensionId +
                ^
  line 125: E0131: Single-quoted string preferred over double-quoted string.
                "/external/" + fileName;
                ^
  line 127: E0012: Illegal semicolon after function declaration
};
 ^
  line 132: E0131: Single-quoted string preferred over double-quoted string.
    var fileUrl = createFileUrl("archive/archive_mount_path");
                                ^
  line 147: E0131: Single-quoted string preferred over double-quoted string.
              "getVolumeMetadata result for first volume not as expected");
              ^
  line 157: E0131: Single-quoted string preferred over double-quoted string.
              "getVolumeMetadata result for second volume not as expected");
              ^
  line 167: E0131: Single-quoted string preferred over double-quoted string.
              "getVolumeMetadata result for third volume not as expected");
              ^
  line 173: E0131: Single-quoted string preferred over double-quoted string.
        createFileUrl("removable/non_existent_device_path"),
                      ^
  line 182: E0131: Single-quoted string preferred over double-quoted string.
        createFileUrl("archive/archive_mount_path"),
                      ^
  line 190: E0131: Single-quoted string preferred over double-quoted string.
        "some path",
        ^
  line 191: E0131: Single-quoted string preferred over double-quoted string.
        chrome.test.callbackFail("Invalid mount path."));
                                 ^
  line 203: E0002: Missing space after "+"
                'getMountPoints result[' + i +'] not as expected');
                                             ^

Presubmit checks took 6.6s to calculate.

[hshi, 2012-04-25 20:16:53]

Patch Set #12 updates filebrowser_mount/test.js (cosmetic change only).

Apparently commit-bot dislikes double-quotes in javascript. All fixed. These are
not really due to my change, I have no idea why they were allowed before...

[commit-bot: I haz the power, 2012-04-25 20:17:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@google.com/10083067/58008

[commit-bot: I haz the power, 2012-04-25 21:46:50]

Change committed as 133987