Bug 36896

Created attachment 52208 [details] Patch

Created attachment 52209 [details] Fixed ChangeLog tabs

Attachment 52208 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/mac/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] WebKitTools/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebKitTools/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Total errors found: 7 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 52209 [details] Fixed ChangeLog tabs For adding a new ENABLE_ flags, please follow the instructions found in the beginning of JavaScriptCore/Configurations/FeatureDefines.xcconfig, as for which files have to be in sync. It doesn't look like the patch affects all of them. Also, the patch as is (without FileThread.h and FileThread.cpp) would not compile. It is hard to say if the code in ScriptExecutionContext that deals with FileThread object makes sense w/o looking at FileThread code. I suggest adding FileThread code to the patch at least. It can be not completely functional yet. It is ok to add more code later.

Comment on attachment 52209 [details] Fixed ChangeLog tabs r- due to notes above.

Created attachment 52218 [details] Patch

Thanks, tried to address the issues. (I can separate the patch into two, add feature flags and add FileThread if it's more preferable.)

Comment on attachment 52218 [details] Patch 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+.

Created attachment 52251 [details] Patch

Created attachment 52252 [details] Patch

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

Comment on attachment 52252 [details] Patch 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.

Created attachment 52333 [details] Patch

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.

Comment on attachment 52333 [details] Patch Thanks! r=me

Comment on attachment 52333 [details] Patch Clearing flags on attachment: 52333 Committed r56968: <http://trac.webkit.org/changeset/56968>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/56968 might have broken Chromium Win Release

No, this was just the Chromium Builder being unreliable. :(

Created attachment 52560 [details] Patch

Comment on attachment 52560 [details] Patch Webkit-patch post just sent my patch here instead of 36671. I have no idea why.

Created attachment 52799 [details] Patch

Created attachment 52801 [details] Patch

(In reply to comment #23) > Created an attachment (id=52801) [details] > Patch Same thing. webkit-patch seems to like this bug.