[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 #52252|review?                     |review-
               Flag|                            |




--- Comment #12 from Dmitry Titov <dimich at chromium.org>  2010-04-01 12:32:56 PST ---
(From update of attachment 52252)
Almost there. Adding those ENABLE_ flags can be tedious indeed, but we are
getting close.

> diff --git a/ChangeLog b/ChangeLog
> +        Add a enable flag for FileReader and FileWriter.
> +        https://bugs.webkit.org/show_bug.cgi?id=36896
> +
> +        * configure.ac:
> +

> diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
> +        Add ENABLE_FILE_READER and ENABLE_FILE_WRITER flags.
> +
> +        * Configurations/FeatureDefines.xcconfig:

The ChangeLog should include a link to the bug. Also, it would be nice if the
title (above the link to the bug) was the same in all ChangeLogs in the patch
and also matched the title of the bug itself (this is not strictly speaking
required, but sometimes is useful).

> diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
> +# ---
> +# FileReader support
> +# ---
> +if ENABLE_FILE_READER
> +FEATURE_DEFINES += ENABLE_FILE_READER=1
> +webcore_cppflags += -DENABLE_FILE_READER=1
> +
> +# Add FileThread sources if FileWriter or FileReader is enabled.
> +webcore_sources += \
> +	WebCore/html/FileThread.cpp \
> +	WebCore/html/FileThread.h
> +endif  # END ENABLE_FILE_READER
> +
> +# ---
> +# FileWriter support
> +# ---
> +if ENABLE_FILE_WRITER
> +FEATURE_DEFINES += ENABLE_FILE_WRITER=1
> +webcore_cppflags += -DENABLE_FILE_WRITER=1
> +
> +# Add FileThread sources if FileWriter or FileReader is enabled.
> +webcore_sources += \
> +	WebCore/html/FileThread.cpp \
> +	WebCore/html/FileThread.h
> +endif  # END ENABLE_FILE_WRITER

I missed it on first review: would it add FileThread.cpp to the list of
compiled files twice if both switches are defined? Since content of those files
is protected by ifdefs, perhaps it would be simpler to just add those files to
the list of WebCore sources, let them compile in any case, their content is
going to be disabled anyways if none of the flags is defined. Just like you
have the code in ScriptExecutionContext that is there but not compiled if
features are not enabled. Same about other build files.

> diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
> +contains(DEFINES, ENABLE_FILE_READER) {
> +    HEADERS += \
> +      html/FileThread.h
> +    SOURCES += \
> +      html/FileThread.cpp
> +}
> +
> +contains(DEFINES, ENABLE_FILE_WRITER) {
> +    HEADERS += \
> +      html/FileThread.h
> +    SOURCES += \
> +      html/FileThread.cpp
> +}

Same as above (just add those files unconditionally).

> diff --git a/WebCore/WebCore.vcproj/WebCore.vcproj b/WebCore/WebCore.vcproj/WebCore.vcproj
>  			</File>
>  			<File
> +				RelativePath="..\html\FileThread.cpp"
> +				>
> +			</File>
> +			<File
> +				RelativePath="..\html\FileThread.h"
> +				>
> +			</File>
> +			<File
> +				RelativePath="..\html\FileThread.cpp"
> +				>
> +			</File>
> +			<File
> +				RelativePath="..\html\FileThread.h"
> +				>
> +			</File>

Seems like duplication here.

> diff --git a/WebCore/dom/ScriptExecutionContext.cpp b/WebCore/dom/ScriptExecutionContext.cpp

>  ScriptExecutionContext::Task::~Task()
>  {
>  }

This destructor is already defined in .h file...

> diff --git a/WebCore/html/FileThread.h b/WebCore/html/FileThread.h

> +    class Task : public Noncopyable {
> +    public:
> +        Task(PlatformFileHandle handle) : m_handle(handle) { }
> +        virtual ~Task() { }
> +        virtual void performTask() = 0;
> +        virtual PlatformFileHandle fileHandle() const { return m_handle; }

Not sure it has to be virtual now.

> +    protected:
> +        Task() { }

This constructor does not seem to be needed... Can be added later if there will
be need.

> diff --git a/WebKit/chromium/features.gypi b/WebKit/chromium/features.gypi

There should be a ChangeLog file mentioning this change.

> diff --git a/WebKit/mac/Configurations/FeatureDefines.xcconfig b/WebKit/mac/Configurations/FeatureDefines.xcconfig
> +ENABLE_FILE_READER = ENABLE_FILE_READER;
> +ENABLE_FILE_WRITER = ENABLE_FILE_WRITER;

In other xcconfig files, these are set like:
> +ENABLE_FILE_READER = ;
> +ENABLE_FILE_WRITER = ;

Why the difference?

> diff --git a/WebKitLibraries/ChangeLog b/WebKitLibraries/ChangeLog
> +        Adds ENABLE_FILE_READER and ENABLE_FILE_WRITER feature flags
> +        for FileReader and FileWriter support.
> +
> +        * win/tools/vsprops/FeatureDefines.vsprops:
> +        * win/tools/vsprops/FeatureDefinesCairo.vsprops:

Same as above - needs a link to the bug. Check all ChangeLogs for the
uniformness. Also, WebKitTools/Scripts/prepare-ChangeLog helps to generate
them, especially with "--bug" option. I sometimes use it again to 'regenerate'
the ChangeLogs after more changes are added to the patch.

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