Integrate external mount points to IsolatedContext

Integrate external mount points to IsolatedContext

* To support MTP/Media filesystems in CrOS's FileBrowser
* To eventually support external, persistent mount points in sans-CrOS chrome (currently ifdef'ed only for cros since we need http://crbug.com/142289 to do the same in chrome)
* To introduce more finer-grained filesystem types
* To reduce duplicated code

What this patch actually does:
- Add external mount point support in IsolatedContext
- Introduce new filesystem types, NativeLocal and GData, to represent file systems supported by CrOS
- Replace CrOSMountPointProvider's internal mount map with IsolatedContext

BUG=139223
TEST=manually tested
TEST=existing tests

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

[kinuko, 2012-08-14 08:10:16]

This is the first cut for supporting MTP filesystem in CrosMountPointProvider. 
This does not do anything for MTP yet but integrate CrosMountPointProvider with
IsolatedContext so that we can later easily plug into MTP/Media file system
code.

zel: since this patch largely changes internal implementation of
CrosMountPointProvider, could you take a look to see if general direction looks
good to you?  Thanks!

[kmadhusu, 2012-08-14 18:01:13]

Drive-by comments.

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:508: if
(!fileapi::IsCrosManagedFileSystemType(url.type()))
nit: if (!url.is_valid() || !fileapi::IsCros...)

Looks like this check is done at several places in this file and at
file_handler_util.cc. Can we have a helper function to do that?

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:604: if
(!file_util::PathExists(url.path()) ||
nit: url.path() and url.virtual_path() are called several times in the same
function. Can we store the results and re-use it?

FilePath local_path = url.path();
FilePath virtual_path = url.virtual_path();

http://codereview.chromium.org/10823273/diff/1036/content/browser/fileapi/fil...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10823273/diff/1036/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:775: return true;
Don't we need to set *error if HasPermissionsForFileSystem call failed? Is there
any situation when there is no permission to access the file system but there is
permission to read the file?

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.h:36: // Returns the path part of the original
filesysstem url.
nit: filesystem typo

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/isolated_con...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/isolated_con...
webkit/fileapi/isolated_context.h:224: ISOLATED,
nit: Can we have TEMPORARY and PERMANENT types here?

[kinuko, 2012-08-16 08:05:05]

Kausalya, thanks for your comments.  I will address them once cros team agrees
with the general direction of this patch.

[zel, 2012-08-18 00:22:08]

LGTM - for ChromeOS part

satorux@, please take a look at this as well

[satorux1, 2012-08-20 03:30:22]

LGTM with nits

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:597: bool is_gdata_file
= url.type() == fileapi::kFileSystemTypeGData;
is_gdata_file -> is_drive_file

kFileSystemTypeGData -> kFileSystemTypeDrive

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:161: case kFileSystemTypeGData:
could you rename this to kFileSystemTypeDrive? we'll stop calling "gdata" for
Drive stuff.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:141: bool FILEAPI_EXPORT
IsCrosManagedFileSystemType(FileSystemType type);
function comment is missing. please describe what Cros Managed FileSystem is.

[satorux1, 2012-08-20 03:51:43]

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.h:34: const FilePath& path() const { return
path_; }
function comment is missing. An example would be helpful.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.h:38: const FilePath& virtual_path() const {
return virtual_path_; }
could you describe a bit more about the difference between path()? Examples
would be helpful to understand.

[tzik, 2012-08-20 03:58:50]

LGTM

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url.cc (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.cc:44: url.path_ = path;
Could you invalidate virtual_path here?

[kinuko, 2012-08-20 08:54:35]

Thanks for your review!

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:508: if
(!fileapi::IsCrosManagedFileSystemType(url.type()))
On 2012/08/14 18:01:13, kmadhusu wrote:
> nit: if (!url.is_valid() || !fileapi::IsCros...)
> 
> Looks like this check is done at several places in this file and at
> file_handler_util.cc. Can we have a helper function to do that?

Done.

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:597: bool is_gdata_file
= url.type() == fileapi::kFileSystemTypeGData;
On 2012/08/20 03:30:23, satorux1 wrote:
> is_gdata_file -> is_drive_file
> 
> kFileSystemTypeGData -> kFileSystemTypeDrive

Done.

http://codereview.chromium.org/10823273/diff/1036/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:604: if
(!file_util::PathExists(url.path()) ||
On 2012/08/14 18:01:13, kmadhusu wrote:
> nit: url.path() and url.virtual_path() are called several times in the same
> function. Can we store the results and re-use it?
> 
> FilePath local_path = url.path();
> FilePath virtual_path = url.virtual_path();

Done.

http://codereview.chromium.org/10823273/diff/1036/content/browser/fileapi/fil...
File content/browser/fileapi/fileapi_message_filter.cc (right):

http://codereview.chromium.org/10823273/diff/1036/content/browser/fileapi/fil...
content/browser/fileapi/fileapi_message_filter.cc:775: return true;
On 2012/08/14 18:01:13, kmadhusu wrote:
> Don't we need to set *error if HasPermissionsForFileSystem call failed? Is
there
> any situation when there is no permission to access the file system but there
is
> permission to read the file? 

I needed this hack for cros's external mount points. I added
GrantReadWriteFileSystem for them in file_browser_private_api to revert this
change.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url.cc (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.cc:44: url.path_ = path;
On 2012/08/20 03:58:50, tzik wrote:
> Could you invalidate virtual_path here?

Done.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.h:34: const FilePath& path() const { return
path_; }
On 2012/08/20 03:51:43, satorux1 wrote:
> function comment is missing. An example would be helpful.
> 

Done.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.h:36: // Returns the path part of the original
filesysstem url.
On 2012/08/14 18:01:13, kmadhusu wrote:
> nit: filesystem typo

Done.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.h:38: const FilePath& virtual_path() const {
return virtual_path_; }
On 2012/08/20 03:51:43, satorux1 wrote:
> could you describe a bit more about the difference between path()? Examples
> would be helpful to understand.

Done.  Also renamed virtual_path() to path_exposed_in_url() to try to make it
look clearer.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.cc (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.cc:161: case kFileSystemTypeGData:
On 2012/08/20 03:30:23, satorux1 wrote:
> could you rename this to kFileSystemTypeDrive? we'll stop calling "gdata" for
> Drive stuff.

Done.

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_util.h:141: bool FILEAPI_EXPORT
IsCrosManagedFileSystemType(FileSystemType type);
On 2012/08/20 03:30:23, satorux1 wrote:
> function comment is missing. please describe what Cros Managed FileSystem is.

Done.  (Removed this method but instead added static function
CrosMountPointProvider::CanHandleURL() with a comment)

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/isolated_con...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/isolated_con...
webkit/fileapi/isolated_context.h:224: ISOLATED,
On 2012/08/14 18:01:13, kmadhusu wrote:
> nit: Can we have TEMPORARY and PERMANENT types here? 

Do you mean we should use the term TEMPORARY/PERMANENT rather than
ISOLATED/EXTERNAL? I'm afraid it may confuse which type is referred by what type
name.  I've added comments for each type instead.

(I also moved this into .cc to make it private)

[kinuko, 2012-08-20 10:37:53]

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_url.h (right):

http://codereview.chromium.org/10823273/diff/1036/webkit/fileapi/file_system_...
webkit/fileapi/file_system_url.h:38: const FilePath& virtual_path() const {
return virtual_path_; }
On 2012/08/20 08:54:35, kinuko wrote:
> On 2012/08/20 03:51:43, satorux1 wrote:
> > could you describe a bit more about the difference between path()? Examples
> > would be helpful to understand.
> 
> Done.  Also renamed virtual_path() to path_exposed_in_url() to try to make it
> look clearer.

Per offline chat with tzik@ I renamed this back to virtual_path() as it might be
more intuitive.

[tzik, 2012-08-20 13:03:03]

LGTM

http://codereview.chromium.org/10823273/diff/15024/webkit/fileapi/file_system...
File webkit/fileapi/file_system_types.h (right):

http://codereview.chromium.org/10823273/diff/15024/webkit/fileapi/file_system...
webkit/fileapi/file_system_types.h:45: // Indicates a transient, isolated file
system for dragge files (which could
s/dragge/dragged/

http://codereview.chromium.org/10823273/diff/15024/webkit/fileapi/file_system...
webkit/fileapi/file_system_types.h:57: // Indicates a drive filesystem.
Could you add some description for this filesystem?

[kmadhusu, 2012-08-21 01:01:06]

Thanks for addressing drive-by comments. lgtm

[commit-bot: I haz the power, 2012-08-21 04:11:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/10823273/15024

[kinuko, 2012-08-21 09:59:16]

Fixed cros tests (gotten broken after the last change). I shouldn't have applied
filesystem_id based permission check to external file systems.  Taiju, could you
take one more look? Thanks!!!!

[kinuko, 2012-08-21 14:55:23]

Submitting this now.