FileAPI: Add WebFileSystem::resolveURL

FileAPI: Add WebFileSystem::resolveURL

This introduces WebFileSystem::resolveURL so that resolveLocalFileSystemURL,
a member of FileSystem API, can resolve a filesystem URL for external
filesystem.

This includes:
- Cracking the given filesystem URL not in Blink side but in Chromium side so
that we can get more specific external filesystem type (e.g. SyncFileSystem).
It is necessary to resolve the external filesystem URL.
- Replacing readFileSystem with resolveURL since it's used only to resolve URLs.
- Changing the error code, and these must be correct:
(http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#errors-and-exceptions)

Depends on Chromium side change:
https://codereview.chromium.org/23856002/


BUG=177137
TEST=run_webkit_tests.sh http/tests/filesystem/\*
TEST=run_webkit_tests.sh http/tests/inspector/filesystem/\*

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158157

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

Let me send out some early comments

https://codereview.chromium.org/23537011/diff/8001/Source/core/inspector/Insp...
File Source/core/inspector/InspectorFileSystemAgent.cpp (right):

https://codereview.chromium.org/23537011/diff/8001/Source/core/inspector/Insp...
Source/core/inspector/InspectorFileSystemAgent.cpp:354: if
(!DOMFileSystemBase::crackFileSystemURL(m_url, type, m_path)) {
I guess we can skip this crackFileSystemURL (and probably can deprecate the
method itself)?

https://codereview.chromium.org/23537011/diff/8001/Source/core/inspector/Insp...
Source/core/inspector/InspectorFileSystemAgent.cpp:360:
OwnPtr<ResolveURICallbacks> fileSystemCallbacks =
ResolveURICallbacks::create(successCallback, errorCallback,
scriptExecutionContext, type, m_path);
Now that we can crack the given URL in the browser process the type should be
supplied by the callback. Can't we stop using the type information cracked in
Blink here?

[nhiroki, 2013-09-12 05:15:22]

+tzik@

Updated. Can you take another look? Thanks!

https://codereview.chromium.org/23537011/diff/8001/Source/core/inspector/Insp...
File Source/core/inspector/InspectorFileSystemAgent.cpp (right):

https://codereview.chromium.org/23537011/diff/8001/Source/core/inspector/Insp...
Source/core/inspector/InspectorFileSystemAgent.cpp:354: if
(!DOMFileSystemBase::crackFileSystemURL(m_url, type, m_path)) {
On 2013/09/05 11:12:11, kinuko wrote:
> I guess we can skip this crackFileSystemURL (and probably can deprecate the
> method itself)?

Removed.

> (and probably can deprecate the method itself)?

FYI: Even after this change, DeleteEntryRequest::start (in
InspectorFileSystemAgent.cpp) still uses the cracking function, and we need to
change deleteFileSystem like readFileSystem/resolveURL. I'll consider this
later.

https://codereview.chromium.org/23537011/diff/8001/Source/core/inspector/Insp...
Source/core/inspector/InspectorFileSystemAgent.cpp:360:
OwnPtr<ResolveURICallbacks> fileSystemCallbacks =
ResolveURICallbacks::create(successCallback, errorCallback,
scriptExecutionContext, type, m_path);
On 2013/09/05 11:12:11, kinuko wrote:
> Now that we can crack the given URL in the browser process the type should be
> supplied by the callback. Can't we stop using the type information cracked in
> Blink here?

Done.

[kinuko, 2013-09-12 05:58:14]

https://codereview.chromium.org/23537011/diff/19001/Source/modules/filesystem...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/23537011/diff/19001/Source/modules/filesystem...
Source/modules/filesystem/LocalFileSystem.cpp:65:
client()->resolveURL(fileSystemURL, callbacks);
Hmm, looks like we can directly call
WebKit::Platform::current()->fileSystem()->resolveURL() without asking
FileSystemClient impl to do the task, as we no longer call any Source/web
specific code there after recent cleanup.  (As well as openFileSystem and
deleteFileSystem, they can be also removed from FileSystemClient.h interface)

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

Updated, ptal.

https://codereview.chromium.org/23537011/diff/19001/Source/modules/filesystem...
File Source/modules/filesystem/LocalFileSystem.cpp (right):

https://codereview.chromium.org/23537011/diff/19001/Source/modules/filesystem...
Source/modules/filesystem/LocalFileSystem.cpp:65:
client()->resolveURL(fileSystemURL, callbacks);
On 2013/09/12 05:58:14, kinuko wrote:
> Hmm, looks like we can directly call
> WebKit::Platform::current()->fileSystem()->resolveURL() without asking
> FileSystemClient impl to do the task, as we no longer call any Source/web
> specific code there after recent cleanup.  (As well as openFileSystem and
> deleteFileSystem, they can be also removed from FileSystemClient.h interface)

Done. I'll make a separate patch for other functions.

[kinuko, 2013-09-12 10:55:58]

lgtm

https://codereview.chromium.org/23537011/diff/29001/LayoutTests/http/tests/fi...
File LayoutTests/http/tests/filesystem/workers/resolve-url-sync-expected.txt
(right):

https://codereview.chromium.org/23537011/diff/29001/LayoutTests/http/tests/fi...
LayoutTests/http/tests/filesystem/workers/resolve-url-sync-expected.txt:30:
[Worker] A URI supplied to the API was malformed, or the resulting Data URL has
exceeded the URL length limitations for Data URLs.
In the issue description can you also note that we changed the error code, and
also note that the new error code must be correct
(http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#errors-and-exceptions)

https://codereview.chromium.org/23537011/diff/29001/Source/modules/filesystem...
File Source/modules/filesystem/FileSystemClient.h (right):

https://codereview.chromium.org/23537011/diff/29001/Source/modules/filesystem...
Source/modules/filesystem/FileSystemClient.h:41: class KURL;
Not necessary any more?

[tzik, 2013-09-13 05:07:39]

lgtm

[nhiroki, 2013-09-13 07:33:43]

Thank you for reviewing!

https://codereview.chromium.org/23537011/diff/29001/Source/modules/filesystem...
File Source/modules/filesystem/FileSystemClient.h (right):

https://codereview.chromium.org/23537011/diff/29001/Source/modules/filesystem...
Source/modules/filesystem/FileSystemClient.h:41: class KURL;
On 2013/09/12 10:55:59, kinuko wrote:
> Not necessary any more?

Done.

[nhiroki, 2013-09-13 07:44:16]

+abarth@, could you review this? Thanks.

[abarth-chromium, 2013-09-13 17:33:01]

LGTM

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
File Source/modules/filesystem/DOMFileSystemBase.cpp (right):

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
Source/modules/filesystem/DOMFileSystemBase.cpp:123: result.append("/");
You should use use operator+ as follows:

String result = "filesystem:" + origin + "/" + typeString + "/";

That's maximally efficient because we'll allocate exactly the right size buffer
for the result.

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
Source/modules/filesystem/DOMFileSystemBase.cpp:173: return true;
There's no need to write "return false" or "return true".  You can just return
the boolean directly:

return (type != FileSystemTypeTemporary && type != FileSystemTypePersistent) ||
DOMFilePath::isValidPath(absolutePath);

(I might have screwed up the logic.)

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
Source/modules/filesystem/LocalFileSystem.h:50: void
resolveURL(ScriptExecutionContext*, KURL, PassOwnPtr<AsyncFileSystemCallbacks>);
KURL -> const KURL& ?

[nhiroki, 2013-09-16 16:36:07]

Thank you for reviewing!

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
File Source/modules/filesystem/DOMFileSystemBase.cpp (right):

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
Source/modules/filesystem/DOMFileSystemBase.cpp:123: result.append("/");
On 2013/09/13 17:33:01, abarth wrote:
> You should use use operator+ as follows:
> 
> String result = "filesystem:" + origin + "/" + typeString + "/";
> 
> That's maximally efficient because we'll allocate exactly the right size
buffer
> for the result.

Done.

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
Source/modules/filesystem/DOMFileSystemBase.cpp:173: return true;
On 2013/09/13 17:33:01, abarth wrote:
> There's no need to write "return false" or "return true".  You can just return
> the boolean directly:
> 
> return (type != FileSystemTypeTemporary && type != FileSystemTypePersistent)
||
> DOMFilePath::isValidPath(absolutePath);
> 
> (I might have screwed up the logic.)

Done.

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
File Source/modules/filesystem/LocalFileSystem.h (right):

https://codereview.chromium.org/23537011/diff/38001/Source/modules/filesystem...
Source/modules/filesystem/LocalFileSystem.h:50: void
resolveURL(ScriptExecutionContext*, KURL, PassOwnPtr<AsyncFileSystemCallbacks>);
On 2013/09/13 17:33:01, abarth wrote:
> KURL -> const KURL& ?

Done.

[commit-bot: I haz the power, 2013-09-18 01:21:16]

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

[commit-bot: I haz the power, 2013-09-21 05:29:07]

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

[commit-bot: I haz the power, 2013-09-21 05:29:44]

Change committed as 158157