Bug 55644 – [fileapi] Add URI resolution support to WorkerContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Klein]

[fileapi] Add URI resolution support to WorkerContext

[Adam Klein]

Created attachment 84508 [details]
Patch

[Adam Klein]

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

[Kinuko Yasuda]

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.

[Adam Klein]

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.

[Adam Klein]

Created attachment 84604 [details]
Patch

[Kinuko Yasuda]

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

[Adam Klein]

CCing some possible reviewers.

[David Levin]

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.

[Adam Klein]

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

[Adam Klein]

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.

[Adam Klein]

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

[David Levin]

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);

[Adam Klein]

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

[Adam Klein]

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.

[Adam Klein]

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

[Adam Klein]

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

[Adam Barth]

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

[David Levin]

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

[Adam Klein]

Created attachment 86384 [details]
Patch

[Adam Klein]

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.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 86384 [details]
Patch

Clearing flags on attachment: 86384

Committed r81640: <http://trac.webkit.org/changeset/81640>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.