[webkit-reviews] review denied: [Bug 37218] Implement FileStreamProxy that calls FileStream methods on FileThread for FileAPI : [Attachment 52921] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Kinuko Yasuda <kinuko at chromium.org>'s
request for review:
Bug 37218: Implement FileStreamProxy that calls FileStream methods on
FileThread for FileAPI
https://bugs.webkit.org/show_bug.cgi?id=37218

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

------- Additional Comments from Jian Li <jianli at chromium.org>
I think the way to fix "new file mode..." problem in the patch is to create a
new file
from the scratch and then copy and paste the content over. You need to revert
all new
files just added and then create files and add them to the repository.

> diff --git a/WebCore/html/FileStreamProxy.cpp
b/WebCore/html/FileStreamProxy.cpp
> +FileStreamProxy::FileStreamProxy(ScriptExecutionContext* context,
FileStreamClient* client)
> +    : m_context(context)
> +    , m_client(client)
> +    , m_stream(FileStream::create(this))
> +{
> +    ref();
Could you please add the comment about why we need to keep its own ref here?

> +void FileStreamProxy::abort()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +}
> +
> +void FileStreamProxy::stop()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +    fileThread()->postTask(createFileThreadTask(m_stream.get(),
&FileStream::stop));
> +}
I still think we do not need both abort() and stop(). Since we are not going to
resume
after abort, can we simply call stop() from FileReader/FieWriter::abort()?

> diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> +class FileStreamProxy : public RefCounted<FileStreamProxy>, public
FileStreamClient {
> +protected:
If we're not going to have other child classes derived from FileStreamProxy,
it would be better to make it "private".