[Webkit-unassigned] [Bug 36896] Add FileThread for async file operation support in FileReader and FileWriter

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Kinuko Yasuda <kinuko at chromium.org>  2010-03-31 21:23:37 PST ---
(In reply to comment #8)
> > diff --git a/JavaScriptCore/Configurations/FeatureDefines.xcconfig b/JavaScriptCore/Configurations/FeatureDefines.xcconfig
> It seems there should be JavaScriptCore/ChangeLog in the patch, mentioning this
> file change.

Done.

> > --- a/WebCore/ChangeLog
> > +        No new tests, will add ones when after adding modules which use the
> > +        thread.
> This line may go unwrapped, there is no 80-char width limit in WebKit.

Done.

> > diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
> > +# Add FileThread sources if FileWriter or FileReader is enabled.
> > +if ENABLE_FILE_THREAD
> ENABLE_FILE_THREAD?

Fixed.  (I tried to do a bit fancy in configure.ac and makefile.am. now went
back to basic.)

> > diff --git a/WebCore/WebCore.gypi b/WebCore/WebCore.gypi
> For Chromium, WebKit/chromium/features.gypi should be updated to include new
> ENABLE_* flags.

Done.  (Wow.  We may want a script for adding a feature?)

> > diff --git a/WebCore/dom/ScriptExecutionContext.cpp b/WebCore/dom/ScriptExecutionContext.cpp
> > +#if ENABLE(FILE_READER) || ENABLE(FILE_WRITER)
> > +FileThread* ScriptExecutionContext::fileThread()
> > +{
> > +    if (!m_fileThread && !m_fileThreadCreated) {
> 
> It would be nice to have a comment here explaining why you need to check both
> m_fileThread and m_fileThreadCreated.

Dropped the flag.  It was a remnant of other change that was reverted.

> > +    return m_fileThread ? m_fileThread.get() : 0;
> why not just "return m_fileThread.get()"?

Oops.  Fixed.

> > diff --git a/WebCore/html/FileThread.cpp b/WebCore/html/FileThread.cpp
> > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +#include "config.h"
> Need an empty line right after Copyright header.

Done.

> > +void* FileThread::runLoop()
> > +{
> > +    {
> > +        // Wait for FileThread::start() to complete.
> > +        MutexLocker lock(m_threadCreationMutex);
> 
> It is obvious that this waits for FileThread::start() to terminate. It'd be
> more useful to tell why it is needed (to have m_threadID established in run
> loop).

Added a bit more useful comment.

> > diff --git a/WebCore/html/FileThread.h b/WebCore/html/FileThread.h
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +#ifndef FileThread_h
> Need empty line after Copyright.

Done.

> > +    void terminate();
> 'terminate' usually means 'death right now'. It is unlikely that thread is
> terminated in this function. How about 'stop' or 'exitRunLoop'?

Sounds good... Renamed it 'stop'.

> > +    class Task : public Noncopyable {
> > +    public:
> > +        virtual ~Task() { }
> > +        virtual void performTask() = 0;
> > +        virtual PlatformFileHandle fileHandle() const = 0;
> If most tasks will have a handle, why not add a non-virtual support for that,
> including constructor that would take PlatformFileHandle. If there are tasks
> not related to a file handle, it could have invaldPlatformFileHandle as default
> parameter. It'd avoid multiple copy of the same code across multiple tasks.

Makes sense.  Changed the code as your suggestion.

> BTW, if you will need to use commit-bot (since you are not yet a committer),
> please flip cq=? flag to indicate that, so a reviewer could flip it to +
> together with r+.

I see.  I will do that for future uploads.

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