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

------- Additional Comments from Jian Li <jianli at chromium.org>
Only some minor things to resolve.

> diff --git a/WebCore/html/FileStreamProxy.cpp
b/WebCore/html/FileStreamProxy.cpp
> +static void derefProxyOnContext(ScriptExecutionContext*, FileStreamProxy*
proxy)
> +{
> +    proxy->deref();
Can we add an ASSERT before this line, like:
       ASSERT(proxy->hasOneRef());
This way, we can ensure that the caller, i.e., FileReader/FileWriter, has
already
released the reference and now it should only has ref count 1 now.

> diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> +// A proxy module that calls corresponding FileStream methods on the file
thread.  Note: you must call stop() before destroying a FileStreamProxy
instance, e.g. in the desctructor of an object that holds a reference to the
proxy.
Probably we could say the following for Note part:
   // ... Note: you must call stop() first and then release the reference to
the FileStreamProxy instance.

> +    // Stop() stops any pending tasks, close the file and derefs itself; the
caller should not recycle the instance after calling stop().
It is not needed to repeat stop() in the comment. Also we should use "closes"
to be consistent with "stops".
Probably it is better to say "destruct".
      // Stops the proxy and schedules it to be destructed. All the pending
tasks will be aborted and the file stream will be closed.
      // Note: the caller should not recycle the instance after calling stop().