[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


Dmitry Titov <dimich at chromium.org> changed:

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




--- Comment #8 from Dmitry Titov <dimich at chromium.org>  2010-03-31 17:09:12 PST ---
(From update of attachment 52218)
Thanks for iterating quickly!
Looks very good, just a few notes, mostly formatting/naming:

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

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

> diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
> +# Add FileThread sources if FileWriter or FileReader is enabled.
> +# ---
> +if ENABLE_FILE_THREAD
> +webcore_sources += \
> +	WebCore/html/FileThread.cpp \
> +	WebCore/html/FileThread.h
> +endif  # END ENABLE_FILE_THREAD

ENABLE_FILE_THREAD?

> diff --git a/WebCore/WebCore.gypi b/WebCore/WebCore.gypi

For Chromium, WebKit/chromium/features.gypi should be updated to include new
ENABLE_* flags.

> 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) {
> +        m_fileThread = FileThread::create();
> +        m_fileThreadCreated = true;
> +        if (!m_fileThread->start())
> +            m_fileThread = 0;

It would be nice to have a comment here explaining why you need to check both
m_fileThread and m_fileThreadCreated.

> +    }
> +    return m_fileThread ? m_fileThread.get() : 0;

why not just "return m_fileThread.get()"?

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

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

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

> +    void terminate();

'terminate' usually means 'death right now'. It is unlikely that thread is
terminated in this function. How about 'stop' or 'exitRunLoop'?

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

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

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