Make isolated file system works for a device root (e.g. X:\\)

Make isolated file system works for a device root (e.g. X:\\)

* directory drag-and-drop should work for a USB drive etc
* Media device intent should work on Windows

BUG=134828, 135010
TEST=manually tested
TEST=IsolatedContext\*

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

[benwells, 2012-06-28 20:07:39]

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.h:70: std::string RegisterFileSystem(const
std::vector<FileInfo>& files);
Is there a way to make this change that doesn't need an API change at this level
(although I like the new convenience method for when you only have one file)?

[tzik, 2012-06-29 02:56:35]

Looks good, overall.

http://codereview.chromium.org/10713007/diff/14001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10713007/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:521: std::set<FilePath>
filesets;
can be removed?

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.cc (right):

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.cc:167: }
We can use std::vector::assign if we use std::set as FileMap.

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.h:127: typedef std::map<std::string, FileInfo>
FileMap;
Can we use std::set instead?

[kinuko, 2012-06-29 08:31:22]

http://codereview.chromium.org/10713007/diff/14001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10713007/diff/14001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:521: std::set<FilePath>
filesets;
On 2012/06/29 02:56:35, tzik wrote:
> can be removed?

Done.

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.cc (right):

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.cc:167: }
On 2012/06/29 02:56:35, tzik wrote:
> We can use std::vector::assign if we use std::set as FileMap.

Done.

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.h:127: typedef std::map<std::string, FileInfo>
FileMap;
On 2012/06/29 02:56:35, tzik wrote:
> Can we use std::set instead?

Done.

[kinuko, 2012-06-29 10:11:32]

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10713007/diff/14001/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.h:70: std::string RegisterFileSystem(const
std::vector<FileInfo>& files);
On 2012/06/28 20:07:39, benwells wrote:
> Is there a way to make this change that doesn't need an API change at this
level
> (although I like the new convenience method for when you only have one file)?

The new API is trying to let the caller pick a name for a path rather than
blindly use the basename of the given path. I think this has following
advantages (though the callsite may look noisier):

- The caller can now register paths with arbitrary names. (Though currently we
always call GetNameForPath)
- The caller can register multiple paths with the same base name in one file
system by using different names (e.g. by appending suffix etc) on its own.
- The caller can make sure it uses the same name with the IsolatedContext when
it looks up a file in the file system (*), because the name must be given by the
caller itself.

(*) For example the response.baseName in the code [1] should be same name the
one registered in the context.

[1]
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/resources/...

[benwells, 2012-06-29 17:41:09]

lgtm

http://codereview.chromium.org/10713007/diff/20003/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10713007/diff/20003/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:538: 
Nit - should the TODO about making sure names are unique move here?

http://codereview.chromium.org/10713007/diff/20003/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10713007/diff/20003/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.h:34: std::string name;
Nit - this comment, and maybe the field name, should change, as it is not
necessarily the name of the file.

[kinuko, 2012-07-02 13:51:54]

Slightly changed the IsolatedContext::RegisterFileSystem interface so that the
caller 1) does not need to call GetNameForPath for each path, or 2) does not to
need to take care of duplicated names.  Confirmed that it doesn't break DnD,
media gallery and native file access code.
Can you take one more look?  Thanks!

http://codereview.chromium.org/10713007/diff/20003/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10713007/diff/20003/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:538: 
On 2012/06/29 17:41:09, benwells wrote:
> Nit - should the TODO about making sure names are unique move here?

Done in a way that adding a new helper class, FileInfoSet, which has the
convenient method AddPath that checks the duplication and appends suffix if
necessary.

http://codereview.chromium.org/10713007/diff/20003/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.h (right):

http://codereview.chromium.org/10713007/diff/20003/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.h:34: std::string name;
On 2012/06/29 17:41:09, benwells wrote:
> Nit - this comment, and maybe the field name, should change, as it is not
> necessarily the name of the file.

Done.

[tzik, 2012-07-03 06:17:51]

lgtm

[benwells, 2012-07-03 06:32:37]

still lgtm

[jochen (gone - plz use gerrit), 2012-07-03 12:32:36]

content/ changes look good

[jam, 2012-07-03 17:50:51]

content lgtm

[kinuko, 2012-07-04 05:23:12]

Can thestig@ or someone in media gallery team take a look at the changes in
chrome/browser/{,extensions/api}/media_gallery/?

[kmadhusu, 2012-07-09 16:37:21]

chrome/browser/{,extensions/api}/media_gallery/ changes LGTM