Add mechanism to auto mount file systems in response to a URL request.

Add mechanism to auto mount file systems in response to a URL request.

This code adds a hook for when a file system URL request can not be cracked.
It will allow external media galleries file systems to be lazily created.

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

[vandebo (ex-Chrome), 2014-03-11 23:50:48]

PTAL - The work in progress consumer of this is:
https://codereview.chromium.org/185393012/diff/40001/chrome/browser/media_gal...

https://codereview.chromium.org/195923002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/195923002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:2555: }
Media galleries hook will go here.

[kinuko, 2014-03-12 05:50:39]

Interesting. +tzik for second reviewer, cc-ing mtomasz in case it may interest
him.

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:343: void
FileSystemContext::AttemptAutoMountForURLRequest(
Will you need this only for URL request? Do we also want to do similar for
ResolveURL?

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:75: // does not recognize the URL,
it returns false and does not call |callback|.
nit: would be nice to also note that it's called on IO thread.

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_url_request_job_factory.h (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_url_request_job_factory.h:26:
CreateFileSystemProtocolHandler(const std::string& partition_domain,
nit: prefer using the same term (partition_domain vs storage_domain)

[kinuko, 2014-03-12 05:51:39]

(cc-ing mtomasz in case it may interest him, this time really does so)

[vandebo (ex-Chrome), 2014-03-12 18:35:54]

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:343: void
FileSystemContext::AttemptAutoMountForURLRequest(
On 2014/03/12 05:50:40, kinuko wrote:
> Will you need this only for URL request? Do we also want to do similar for
> ResolveURL?

I initially tried making this part of ResolveURL or OpenFileSystem, but neither
had the information needed.  Specifically, from the URLRequest you can get back
the RenderViewHost of the requestor.

I'm still not clear where Open and Resolve would be called for a media gallery
file system.

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:75: // does not recognize the URL,
it returns false and does not call |callback|.
On 2014/03/12 05:50:40, kinuko wrote:
> nit: would be nice to also note that it's called on IO thread.

Done.

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_url_request_job_factory.h (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_url_request_job_factory.h:26:
CreateFileSystemProtocolHandler(const std::string& partition_domain,
On 2014/03/12 05:50:40, kinuko wrote:
> nit: prefer using the same term (partition_domain vs storage_domain)

Agreed.  storage_partition_impl_map.cc uses partition_domain throughout. I think
outside that contest, storage_domain is more clear, so I've called the new
references outside of there storage_domain.

[kinuko, 2014-03-13 04:19:19]

lgtm

(tzik: can you do a secondary review esp. for unittests?)

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:117:
std::vector<URLRequestAutoMountHandler> auto_mount_handlers,
const ref?

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:343: void
FileSystemContext::AttemptAutoMountForURLRequest(
On 2014/03/12 18:35:54, vandebo wrote:
> On 2014/03/12 05:50:40, kinuko wrote:
> > Will you need this only for URL request? Do we also want to do similar for
> > ResolveURL?
> 
> I initially tried making this part of ResolveURL or OpenFileSystem, but
neither
> had the information needed.  Specifically, from the URLRequest you can get
back
> the RenderViewHost of the requestor.
> 
> I'm still not clear where Open and Resolve would be called for a media gallery
> file system.

(Assuming you're going to support toURL for media gallery files) ResolveURL will
be called if one tries to get a FileEntry from a filesystem: URL by calling
webkitResolveLocalFileSystemURL.

http://www.w3.org/TR/file-system-api/#widl-LocalFileSystem-resolveLocalFileSy...

If it's not expected usage it's ok to leave it in this patch.

(Btw what's missing in ResolveURL for your case? Will you need url_request info
other than storage_domain?  I'd like to know if it could be a TODO issue we can
fix later, since the 'AutoMount' feature itself looks useful but limiting this
to RequestURL only will narrow the potential future application)

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
File content/browser/fileapi/file_system_dir_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
content/browser/fileapi/file_system_dir_url_request_job_unittest.cc:47: static
const char kValidExternalMountPoint[] = "mnt_name";
nit: no need of static inside anon namespace? (And I realized we have static too
on the line above)

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
content/browser/fileapi/file_system_dir_url_request_job_unittest.cc:65:
std::string mount_point = base::WideToUTF8(components[0]);
FilePath(components[0]).AsUTF8Unsafe() would work for either case?

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
File content/browser/fileapi/file_system_url_request_job_unittest.cc (right):

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
content/browser/fileapi/file_system_url_request_job_unittest.cc:73: std::string
mount_point = base::WideToUTF8(components[0]);
ditto, AsUTF8Unsafe() should work?

https://codereview.chromium.org/195923002/diff/80001/content/public/test/test...
File content/public/test/test_file_system_context.h (right):

https://codereview.chromium.org/195923002/diff/80001/content/public/test/test...
content/public/test/test_file_system_context.h:38:
std::vector<fileapi::URLRequestAutoMountHandler> auto_mounters,
const ref

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:117: // mounted.
Can you also add a note that it's hooked when filesystem: URL is accessed?

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:125:
std::vector<URLRequestAutoMountHandler> auto_mount_handlers,
const ref

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_url_request_job_factory.cc (right):

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_url_request_job_factory.cc:68: const
std::string& storage_domain_, FileSystemContext* context) {
nit: no trailing _ for arg

[tzik, 2014-03-13 05:36:06]

lgtm

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:347: FileSystemURL filesystem_url
= FileSystemURL(url_request->url());
Can this be like this?
FileSystemURL filesystem_url(url_request->url());

[vandebo (ex-Chrome), 2014-03-13 20:19:32]

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:117:
std::vector<URLRequestAutoMountHandler> auto_mount_handlers,
On 2014/03/13 04:19:19, kinuko wrote:
> const ref?

Oops.  Fixed.

https://codereview.chromium.org/195923002/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:343: void
FileSystemContext::AttemptAutoMountForURLRequest(
On 2014/03/13 04:19:19, kinuko wrote:
> On 2014/03/12 18:35:54, vandebo wrote:
> > On 2014/03/12 05:50:40, kinuko wrote:
> > > Will you need this only for URL request? Do we also want to do similar for
> > > ResolveURL?
> > 
> > I initially tried making this part of ResolveURL or OpenFileSystem, but
> neither
> > had the information needed.  Specifically, from the URLRequest you can get
> back
> > the RenderViewHost of the requestor.
> > 
> > I'm still not clear where Open and Resolve would be called for a media
gallery
> > file system.
> 
> (Assuming you're going to support toURL for media gallery files) ResolveURL
will
> be called if one tries to get a FileEntry from a filesystem: URL by calling
> webkitResolveLocalFileSystemURL.
> 
>
http://www.w3.org/TR/file-system-api/#widl-LocalFileSystem-resolveLocalFileSy...
> 
> If it's not expected usage it's ok to leave it in this patch.

Hmm... That's specifically for "Local" filesystem URLs.  I think it's ok to
return an error for media galleries fs URLs.

> (Btw what's missing in ResolveURL for your case? Will you need url_request
info
> other than storage_domain?  I'd like to know if it could be a TODO issue we
can
> fix later, since the 'AutoMount' feature itself looks useful but limiting this
> to RequestURL only will narrow the potential future application)

So the thing in URLRequest that makes it all work is that back up in the content
layer, I can use ResourceRequestInfo to get back to a RenderViewHost and
Profile.  See this WIP CL:
https://codereview.chromium.org/185393012/diff/40001/chrome/browser/media_gal...

The media galleries code ensures that the FS is cleaned up when all the RVH
consumers of it go away.

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
File content/browser/fileapi/file_system_dir_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
content/browser/fileapi/file_system_dir_url_request_job_unittest.cc:47: static
const char kValidExternalMountPoint[] = "mnt_name";
On 2014/03/13 04:19:20, kinuko wrote:
> nit: no need of static inside anon namespace? (And I realized we have static
too
> on the line above)

Done.

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
content/browser/fileapi/file_system_dir_url_request_job_unittest.cc:65:
std::string mount_point = base::WideToUTF8(components[0]);
On 2014/03/13 04:19:20, kinuko wrote:
> FilePath(components[0]).AsUTF8Unsafe() would work for either case?

Yea, but then you make an unnecessary object...  I guess for tests it doesn't
matter. Done.

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
File content/browser/fileapi/file_system_url_request_job_unittest.cc (right):

https://codereview.chromium.org/195923002/diff/80001/content/browser/fileapi/...
content/browser/fileapi/file_system_url_request_job_unittest.cc:73: std::string
mount_point = base::WideToUTF8(components[0]);
On 2014/03/13 04:19:20, kinuko wrote:
> ditto, AsUTF8Unsafe() should work?

Done.

https://codereview.chromium.org/195923002/diff/80001/content/public/test/test...
File content/public/test/test_file_system_context.h (right):

https://codereview.chromium.org/195923002/diff/80001/content/public/test/test...
content/public/test/test_file_system_context.h:38:
std::vector<fileapi::URLRequestAutoMountHandler> auto_mounters,
On 2014/03/13 04:19:20, kinuko wrote:
> const ref

Done.

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.cc:347: FileSystemURL filesystem_url
= FileSystemURL(url_request->url());
On 2014/03/13 05:36:07, tzik wrote:
> Can this be like this?
> FileSystemURL filesystem_url(url_request->url());

Done.

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:117: // mounted.
On 2014/03/13 04:19:20, kinuko wrote:
> Can you also add a note that it's hooked when filesystem: URL is accessed?

Done.

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:125:
std::vector<URLRequestAutoMountHandler> auto_mount_handlers,
On 2014/03/13 04:19:20, kinuko wrote:
> const ref

Done.

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_url_request_job_factory.cc (right):

https://codereview.chromium.org/195923002/diff/80001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_url_request_job_factory.cc:68: const
std::string& storage_domain_, FileSystemContext* context) {
On 2014/03/13 04:19:20, kinuko wrote:
> nit: no trailing _ for arg

Done.

[vandebo (ex-Chrome), 2014-03-13 20:20:52]

+joi for content/public
+sky for remaining owners approval (not content public or fileapi)

[sky, 2014-03-13 21:40:20]

LGTM

[Jói, 2014-03-14 12:20:03]

//content/public LGTM

[vandebo (ex-Chrome), 2014-03-14 15:09:06]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-14 15:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/195923002/120001

[commit-bot: I haz the power, 2014-03-14 17:36:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-14 17:36:13]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[vandebo (ex-Chrome), 2014-03-18 21:35:15]

The CQ bit was checked by vandebo@chromium.org

[vandebo (ex-Chrome), 2014-03-18 21:36:39]

tzik: I had to make a few changes to tests to make things work across all
platforms. Please take a quick look at the delta and tell me if there's anything
you'd like me to tweak.

[commit-bot: I haz the power, 2014-03-18 21:41:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/195923002/200001

[commit-bot: I haz the power, 2014-03-18 22:17:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 22:17:52]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[vandebo (ex-Chrome), 2014-03-19 15:18:25]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-19 15:18:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/195923002/200001

[commit-bot: I haz the power, 2014-03-19 16:40:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-19 16:40:57]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[vandebo (ex-Chrome), 2014-03-19 16:49:07]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-19 16:50:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/195923002/200001

[commit-bot: I haz the power, 2014-03-19 19:40:35]

Change committed as 258064

[pneubeck (no reviews), 2014-03-20 10:00:07]

A revert of this CL has been created in
https://codereview.chromium.org/206253002/ by pneubeck@chromium.org.

The reason for reverting is: Best guess that this broke
SyncFileSystemApiTest.WriteFileThenGetUsage
SyncFileSystemApiTest.GetFileStatuses
MediaGalleriesPlatformAppBrowserTest.MediaGalleriesCopyTo
on XP Tests(1)..

[pneubeck (no reviews), 2014-03-20 16:20:06]

On 2014/03/20 10:00:07, pneubeck wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/206253002/ by mailto:pneubeck@chromium.org.
> 
> The reason for reverting is: Best guess that this broke
> SyncFileSystemApiTest.WriteFileThenGetUsage
> SyncFileSystemApiTest.GetFileStatuses
> MediaGalleriesPlatformAppBrowserTest.MediaGalleriesCopyTo
> on XP Tests(1)..

Seems not to have caused the test failures.
See crbug.com/354425 for details.