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

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83025|review?                     |review-
               Flag|                            |




--- Comment #5 from Adam Barth <abarth at webkit.org>  2011-02-21 12:44:36 PST ---
(From update of attachment 83025)
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" ?

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