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

------- Additional Comments from Jian Li <jianli at chromium.org>
> --- /dev/null
> +++ b/WebCore/html/FileStreamProxy.cpp
> +#include "FileThread.h"
> +#include "FileThreadTask.h"
This include should be placed above so that it will be in the alphabetical
order.

> FileStreamProxy::~FileStreamProxy()
> {
>     if (fileThread())
>	  fileThread()->unscheduleTasks(m_stream.get());
Is it possible that m_context is destructed before FileStreamProxy instance?

> +void FileStreamProxy::read(char* buffer, int length)
> +{
> +    fileThread()->postTask(createFileThreadTask(m_stream.get(),
&FileStream::read, buffer, length));
> +}
> +
> +void FileStreamProxy::write(Blob* blob, long long position, size_t length)
We should use either size_t or int for length in read() and write(), for
consistency.

> +FileThread* FileStreamProxy::fileThread()
> +{
> +    ASSERT(m_context->fileThread()); // FIXME
What is this FIXME about? 
> +    return m_context->fileThread();
> +}

> +void FileStreamProxy::abort()
> +{
> +    if (fileThread())
Do we really need to check for fileThread()?
> +	   fileThread()->unscheduleTasks(m_stream.get());
> +}

> +static void notifyWriteOnContext(ScriptExecutionContext* context,
FileStreamProxy* stream, int nwritten)
Better to name it as bytesWritten.