[Webkit-unassigned] [Bug 37218] Implement FileStreamProxy that calls FileStream methods on FileThread for FileAPI

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37218


Jian Li <jianli at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52883|review?                     |review-
               Flag|                            |




--- Comment #9 from Jian Li <jianli at chromium.org>  2010-04-08 15:46:06 PST ---
(From update of attachment 52883)
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.