Bug 55644

Created attachment 84508 [details] Patch

Depends on http://codereview.chromium.org/6613008/.

Comment on attachment 84508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84508&action=review lgtm. > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:60 > +void LocalFileSystem::readFileSystem(ScriptExecutionContext* context, AsyncFileSystem::Type type, PassOwnPtr<AsyncFileSystemCallbacks> callbacks, bool synchronous) The implementation of this method is mostly same as that of requestFileSystem-- maybe we could factor out a common private method that takes all of parameters plus boolean create flag and make both readFileSystem and requestFileSystem a wrapper of the method.

Comment on attachment 84508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84508&action=review >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:60 >> +void LocalFileSystem::readFileSystem(ScriptExecutionContext* context, AsyncFileSystem::Type type, PassOwnPtr<AsyncFileSystemCallbacks> callbacks, bool synchronous) > > The implementation of this method is mostly same as that of requestFileSystem-- maybe we could factor out a common private method that takes all of parameters plus boolean create flag and make both readFileSystem and requestFileSystem a wrapper of the method. Good idea. I've done that. The only thing that I don't like about all this is the fact that there's two bools and a long long all together in one signature. Quite easy to screw up the order and break stuff. I'd like to come back to this code and redo the API with enums instead.

Created attachment 84604 [details] Patch

(In reply to comment #5) > Created an attachment (id=84604) [details] > Patch LGTM. (In reply to comment #4) > > The implementation of this method is mostly same as that of requestFileSystem-- maybe we could factor out a common private method that takes all of parameters plus boolean create flag and make both readFileSystem and requestFileSystem a wrapper of the method. > > Good idea. I've done that. The only thing that I don't like about all this is the fact that there's two bools and a long long all together in one signature. Quite easy to screw up the order and break stuff. I'd like to come back to this code and redo the API with enums instead. Agreed, the signature looks a bit messed up after repeated ad-hoc changes. Anyway the two public methods look nicer now, thanks for doing this.

CCing some possible reviewers.

Comment on attachment 84604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84604&action=review > Source/WebCore/ChangeLog:9 > + in the WebKit Chromium builds. Why are are adding more stuff here if none of it is testable? (I thought I saw some tests for the file system in workers but maybe not.) If they aren't testable, can we fix that before adding more here. > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:77 > + openFileSystemHelper(context, type, callbacks, synchronous, 0, false); That last parameter should be an enum instead of a bool to make the call sites more readable.

Created attachment 84624 [details] Use an enum for internal create param

Comment on attachment 84604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84604&action=review >> Source/WebCore/ChangeLog:9 >> + in the WebKit Chromium builds. > > Why are are adding more stuff here if none of it is testable? (I thought I saw some tests for the file system in workers but maybe not.) > > If they aren't testable, can we fix that before adding more here. Thanks for calling me on this. Kinuko, can you help here? Last time I tried to run the worker tests they just hung. >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:77 >> + openFileSystemHelper(context, type, callbacks, synchronous, 0, false); > > That last parameter should be an enum instead of a bool to make the call sites more readable. Done.

Removing review bit while I go get the filesystem sync tests running in Chromium.

I saw this but then forgot to mention this before. Avoid abbreviations in WebKit. I'm referring to "fs" below: RefPtr<DOMFileSystemSync> fs = readFileSystemHelper.getResult(ec);

Created attachment 86215 [details] Now with tests, but not ready for review

The one thing holding this back from review is the backslash test: I can't figure out why it's passing in async mode but failing in sync. Will probably require a debugging session.

Created attachment 86248 [details] Ready for review, tests pass

Just a friendly ping that this is ready for review again, now with the tests David asked for.

I'm going to let dave_levin review this patch. I looked over it briefly and it looked sane to me. :)

Comment on attachment 86248 [details] Ready for review, tests pass View in context: https://bugs.webkit.org/attachment.cgi?id=86248&action=review Just a few minor things to address. > Source/WebCore/fileapi/LocalFileSystem.h:57 > + void readFileSystem(ScriptExecutionContext*, AsyncFileSystem::Type, PassOwnPtr<AsyncFileSystemCallbacks>, bool synchronous = false); Why add this parameter and then ignore it? > Source/WebCore/workers/WorkerContext.h:136 > + void resolveLocalFileSystemURL(const String& url, PassRefPtr<EntryCallback>, PassRefPtr<ErrorCallback>); The parameter name successCallback would add information in this case so it would be good to add it. > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:67 > +void openFileSystemHelper(ScriptExecutionContext* context, AsyncFileSystem::Type type, PassOwnPtr<AsyncFileSystemCallbacks> callbacks, bool synchronous, long long size, CreationFlag create) Function local to file should be "static".

Created attachment 86384 [details] Patch

Comment on attachment 86248 [details] Ready for review, tests pass View in context: https://bugs.webkit.org/attachment.cgi?id=86248&action=review >> Source/WebCore/fileapi/LocalFileSystem.h:57 >> + void readFileSystem(ScriptExecutionContext*, AsyncFileSystem::Type, PassOwnPtr<AsyncFileSystemCallbacks>, bool synchronous = false); > > Why add this parameter and then ignore it? The code in fileapi/LocalFileSystem.cpp is a stubbed-out implementation that's not currently used since the Chromium port is the only one to implement FileSystem. My code's ignoring of the new parameter is just following in the footsteps of requestFileSystem, in this same file. Note that I do use the bool in LocalFileSystemChromium.cpp. Kinuko may be able to explain why this stubbed impl exists at all; it seems that things would work just fine without it. >> Source/WebCore/workers/WorkerContext.h:136 >> + void resolveLocalFileSystemURL(const String& url, PassRefPtr<EntryCallback>, PassRefPtr<ErrorCallback>); > > The parameter name successCallback would add information in this case so it would be good to add it. Added the name here, and above for requestFileSystem for consistency. >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:67 >> +void openFileSystemHelper(ScriptExecutionContext* context, AsyncFileSystem::Type type, PassOwnPtr<AsyncFileSystemCallbacks> callbacks, bool synchronous, long long size, CreationFlag create) > > Function local to file should be "static". Function was already inside an anonymous namespace, but I've moved the namespace up to cover only the enum, and made the function static. Let me know if you want the function inside the namespace, but that seems redundant to me.

The commit-queue encountered the following flaky tests while processing attachment 86384 [details]: fast/dom/onerror-img.html bug 51019 The commit-queue is continuing to process your patch.

Comment on attachment 86384 [details] Patch Clearing flags on attachment: 86384 Committed r81640: <http://trac.webkit.org/changeset/81640>

All reviewed patches have been landed. Closing bug.