[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 #14 from Kinuko Yasuda <kinuko at chromium.org>  2010-04-01 14:19:28 PST ---
Thanks for your careful review!

I ran prepare-ChangeLog to regenerate and fix the ChangeLogs.

(In reply to comment #12)
> > diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
> > diff --git a/WebCore/WebCore.pro b/WebCore/WebCore.pro
> 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.

Hmm, true.  Changed to add the files to webcore sources by default.

> > diff --git a/WebCore/WebCore.vcproj/WebCore.vcproj
> Seems like duplication here.

Removed the dup.

> > diff --git a/WebCore/dom/ScriptExecutionContext.cpp b/WebCore/dom/ScriptExecutionContext.cpp
> 
> >  ScriptExecutionContext::Task::~Task()
> >  {
> >  }
> 
> This destructor is already defined in .h file...

No the one in .h does not have a body.  (This isn't a part I've touched either)

> > diff --git a/WebCore/html/FileThread.h b/WebCore/html/FileThread.h
> 
> > +    class Task : public Noncopyable {
> > +    public:
> > +        virtual PlatformFileHandle fileHandle() const { return m_handle; }
> Not sure it has to be virtual now.

Removed virtual keyword.

> > +    protected:
> > +        Task() { }
> 
> This constructor does not seem to be needed... Can be added later if there will
> be need.

Right, hadn't fixed the constructors...  Removed it.

> > diff --git a/WebKit/chromium/features.gypi b/WebKit/chromium/features.gypi
> There should be a ChangeLog file mentioning this change.

Added.

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

Great catch, fixed it.

> > diff --git a/WebKitLibraries/ChangeLog b/WebKitLibraries/ChangeLog
> 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.

Yeah... it worked prettily.

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