[webkit-reviews] review denied: [Bug 46405] Add idl and mock classes for FileSystemSync for FileSystem API : [Attachment 69567] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 46405: Add idl and mock classes for FileSystemSync for FileSystem API
https://bugs.webkit.org/show_bug.cgi?id=46405

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

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

> WebCore/bindings/js/JSDirectoryEntrySyncCustom.cpp:89
> +	   flags = toFlags(exec->argument(1));

Up to here it is the same as the previous function.  Please consider a common
function to do this stuff.

This appears to be missing:
  if (exec->hadException())
	return jsUndefined();

here.

> WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:56
> +    STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, path, args[0]);

indent.

> WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:109
> +    }

Duplicate code. consider a common function.

> WebCore/fileapi/DOMFileSystemSync.cpp:43
> +    return adoptRef(new DOMFileSystemSync(fileSystem->m_name,
fileSystem->m_asyncFileSystem.leakPtr()));

This shouldn't be a leakPtr(). This should be a releasePtr.

leakPtr is only use when you want the OwnPtr to stop holding onto to the point
and you want the raw pointer. You will manage the memory in some other way. 
Here you are passing the point to a PassOwnPtr, so releasePtr is perfect.

> WebCore/fileapi/DOMFileSystemSync.cpp:60
> +} // namespace

These // namespace comments aren't necessary in WebKit as far as I know.
However, if you are going to include it, make it correct :).

> WebCore/fileapi/DOMFileSystemSync.h:51
> +    // This call is destructive; the argument fileSystem will become
unusable.

This sounds scary. It makes me wonder why it doesn't take a PassOwnPtr for
fileSystem. If fileSystem is unusable afterward. is there a way to help callers
to do the right thing and not use it again?

> WebCore/fileapi/DOMFileSystemSync.h:62
> +} // namespace

Ditto.

> WebCore/fileapi/DirectoryEntrySync.h:61
> +    DirectoryEntrySync(DOMFileSystemBase* fileSystem, const String&
fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/DirectoryReader.h:60
> +    DirectoryReader(DOMFileSystemBase* fileSystem, const String& fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/DirectoryReaderBase.h:46
> +    void setHasMore(bool hasMore) { m_hasMore = hasMore; }

What does "hasMore" mean? has more what? (My initial reaction is cowbell since
that is the current meme.)

> WebCore/fileapi/DirectoryReaderSync.h:54
> +    DirectoryReaderSync(DOMFileSystemBase* fileSystem, const String&
fullPath);

fileSystem param name.

> WebCore/fileapi/EntryArraySync.h:53
> +    void set(unsigned index, PassRefPtr<EntrySync> entry);

Please remove variable names which add no information. entry

> WebCore/fileapi/EntrySync.h:59
> +    EntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/FileEntrySync.h:54
> +    FileEntrySync(DOMFileSystemBase* fileSystem, const String& fullPath);

Please remove variable names which add no information. fileSystem

> WebCore/fileapi/FileEntrySync.idl:40
> +	   // File file() raises (FileException);

Please remove commented out code.