Bug 84135 – Support cross-filesystem operations in FileSystem API

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Support cross-filesystem operations in FileSystem API.  FileSystem API spec itself does not prohibit cross-filesystem operations, like copy or move across different filesystems, but our implementation does not seem to support it.

[Kinuko Yasuda]

Created attachment 137661 [details]
Patch

[Kinuko Yasuda]

Sorry this patch is kinda huge, but most of the changes are just moving some code piece from one file to another.  The gist of this patch is:
1) stop passing file paths (without origin/filesystem-type info) to platform layer (i.e. AsyncFileSystem layer) but instead pass complete filesystem URL,
2) keep filesystem type and rootURL information in DOMFileSystem rather than in platform layer, and
3) make relevant code relocation for 2).
This change is very important for supporting cross-filesystem operation.  Eric, would you be able to take the first look at the patch?  Thanks.

[Michael Nordman]

Comment on attachment 137661 [details]
Patch

Yup, this is a huge'ish patch alright!

> Source/WebCore/Modules/filesystem/LocalFileSystem.h:59
> +    void requestFileSystem(ScriptExecutionContext*, FileSystemType, long long size, PassOwnPtr<AsyncFileSystemCallbacks>, FileSystemSynchronousMode = SynchronousFileSystem);

Is the change from 'asycn' to 'sync' behavior in the default parameter value intentional?

[Kinuko Yasuda]

Created attachment 138046 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 137661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137661&action=review

>> Source/WebCore/Modules/filesystem/LocalFileSystem.h:59
>> +    void requestFileSystem(ScriptExecutionContext*, FileSystemType, long long size, PassOwnPtr<AsyncFileSystemCallbacks>, FileSystemSynchronousMode = SynchronousFileSystem);
> 
> Is the change from 'asycn' to 'sync' behavior in the default parameter value intentional?

Oops good catch... fixed.

[Eric U.]

I'll take a look on Monday; sorry for the delay, but the webkit meeting threw off my week.

[Eric U.]

Comment on attachment 138046 [details]
Patch

Looks generally good.  Small points below.

View in context: https://bugs.webkit.org/attachment.cgi?id=138046&action=review

> Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:75
> +        // FIXME: Remove this clause once http://codereview.chromium.org/7811006

It landed a while back; mind trimming this dead code while you're in there?
If we don't have an innerURL, the URL isn't valid, and we should return false.

> Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:129
> +    return KURL(ParsedURLString, url);

Why are we not just returning url?  What does the extra constructor call do?

> Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:144
> +    // chromium and is validated each time in the chromium port.

This comment needs rewriting for clarity, and "to the chromium" doesn't make sense.  I realize it isn't a new comment, but deleting it would be better than leaving it as-is.

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:52
>  namespace {

Make sure you copy all relevant changes into AsyncFileSystemBlackBerry.cpp and AsyncFileSystemGtk.cpp.

[Kinuko Yasuda]

Created attachment 138521 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 138046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138046&action=review

>> Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:75
>> +        // FIXME: Remove this clause once http://codereview.chromium.org/7811006
> 
> It landed a while back; mind trimming this dead code while you're in there?
> If we don't have an innerURL, the URL isn't valid, and we should return false.

Done.

>> Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:129
>> +    return KURL(ParsedURLString, url);
> 
> Why are we not just returning url?  What does the extra constructor call do?

Nope, this should just return url. Fixed.

>> Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:144
>> +    // chromium and is validated each time in the chromium port.
> 
> This comment needs rewriting for clarity, and "to the chromium" doesn't make sense.  I realize it isn't a new comment, but deleting it would be better than leaving it as-is.

I updated the comment.  What it wants to say is that the root URL we create here is going to be validated in the browser process each time a request (which contains the root URL) is processed.

> Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp:53
>  

Thanks for pointing it out... I haven't realized they have the files.  Done.

[WebKit Review Bot]

Comment on attachment 138521 [details]
Patch

Attachment 138521 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12527339

New failing tests:
svg/carto.net/colourpicker.svg

[WebKit Review Bot]

Created attachment 138632 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kinuko Yasuda]

Created attachment 138977 [details]
Patch

rebased.

[Eric U.]

Comment on attachment 138977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138977&action=review

I'm assuming the GTK changes compile; I'm sure those folks will mention it if they don't.  LGTM, and thanks for the cleanup.

> Source/WebKit/chromium/ChangeLog:5
> +        Need a short description and bug URL (OOPS!)

Needs comments here.

[Kinuko Yasuda]

Thanks!

(In reply to comment #13)
> (From update of attachment 138977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138977&action=review
> 
> I'm assuming the GTK changes compile; I'm sure those folks will mention it if they don't.  LGTM, and thanks for the cleanup.
>
> > Source/WebKit/chromium/ChangeLog:5
> > +        Need a short description and bug URL (OOPS!)
> 
> Needs comments here.

I'll fix this in my next patch or before I submit.

David, now it seems ready for your review-- can you take a look at the patch as a reviewer?  Thanks!

[Kinuko Yasuda]

Comment on attachment 138977 [details]
Patch

I'm separating this into multiple subpatches as this is a bit too big.

[Kinuko Yasuda]

Factored out three cleanup changes from this one:
https://bugs.webkit.org/show_bug.cgi?id=85736
https://bugs.webkit.org/show_bug.cgi?id=85738
https://bugs.webkit.org/show_bug.cgi?id=85741

[Kinuko Yasuda]

Created attachment 140522 [details]
Patch

[Kinuko Yasuda]

Created a new patch which does not include slightly irrelevant cleanups.  This is still big but hopefully easier to follow.

This patch is also adding the new header file 'FileSystemType.h' to build files (e.g. WebCore.gypi) which was missing in the previous patch.  The reason why I merged the change into this one is this patch also moves the file from platform/ to Modules/filesystem/, so build file change is inevitable if I separately made the change or not.  Hope this makes sense.

[David Levin]

Comment on attachment 140522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140522&action=review

> Source/WebCore/ChangeLog:24
> +        - adding FileSystemType.h entry to build files (e.g. WebCore.gypi, WebCore.xcodeproj etc)

Ok. I got through it. 

Perhaps there could have been a few other smaller pieces to break out but I believe I reviewed this well as is.

Thanks for pulling out the pieces that you did because that made it easier!

> Source/WebKit/chromium/ChangeLog:68
> [phone]  Kinuko Yasuda  <[e-mail]>

repeated entry.

> Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:116
> +    // filesystemName.toString();

Please remove this commented out line.

[Kinuko Yasuda]

Committed r116388: <http://trac.webkit.org/changeset/116388>

[Chris H-C]

(In reply to comment #20)
> Committed r116388: <http://trac.webkit.org/changeset/116388>

Uh... not sure how to say this, but your patch doesn't compile in the BlackBerry port, and I'm not sure how it could compile if FILE_SYSTEM is def'd.

Among the problems is FileSystemSynchronousMode, which doesn't exist outside of LocalFileSystem.cpp, and is a mismatch with the signature in LocalFileSystem.h.

[Kinuko Yasuda]

(In reply to comment #21)
> (In reply to comment #20)
> > Committed r116388: <http://trac.webkit.org/changeset/116388>
> 
> Uh... not sure how to say this, but your patch doesn't compile in the BlackBerry port, and I'm not sure how it could compile if FILE_SYSTEM is def'd.
> 
> Among the problems is FileSystemSynchronousMode, which doesn't exist outside of LocalFileSystem.cpp, and is a mismatch with the signature in LocalFileSystem.h.

Oops.  Sorry I think I was careless not to break build with FILE_SYSTEM defines on other ports.  As for the FileSystemSynchronousMode problem it should have been FileSystemSynchronousType (s/Mode/Type/).  If you could wait a few more days I'll try fixing those issues (do you mind filing a bug in the case?).

[Chris H-C]

(In reply to comment #22)
> Oops.  Sorry I think I was careless not to break build with FILE_SYSTEM defines on other ports.  As for the FileSystemSynchronousMode problem it should have been FileSystemSynchronousType (s/Mode/Type/).  If you could wait a few more days I'll try fixing those issues (do you mind filing a bug in the case?).

We've made a local change to that effect, thanks for the confirmation.

I've filed Bug#86103 and CC-ed you on it.

[Eric U.]

*** Bug 61404 has been marked as a duplicate of this bug. ***