[webkit-reviews] review denied: [Bug 56704] [fileapi/chromium] Fetch platform path using GetMetadata before creating File from FileEntry* : [Attachment 86274] Patch to make supplying a platform path optional; once chromium's caught up, it'll be mandatory.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric U. <ericu at chromium.org>'s
request for review:
Bug 56704: [fileapi/chromium] Fetch platform path using GetMetadata before
creating File from FileEntry*
https://bugs.webkit.org/show_bug.cgi?id=56704

Attachment 86274: Patch to make supplying a platform path optional; once
chromium's caught up, it'll be mandatory.
https://bugs.webkit.org/attachment.cgi?id=86274&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86274&action=review

Nothing huge but a ton of little things so it would be nice to see it one more
time.

> WebCore/fileapi/DOMFileSystemSync.cpp:83
> +	     : m_failed(false)

4 space indent.

> WebCore/fileapi/DOMFileSystemSync.cpp:89
> +    static PassOwnPtr<GetPathHelper> create(GetPathResult* result) {

{ placement. lots of places.


Should be PassRefPtr<GetPathResult>.

> WebCore/fileapi/DOMFileSystemSync.cpp:95
> +	 ASSERT_NOT_REACHED();

indent. Lots of places.

> WebCore/fileapi/DOMFileSystemSync.cpp:107
> +    virtual void didReadDirectoryEntries(bool hasMore)

You'll need to comment the variable name (because it is unused and that is
going to give you a compile error on some platforms).  Several places.

> WebCore/fileapi/DOMFileSystemSync.cpp:130
> +    GetPathHelper(GetPathResult* result)

Should be PassRefPtr<GetPathResult>.

> WebCore/fileapi/DOMFileSystemSync.cpp:143
> +    RefPtr<GetPathHelper::GetPathResult> result(adoptRef(new
GetPathHelper::GetPathResult()));

adoptRef outside of constructor is a really bad sign. The constructor should be
private and there should be a static create method which returns a PassRefPtr.

> WebCore/fileapi/DOMFileSystemSync.cpp:144
> +    m_asyncFileSystem->readMetadata(platformPath,
GetPathHelper::create(result.get()));

I wonder why GetPathHelper::GetPathResult even exists.	Why can't the data just
be stored in GetPathHelper and get rid of GetPathHelper::GetPathResult?

> WebCore/fileapi/DOMFileSystem.cpp:119
> +    static PassOwnPtr<GetPathCallback> create(DOMFileSystem* filesystem,
const String& path, PassRefPtr<FileCallback> successCallback,
PassRefPtr<ErrorCallback> errorCallback) {

{ placement...

> WebCore/fileapi/DOMFileSystem.cpp:132
> +	 : FileSystemCallbacksBase(errorCallback)

spacing.