[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





--- Comment #10 from Kinuko Yasuda <kinuko at chromium.org>  2010-04-08 16:44:56 PST ---
(In reply to comment #9)
> (From update of attachment 52883 [details])
> > 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

Hmm they're done by webkit-patch.  I'll look into how I can avoid that.

> > +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?

Abort() is straightforward, it needs to stop any (pending) writes/reads, thus
it unschedules tasks.  it's just an API and by definition it doesn't have
anything to do with destructing FileStreamProxy/FileStream.

Stop() is for enforcing a destruction order and closing a file handle on the
file thread.  It shouldn't be called for other purpose.
It might be written better, but the logic I thought is:

0. FileStreamProxy holds refs for ScriptExecutionContext and FileStream, so
they must be available throughout the proxy's life cycle.
1. when we need to stop/destroy FileStreamProxy, we call
FileStreamProxy::stop() and then deref the proxy.
    (e.g. m_streamProxy->stop(); m_streamProxy = 0;)
     FileStreamProxy holds its own ref so it won't be deleted until it really
thinks it's done.
2. FileStreamProxy::stop() calls unscheduleTasks to remove any pending
writes/reads (i.e. tasks still in the queue) for the stream.
3. FileStreamProxy::stop() posts a task to close the file and waits for the
close (and any ongoing tasks) to be completed.
4. When FileStreamProxy::didStop() is called we can safely say that the file is
closed and there're no ongoing/pending tasks that need call back to the proxy.
5. FileStreamProxy::didStopAndDestroy() derefs itself and the context.

> > +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.

Sounds good.  notifyXXX are all static methods and they are less likely to be
called unexpectedly.
> > +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.

I've tried to name it better but haven't come up with a good idea.

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