[webkit-reviews] review denied: [Bug 37218] Implement FileStreamProxy that calls FileStream methods on FileThread for FileAPI : [Attachment 52883] 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 52883: Patch
https://bugs.webkit.org/attachment.cgi?id=52883&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
> diff --git a/WebCore/html/FileStreamProxy.cpp
b/WebCore/html/FileStreamProxy.cpp
> new file mode 100644
> index 0000000..59abd68
We should not have the above info in the patch. Otherwise, you will make svn
think it
is branched from other existing file, like previously landed patch:
  http://trac.webkit.org/changeset/57229

> +void FileStreamProxy::abort()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +}
> +
> +void FileStreamProxy::stop()
> +{
> +    fileThread()->unscheduleTasks(m_stream.get());
> +    fileThread()->postTask(createFileThreadTask(m_stream.get(),
&FileStream::stop));
> +}
It seems to be quite confusing with both abort() and stop() and they seem to do
the similar things.
For stop(), you call unscheduleTasks to remove all file thread tasks and then
call postTask
to add a new one. This seems to be a bit unnatural. Can you illustrate how the
whole destruction
logic work?

> +static void notifyGetSizeOnContext(ScriptExecutionContext* context,
FileStreamProxy* proxy, long long size)
> +{
> +    ASSERT(context->isContextThread());
Probably this ASSERT is really not needed. I think it would be better to have
ASSERTs on did*** methods to
ensure they're called from file thread; and ASSERTS on open/read/write/...
methods to ensure they're called
from non-file thread.

> +void FileStreamProxy::didStop()
> +{
> +    m_context->postTask(createCallbackTask(&notifyStopOnContext, this));
> +}
> +
> +void FileStreamProxy::didStopAndDestroy()
didStop() is called on file thread and didStopAndDestroy is called on context
thread and both of them
are prefixed with "did", which increases the difficulty to understand.