[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 #83740|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #14 from Adam Barth <abarth at webkit.org>  2011-02-24 18:44:44 PST ---
(From update of attachment 83740)
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.  :(

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