[Webkit-unassigned] [Bug 54774] [fileapi] Implement LocalFileSystem.resolveFileSystemURI

Tue Feb 22 09:55:20 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=54774

--- Comment #7 from Adam Klein <adamk at chromium.org>  2011-02-22 09:55:20 PST ---
(From update of attachment 83025)
View in context: https://bugs.webkit.org/attachment.cgi?id=83025&action=review

>> LayoutTests/fast/filesystem/resources/resolve-uri.js:14
>> +    testFailed("Error occured:" + error.code);
> 
> Typo: occurred

Fixed.

>> LayoutTests/fast/filesystem/resources/resolve-uri.js:38
>> +var jsTestIsAsync = true;
> 
> I think it's worth testing more than one URL.
> For example, try one with a directory, try both temporary and persistent, try the root directory, try removing the entry before resolving the URL.
> Also, generate some by hand and test a few kinds of malformed URLs, and try directories with and without the trailing slash.

Sorry the initial upload didn't include more tests; will add lots more. I mainly need to come up with a bit more framework to make it easy to add new testcases.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:53
>> +const char DOMFileSystemBase::kTemporaryPathString[] = "temporary";
> 
> Now that we've got these constants defined, we should really use them in toURI as well.

Only not using them there because that usage is moving, in another patch, from one file to the other.  Depending on which lands first, I'll consolidate, and there won't be any more references to the bare string.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:62
>> +    if (path.isEmpty() || !path[0] == '/')
> 
> Do you mean:
> 
>     path[0] != '/'
> 
> ?

Wow, I did, that's really bad. I think I'm too used to unittests catching my mistakes.  Fixed.

>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:68
>> +        path = path.substring(sizeof(kTemporaryPathString) - 1);
> 
> pls use a constant for this value.

Will do.

>>> Source/WebCore/fileapi/DOMFileSystemBase.cpp:75
>>> +    if (path.isEmpty() || !path[0] == '/')
>> 
>> Same here.
> 
> Please test the cases where the URL contains \ instead of / to make sure we're canonicalizing things properly.

Fixed the logical error. And will add this to the list of tests I need to create. The '\' '/' thing worked correctly with GURL, but you're right that I haven't checked into KURL's behavior.

>>> Source/WebCore/fileapi/DOMFileSystemBase.h:64

>> 
>> Another way to do this is to have static functions that return const String& and use DEFINE_STATIC_LOCAL to define the strings.  That way, you get the length for free and it's easier to use various String methods.
> 
> If you do this please make sure it's called only on the document thread (maybe with an assert).  (As most of the DOMFileSystemBase's methods are called on both main thread and worker thread.)

This will also be used, eventually, from worker threads, so if DEFINE_STATIC_LOCAL isn't threadsafe it seems I shouldn't use it at all.

>> Source/WebCore/page/DOMWindow.cpp:765
>> +    KURL parsedURL(ParsedURLString, uri);
> 
> This is unlikely to be correct.  ParsedURLString means that |uri| has already been canonicalized by KURL, but |uri| here can be an arbitrary string.  What is the expected behavior when |uri| is a string like "foo" ?

I guess I was reading KURL's interface poorly: I was interpreting "ParsedURLString" as "ParseURLString", an instruction of what to do to the argument rather than a description of that argument. I'd expected that I'd end up with an invalid URL if the string was "foo".  If it was, e.g., "http://www.google.com/index.html", DOMFileSystemBase::crackFileSystemURL() will complain about the wrong protocol.

My next patch upload will add lots more tests with random strings.  But do you have a better suggestion for how to tell whether this looks like a valid URL?  Should I see if it begins with "filesystem:" before doing anything else?

>> Source/WebCore/page/DOMWindow.cpp:766
>> +    if (!AsyncFileSystem::isAvailable() || !securityOrigin->canAccessFileSystem() || !securityOrigin->canRequest(parsedURL)) {
> 
> I'm not sure this is strict enough.  Should the fact that the security origin can load from this URL be enough to grant it [writeable] access to the Entry?
> Currently we're talking about not letting any cross-origin requests be made at all, but if we should relax that, we wouldn't necessarily want to allow this as well.

For now this check is exactly as strict as you suggest: canRequest requires same-origin, so this effectively makes the file only available on the same origin.  I used a call to securityOrigin rather than re-implementing that logic here.  But do you think I should instead duplicate the logic, in case we decide to loosen the canRequest restrictions?  It seems unlikely to me that we'd really loosen canRequest (which is for XHRs), though we might loosen canDisplay (for, e.g., <img> tags).

>> Source/WebCore/page/DOMWindow.cpp:778
>> +    LocalFileSystem::localFileSystem().readFileSystem(document, type, 0, ResolveURICallbacks::create(successCallback, errorCallback, document, filePath));
> 
> (Not a comment for this change) actually probably we should drop the 'size' parameter from readFileSystem as it's supposed to return a read-only file system.

Are there any other callers of readFileSystem? I didn't see any.  I could just do that in this change...

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.