[webkit-reviews] review denied: [Bug 54774] [fileapi] Implement LocalFileSystem.resolveFileSystemURI : [Attachment 83025] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Adam Klein <adamk at chromium.org>'s
request for review:
Bug 54774: [fileapi] Implement LocalFileSystem.resolveFileSystemURI
https://bugs.webkit.org/show_bug.cgi?id=54774

Attachment 83025: Patch
https://bugs.webkit.org/attachment.cgi?id=83025&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83025&action=review

This could use a bunch more tests for the URL cracking code.  You're not
testing any of the error cases, as far as I can tell.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:68
> +	   path = path.substring(sizeof(kTemporaryPathString) - 1);

pls use a constant for this value.

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

> Source/WebCore/fileapi/DOMFileSystemBase.h:64
> +    static const char kPersistentPathString[];
> +    static const char kTemporaryPathString[];

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.

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