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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 83740: Patch
https://bugs.webkit.org/attachment.cgi?id=83740&action=review

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

Some minor nits.  The one thing that seems slightly incorrect is passing a
random string to KURL as if it were a canonicalized URLString.	We're working
to improve the type system to make that impossible.  If you were writing this
patch in a few months, the type system would prevent you from writing that line
of code.  Please consider changing it before landing.

Thanks!

> LayoutTests/ChangeLog:34
> +	   (expectedAndRunNext):
> +	   (errorCallback):
> +	   (createTestFile):
> +	   ():
> +	   (assertIsDirectory):
> +	   (assertIsFile):
> +	   (runBasicTest):
> +	   (runHandmadeURI):
> +	   (runBogusURI):
> +	   (runWrongProtocol):
> +	   (runNotEnoughSlashes):
> +	   (runNotEnoughSlashes2):
> +	   (runUseBackSlashes.):
> +	   (runUseBackSlashes):
> +	   (runDirectory.):
> +	   (runDirectory):
> +	   (runWithTrailingSlash.):
> +	   (runWithTrailingSlash):
> +	   (runNextTest):
> +	   (cleanupAndRunNext):
> +	   (fileSystemCallback):
> +	   (var):

I would just remove this boilerplate from the changelog.  It's not really
helping anyone.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:68
> +    path = path.substring(1);
> +
> +    if (path.startsWith(kTemporaryPathPrefix)) {

Can you give an offset to startsWith to avoid the extra malloc from creating
the substring?

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:195
> +// ResolveURICallbacks
--------------------------------------------------------

We don't usually include these sorts of comments.

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:223
> +bool ErrorCallbackWrapper::handleEvent(FileError*)

The error doesn't matter?

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:229
> +} // namespace (anonymous)

Usually we would omit the (anonymous) part of this comment.

> Source/WebCore/fileapi/FileSystemCallbacks.cpp:251
>  // MetadataCallbacks
----------------------------------------------------------

I see that this file has a bunch of these comment already.

> Source/WebCore/page/DOMWindow.cpp:765
> +    KURL parsedURL(ParsedURLString, uri);

This still seems strange to me.  Everywhere else we call competeURL.  If this
is what the spec says, it's probably wrong.  :(