[webkit-reviews] review denied: [Bug 47044] Add FileSystemSync implementation for Worker : [Attachment 69761] 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 47044: Add FileSystemSync implementation for Worker
https://bugs.webkit.org/show_bug.cgi?id=47044

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

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

Just a few things to address.

> WebCore/fileapi/DirectoryEntry.cpp:54
> +    filesystem()->scheduleCallback(errorCallback, FileError::create(error));


Please get rid of the macro.

> WebCore/fileapi/Entry.cpp:51
> +    filesystem()->scheduleCallback(errorCallback, FileError::create(error));


Please get rid of the macro.

>> WebCore/fileapi/EntrySync.h:50
>> +	static PassRefPtr<EntrySync> create(EntryBase* entry);
> 
> param name.

(remove the param name)

> WebCore/fileapi/FileEntry.h:-42
> -class DOMFileSystem;

Why didn't this become DOMFileSystemBase?

> WebCore/platform/AsyncFileSystem.h:63
> +    // This should return false if there're no pending operations.

s/there're/there are/

> WebCore/workers/WorkerContext.h:51
> +    class DOMFileSystemSync;

out of order.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp:77
> +	   return false;

It seems like m_bridgeForLastOperation should be cleared at the end of this.

If that is true, then consider
RefPtr<> bridge = m_bridge.release();

and use bridge. Now bridge will get deref'ed when this function is exited.

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:62
> +    virtual bool waitCompletion();

s/waitCompletion/waitForOperationToComplete/

> WebKit/chromium/src/WorkerAsyncFileSystemChromium.h:82
> +    RefPtr<WebKit::WorkerFileSystemCallbacksBridge>
m_bridgeForLastOperation;

m_bridgeForCurrentOperation?