SyncFS: Support resolveLocalFileSystemURL on SyncFileSystem

SyncFS: Support resolveLocalFileSystemURL on SyncFileSystem

window.resolveLocalFileSystemURL is originally defined only for FileSystem API,
but it'd be nice if it works on SyncFileSystem in a similar way. For that this
change adds new IPC messages and interfaces.

Blink side change depends on this:
https://codereview.chromium.org/23537011/


BUG=177137
TEST=manual (Get FileSystemURL for a file on SyncFS, and then resolve it)
TEST=unit_tests
TEST=run_webkit_tests.sh http/tests/inspector/filesystem/\* 
TEST=run_webkit_tests.sh http/tests/inspector/filesystem/\*

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

[nhiroki, 2013-09-05 09:12:07]

Hi, can you take an early look?

(The blink side change is still rough, but I think it'd be helpful to review
this.)

Thanks!

[tzik, 2013-09-05 09:30:25]

https://codereview.chromium.org/23856002/diff/14002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_file_system_service_factory.cc (left):

https://codereview.chromium.org/23856002/diff/14002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_file_system_service_factory.cc:66:
RegisterSyncableFileSystem();
I added RegisterSyncableFileSystem call around here recently.
Could you rebase this patch and remove the call?

https://codereview.chromium.org/23856002/diff/14002/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.h (right):

https://codereview.chromium.org/23856002/diff/14002/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.h:57: const WebKit::WebURL&
storage_partition,
I think this URL is not a storage partition (=security origin).
Could you rename this? (maybe just "path" will work).

https://codereview.chromium.org/23856002/diff/14002/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/23856002/diff/14002/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:280: FileSystemURL url =
CrackURL(GURL(filesystem_url));
extra GURL()?

[kinuko, 2013-09-05 11:11:51]

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:242:
context_->ResolveURL(filesystem_url, base::Bind(
Can we just pass the cracked URL here?

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:189: const std::string& name,
name -> filesystem_name

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:190: const GURL& root,
Shouldn't we give the type information from the browser process too?

[nhiroki, 2013-09-09 09:51:05]

Updated. Can you take another look? Thanks!

https://codereview.chromium.org/23856002/diff/14002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_file_system_service_factory.cc (left):

https://codereview.chromium.org/23856002/diff/14002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_file_system_service_factory.cc:66:
RegisterSyncableFileSystem();
On 2013/09/05 09:30:26, tzik wrote:
> I added RegisterSyncableFileSystem call around here recently.
> Could you rebase this patch and remove the call?

Done.

https://codereview.chromium.org/23856002/diff/14002/content/child/fileapi/web...
File content/child/fileapi/webfilesystem_impl.h (right):

https://codereview.chromium.org/23856002/diff/14002/content/child/fileapi/web...
content/child/fileapi/webfilesystem_impl.h:57: const WebKit::WebURL&
storage_partition,
On 2013/09/05 09:30:26, tzik wrote:
> I think this URL is not a storage partition (=security origin).
> Could you rename this? (maybe just "path" will work).

I see. This contains full filesystem URL, so how about "filesystem_url"?

https://codereview.chromium.org/23856002/diff/14002/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/23856002/diff/14002/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:280: FileSystemURL url =
CrackURL(GURL(filesystem_url));
On 2013/09/05 09:30:26, tzik wrote:
> extra GURL()?

Done.

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:242:
context_->ResolveURL(filesystem_url, base::Bind(
On 2013/09/05 11:11:51, kinuko wrote:
> Can we just pass the cracked URL here?

Done.

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:189: const std::string& name,
On 2013/09/05 11:11:51, kinuko wrote:
> name -> filesystem_name

Done.

https://codereview.chromium.org/23856002/diff/21001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:190: const GURL& root,
On 2013/09/05 11:11:51, kinuko wrote:
> Shouldn't we give the type information from the browser process too?

Done.

[kinuko, 2013-09-09 12:52:22]

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:282: DCHECK(!callback.is_null());
Can we add DCHECK(io_task_runner_->RunsTasksOnCurrentThread())? This one calls
FileSystemOperation methods in its callback chain and cannot be called other
than on IO thread.  (It looks we should have the same DCHECK in OpenFileSystem,
DeleteFileSystem etc)

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:501: void
FileSystemContext::DidGetMetadataForResolveURL(
Does this need to be a method of FSC?

(I guess DidOpenFileSystemForResolveURL can also be a mere function)

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:180: // checks the existence of the
file entry referred by the URL and its file
nit: file type -> filesystem type

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
File webkit/common/fileapi/file_system_info.h (right):

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:13: 
Can we add a struct comment?

"This struct is used to send the necessary information for Blink to create a
DOMFileSystem.  Since Blink side only uses
mount_type (rather than detailed/cracked filesystem type)
this only contains mount_type but not type."

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:20: std::string filesystem_name;
nit: Maybe just 'name' is ok here (since we're in a structure named
FileSystemInfo)

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:25: }
}  // namespace fileapi

[nhiroki, 2013-09-10 06:30:39]

Updated!

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.cc (right):

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:282: DCHECK(!callback.is_null());
On 2013/09/09 12:52:23, kinuko wrote:
> Can we add DCHECK(io_task_runner_->RunsTasksOnCurrentThread())? This one calls
> FileSystemOperation methods in its callback chain and cannot be called other
> than on IO thread.  (It looks we should have the same DCHECK in
OpenFileSystem,
> DeleteFileSystem etc)

Added the check for the ResolveURL.

I'm going to create a separate patch for other methods because adding checks
cause test failures (e.g. the check for OpenFileSystem fails
LocalFileSyncContextTest) and it's not directly related to this issue.

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.cc:501: void
FileSystemContext::DidGetMetadataForResolveURL(
On 2013/09/09 12:52:23, kinuko wrote:
> Does this need to be a method of FSC?
> 
> (I guess DidOpenFileSystemForResolveURL can also be a mere function)

Done.

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/23856002/diff/36001/webkit/browser/fileapi/fi...
webkit/browser/fileapi/file_system_context.h:180: // checks the existence of the
file entry referred by the URL and its file
On 2013/09/09 12:52:23, kinuko wrote:
> nit: file type -> filesystem type

Done (deleted the part thanks to introducing FileSystemInfo).

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
File webkit/common/fileapi/file_system_info.h (right):

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:13: 
On 2013/09/09 12:52:23, kinuko wrote:
> Can we add a struct comment?
> 
> "This struct is used to send the necessary information for Blink to create a
> DOMFileSystem.  Since Blink side only uses
> mount_type (rather than detailed/cracked filesystem type)
> this only contains mount_type but not type."

Done.

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:20: std::string filesystem_name;
On 2013/09/09 12:52:23, kinuko wrote:
> nit: Maybe just 'name' is ok here (since we're in a structure named
> FileSystemInfo)

Done.

https://codereview.chromium.org/23856002/diff/36001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:25: }
On 2013/09/09 12:52:23, kinuko wrote:
> }  // namespace fileapi

Done.

[kinuko, 2013-09-10 07:02:02]

ok, lgtm.  (You'll need security review for IPC changes)

Please also run layout tests (linux_layout_rel etc) in addition to default try
tests before landing.

https://codereview.chromium.org/23856002/diff/46001/webkit/common/fileapi/fil...
File webkit/common/fileapi/file_system_info.h (right):

https://codereview.chromium.org/23856002/diff/46001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:21: fileapi::FileSystemType
mount_type);
nit: I guess you may also need out-of-line dtor (for clang build).

[tzik, 2013-09-10 07:38:03]

lgtm

[nhiroki, 2013-09-12 05:08:58]

Can you take another look?

I fixed error handling because some layout tests can be failed due to it.

Thanks!

https://codereview.chromium.org/23856002/diff/46001/webkit/common/fileapi/fil...
File webkit/common/fileapi/file_system_info.h (right):

https://codereview.chromium.org/23856002/diff/46001/webkit/common/fileapi/fil...
webkit/common/fileapi/file_system_info.h:21: fileapi::FileSystemType
mount_type);
On 2013/09/10 07:02:02, kinuko wrote:
> nit: I guess you may also need out-of-line dtor (for clang build).

Done.

[nhiroki, 2013-09-12 06:18:31]

+tsepez@

Hi Tom, can you review file_system_messages.h?

This change adds new IPC messages: FileSystemMsg_DidResolveURL and
FileSystemHostMsg_ResolveURL. These are used to open a filesystem as read-only,
and to send back filesystem information and file metadata at the given URL like
other filesystem messages (e.g. *OpenFileSystem messages).

Thanks,
nhiroki

[Tom Sepez, 2013-09-12 16:32:49]

LGTM - permissions check in OnResolveURL() seems fine.

[commit-bot: I haz the power, 2013-09-16 14:48:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/23856002/53001

[commit-bot: I haz the power, 2013-09-16 14:52:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/23856002/53001

[commit-bot: I haz the power, 2013-09-16 14:57:04]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-17 14:03:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/23856002/88001

[commit-bot: I haz the power, 2013-09-17 17:51:52]

Change committed as 223635