Created attachment 137661 [details] Patch

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.

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?

Created attachment 138046 [details] Patch

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.

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

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.

Created attachment 138521 [details] Patch

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.

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

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

Created attachment 138977 [details] Patch rebased.

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.

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!

Comment on attachment 138977 [details] Patch I'm separating this into multiple subpatches as this is a bit too big.

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

Created attachment 140522 [details] Patch

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.

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 > +2012-04-24 Kinuko Yasuda <kinuko@chromium.org> repeated entry. > Source/WebCore/Modules/filesystem/chromium/DOMFileSystemChromium.cpp:116 > + // filesystemName.toString(); Please remove this commented out line.

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

(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.

(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?).

(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.

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