Description
Zoltan Horvath
2010-11-22 01:47:09 PST
Created attachment 74534 [details]
proposed patch
All in one patch. A bit large.
Comment on attachment 74534 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=74534&action=review > JavaScriptCore/runtime/DateInstanceCache.h:40 > + I'll remove this. > WebKitTools/DumpRenderTree/ForwardingHeaders/wtf/Noncopyable.h:-1 > -#include <JavaScriptCore/Noncopyable.h> prepare-ChangeLog didn't generate changelog for WebKitTools. I'll make up. I won't update the patch until Darin's comment. (In reply to comment #3) > I won't update the patch until Darin's comment. Only 3 files should be updated to make this patch appliable: JavaScriptCore/bytecode/CodeBlock.h WebCore/bindings/js/JSSVGContextCache.h WebCore/svg/DeprecatedSVGAnimatedPropertyTraits.h We would like to know what EWS bots say. ;) We would not want to know yet. (In reply to comment #3) > I won't update the patch until Darin's comment. If it builds, I think it’s fine to do this all at once. Created attachment 74640 [details]
proposed patch
Attachment 74640 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6404012 Attachment 74640 [details] did not build on qt: Build output: http://queues.webkit.org/results/6373010 Attachment 74640 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6420010 Created attachment 74652 [details]
playing with EWSs
Attachment 74652 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6389012 Attachment 74652 [details] did not build on qt: Build output: http://queues.webkit.org/results/6408014 Attachment 74652 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6391012 Created attachment 74660 [details]
playing with EWSs round #2
I can't do anything on win applying.
Attachment 74660 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6348018 Created attachment 74664 [details]
playing with Chromium's EWS bots round #3
Created attachment 74665 [details]
playing with Chromium's EWS bots round #3
FontCustomPlatformDataCairo.h fixed.
To be continued tomorrow. Attachment 74660 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6404015 Attachment 74665 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6337018 Created attachment 74729 [details]
playing with Chromium's EWS bot round #4
Created attachment 74876 [details]
proposed patch (review candidate)
I tested it with Linux-Chrome, Linux-Gtk and others.
It would be awesome if WTF_MAKE_FAST_ALLOCATED required a semicolon at the end of it for two reasons: 1. It just follows the typical (mental) pattern in C++ for statements (otherwise it looks like a modifier for the next item. 2. It will help with things that do regex for simple parsing. For example, editors (like emacs) that help with indenting or check-webkit-style. A simple way to do this would be to add the following as the last line of WTF_MAKE_FAST_ALLOCATED : typedef int ThisIsHereToForceASemicolonAfterThisMacro Attachment 74876 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6342043 Created attachment 74926 [details]
updated rc
(In reply to comment #24) > It would be awesome if WTF_MAKE_FAST_ALLOCATED required a semicolon at the end of it for two reasons: > 1. It just follows the typical (mental) pattern in C++ for statements (otherwise it looks like a modifier for the next item. > 2. It will help with things that do regex for simple parsing. For example, editors (like emacs) that help with indenting or check-webkit-style. > > A simple way to do this would be to add the following as the last line of WTF_MAKE_FAST_ALLOCATED : > typedef int ThisIsHereToForceASemicolonAfterThisMacro Good idea, I agree with you. I modified the patch. Created attachment 75113 [details]
updated proposed patch rc
Updated to latest.
Comment on attachment 75113 [details] updated proposed patch rc View in context: https://bugs.webkit.org/attachment.cgi?id=75113&action=review It isn't clear why the noncopyable attribute wasn't retained in various places. It also isn't clear why noncopyable was converted to noncopyable+fastallocated in some places but only noncopyable in others. Lastly, I have some concerns about the macro placement and about some side effects of what is hidden in those macros (private:/public: , etc). > JavaScriptCore/bytecode/CodeBlock.h:582 > + public: Remove "public:" here. (Not equivalent to what was here before.) > JavaScriptCore/bytecode/CodeBlock.h:606 > + private: Remove "private:" here. > JavaScriptCore/parser/Nodes.h:116 > + WTF_MAKE_FAST_ALLOCATED; Remove "WTF_MAKE_FAST_ALLOCATED" (because it appears to me that this is not equivalent to what was here before.) > JavaScriptCore/parser/Nodes.h:1476 > + WTF_MAKE_FAST_ALLOCATED; Remove "WTF_MAKE_FAST_ALLOCATED" > JavaScriptCore/pcre/pcre_exec.cpp:51 > +#include <string.h> Why is this getting added? > JavaScriptCore/runtime/MarkStack.h:31 > +#include <string.h> Why did this get added? > JavaScriptCore/runtime/RegExpObject.h:63 > + struct RegExpObjectData { What happened to the fast allocated quality here? > JavaScriptCore/runtime/RegExpObject.h:64 > + public: Remove "public:" > JavaScriptCore/runtime/SmallStrings.h:41 > + WTF_MAKE_NONCOPYABLE(SmallStrings); WTF_MAKE_FAST_ALLOCATED; Remove "WTF_MAKE_FAST_ALLOCATED" > JavaScriptCore/runtime/SymbolTable.h:125 > + WTF_MAKE_FAST_ALLOCATED; Remove "WTF_MAKE_FAST_ALLOCATED" > JavaScriptCore/wtf/MD5.h:35 > +#include <stdint.h> Why is this being added? > JavaScriptCore/wtf/OwnArrayPtr.h:37 > +template <typename T> class OwnArrayPtr { Needs WTF_MAKE_NONCOPYABLE > JavaScriptCore/wtf/OwnPtr.h:41 > + template<typename T> class OwnPtr { Needs WTF_MAKE_NONCOPYABLE > JavaScriptCore/wtf/Platform.h:1122 > +/* Noncopyable's implementation as a macro */ Use C++ style comments. platform.h seems like the wrong header file for this. Why not put them in the header files where they were? > JavaScriptCore/wtf/Platform.h:1133 > + private: \ Hiding private: inside of this macro is unexpected and will lead to people scratching their heads when their class members become private after this is put in. (Much like hiding goto or return in a macro). Instead how about just putting this macro at the end of classes and explicitly adding a private: section if there isn't one. > JavaScriptCore/wtf/Platform.h:1147 > +public: \ Same comment as above but instead of putting this macro at the end of the class, one could put it at the end of the public section. I tend to expect important information at the top of the class and whether the class is fast allocated or not really doesn't matter to me when I trying to use it. > JavaScriptCore/wtf/Platform.h:1176 > +private: \ Ditto. > WebKitTools/WebKitAPITest/HostWindow.h:32 > +class HostWindow { What happened to Noncopyable? > WebKitTools/WebKitTestRunner/TestInvocation.h:33 > +class TestInvocation { What happened to Noncopyable? Created attachment 75696 [details] updated patch (In reply to comment #29) > (From update of attachment 75113 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75113&action=review > > It isn't clear why the noncopyable attribute wasn't retained in various places. > > It also isn't clear why noncopyable was converted to noncopyable+fastallocated in some places but only noncopyable in others. When we implemented custom allocation framework into WebKit we made a decision to add FastAllocBase as a base class of Noncopyable because it is a superclass and a lot of heap allocated classes are inherited from it, so some classes needlessly got FastAllocBase's methods. In this patch I corrected these things, because we can choose exactly which class has to be either fast allocated or noncopyable or both. I use our static code analyzer tool (called Columbus, check http://www.inf.u-szeged.hu/sed/softwarequality) to determine these things. > Lastly, I have some concerns about the macro placement and about some side effects of what is hidden in those macros (private:/public: , etc). > > > JavaScriptCore/bytecode/CodeBlock.h:582 > > + public: > > Remove "public:" here. (Not equivalent to what was here before.) > > > JavaScriptCore/bytecode/CodeBlock.h:606 > > + private: > > Remove "private:" here. Adding of WTF_MAKE_FAST_ALLOCATED to RareData struct causes compile errors on windows: 2>c:\cygwin\home\webkit\webkit\javascriptcore\wtf\OwnPtrCommon.h(59) : error C2248: 'JSC::CodeBlock::RareData' : cannot access private struct declared in class 'JSC::CodeBlock' 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(582) : see declaration of 'JSC::CodeBlock::RareData' 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(245) : see declaration of 'JSC::CodeBlock' 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\wtf/PassOwnPtr.h(58) : see reference to function template instantiation 'void WTF::deleteOwnedPtr<JSC::CodeBlock::RareData>(T *)' being compiled 2> with 2> [ 2> T=JSC::CodeBlock::RareData 2> ] 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\wtf/PassOwnPtr.h(58) : while compiling class template member function 'WTF::PassOwnPtr<T>::~PassOwnPtr(void)' 2> with 2> [ 2> T=JSC::CodeBlock::RareData 2> ] 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(535) : see reference to class template instantiation 'WTF::PassOwnPtr<T>' being compiled 2> with 2> [ 2> T=JSC::CodeBlock::RareData 2> ] I don't know what causes this. Modifying its access level has solved the problem. > > JavaScriptCore/parser/Nodes.h:116 > > + WTF_MAKE_FAST_ALLOCATED; > > Remove "WTF_MAKE_FAST_ALLOCATED" (because it appears to me that this is not equivalent to what was here before.) Removed. > > JavaScriptCore/parser/Nodes.h:1476 > > + WTF_MAKE_FAST_ALLOCATED; > > Remove "WTF_MAKE_FAST_ALLOCATED" FunctionParameters is a Vector and is a RefCounted. Both Vector and RefCounted is fast allocated, so we need to add the macro to make the methods unambigous for the compilers. > > JavaScriptCore/pcre/pcre_exec.cpp:51 > > +#include <string.h> > > Why is this getting added? It is needed for memcpy on Chromium build. > > JavaScriptCore/runtime/MarkStack.h:31 > > +#include <string.h> > > Why did this get added? When I removed include of Noncopyable.h memcpy missed string.h. > > JavaScriptCore/runtime/RegExpObject.h:63 > > + struct RegExpObjectData { > > What happened to the fast allocated quality here? > Same problem on Windows like at RareData struct. In the new patch I did the same as with RareData. > > > JavaScriptCore/runtime/RegExpObject.h:64 > > + public: > > Remove "public:" Ups. It's a mistake, I removed it. > > JavaScriptCore/runtime/SmallStrings.h:41 > > + WTF_MAKE_NONCOPYABLE(SmallStrings); WTF_MAKE_FAST_ALLOCATED; > > Remove "WTF_MAKE_FAST_ALLOCATED" Yeap, removed. SmallStringsStorage should be fast allocated not SmallStrings, changed. > > JavaScriptCore/runtime/SymbolTable.h:125 > > + WTF_MAKE_FAST_ALLOCATED; > > Remove "WTF_MAKE_FAST_ALLOCATED" SharedSymbolTable is a SymbolTable which is HashMap (HashMap is fast allocated), and it is RefCounted as well. The reason is same as for FunctionParameters class. > > JavaScriptCore/wtf/MD5.h:35 > > +#include <stdint.h> > > Why is this being added? It is needed on windows because of uint8_t and etc. MD5.cpp also needs string.h because of memcpy. > > JavaScriptCore/wtf/OwnArrayPtr.h:37 > > +template <typename T> class OwnArrayPtr { > > Needs WTF_MAKE_NONCOPYABLE It doesn't need because it has it's own copy constructor: JavaScriptCore/wtf/OwnArrayPtr.h:50 > > JavaScriptCore/wtf/OwnPtr.h:41 > > + template<typename T> class OwnPtr { > > Needs WTF_MAKE_NONCOPYABLE Same case as in OwnArrayPtr. CC: JavaScriptCore/wtf/OwnPtr.h:55 > > JavaScriptCore/wtf/Platform.h:1122 > > +/* Noncopyable's implementation as a macro */ > > Use C++ style comments. Gtk build won't compile with C++ style comments. Platform.h contains only C style comments. > platform.h seems like the wrong header file for this. Why not put them in the header files where they were? I think it is simplier to put these to platform.h, in this way we can avoid including Noncopyable.h and FastAllocBase.h headers. > > JavaScriptCore/wtf/Platform.h:1133 > > + private: \ > > Hiding private: inside of this macro is unexpected and will lead to people scratching their heads when their class members become private after this is put in. (Much like hiding goto or return in a macro). This is the original Noncopyable macro implementation made by Anders Carlsson in bug #46589. > > Instead how about just putting this macro at the end of classes and explicitly adding a private: section if there isn't one. > > > JavaScriptCore/wtf/Platform.h:1147 > > +public: \ > > Same comment as above but instead of putting this macro at the end of the class, one could put it at the end of the public section. Then I think it's better to putting to the end of the classes. Btw, I had a related patch in bug #47887, but I closed it because I didn't get response and I wanted to go ahead with this big patch. > I tend to expect important information at the top of the class and whether the class is fast allocated or not really doesn't matter to me when I trying to use it. That is true. I think we should wait for others opinion as well! I haven't changed these in my new patch yet. > > JavaScriptCore/wtf/Platform.h:1176 > > +private: \ > > Ditto. Bug #47887 was related to this. > > WebKitTools/WebKitAPITest/HostWindow.h:32 > > +class HostWindow { > > What happened to Noncopyable? I did't want to include Platform.h. Corrected. > > WebKitTools/WebKitTestRunner/TestInvocation.h:33 > > +class TestInvocation { > > What happened to Noncopyable? Ditto. I think these are the cases which indicate separate header files for noncopyable and fast allocated macros. Attachment 75696 [details] did not build on qt: Build output: http://queues.webkit.org/results/6746081 Created attachment 75700 [details]
updated proposed patch
When I look at http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/Platform.h, it has no macros of the type that you are adding. It is solely focused on defining what platforms use, enable, and support. The added macros have nothing to do with that. (In reply to comment #30) > Created an attachment (id=75696) [details] > updated patch > > (In reply to comment #29) > > (From update of attachment 75113 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=75113&action=review > > > Adding of WTF_MAKE_FAST_ALLOCATED to RareData struct causes compile errors on windows: > > 2>c:\cygwin\home\webkit\webkit\javascriptcore\wtf\OwnPtrCommon.h(59) : error C2248: 'JSC::CodeBlock::RareData' : cannot access private struct declared in class 'JSC::CodeBlock' > 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(582) : see declaration of 'JSC::CodeBlock::RareData' > 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(245) : see declaration of 'JSC::CodeBlock' > 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\wtf/PassOwnPtr.h(58) : see reference to function template instantiation 'void WTF::deleteOwnedPtr<JSC::CodeBlock::RareData>(T *)' being compiled > 2> with > 2> [ > 2> T=JSC::CodeBlock::RareData > 2> ] > 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\wtf/PassOwnPtr.h(58) : while compiling class template member function 'WTF::PassOwnPtr<T>::~PassOwnPtr(void)' > 2> with > 2> [ > 2> T=JSC::CodeBlock::RareData > 2> ] > 2> c:\cygwin\home\webkit\WebKit\JavaScriptCore\bytecode\CodeBlock.h(535) : see reference to class template instantiation 'WTF::PassOwnPtr<T>' being compiled > 2> with > 2> [ > 2> T=JSC::CodeBlock::RareData > 2> ] > > I don't know what causes this. Modifying its access level has solved the problem. It would be good to try to understand why you are doing what you are doing rather than just do something to shut up a compiler. > > > > JavaScriptCore/wtf/Platform.h:1133 > > > + private: \ > > > > Hiding private: inside of this macro is unexpected and will lead to people scratching their heads when their class members become private after this is put in. (Much like hiding goto or return in a macro). > > This is the original Noncopyable macro implementation made by Anders Carlsson in bug #46589. I guess I'll have to take this one up with him. > > > > Instead how about just putting this macro at the end of classes and explicitly adding a private: section if there isn't one. > > > > > JavaScriptCore/wtf/Platform.h:1147 > > > +public: \ > > > > Same comment as above but instead of putting this macro at the end of the class, one could put it at the end of the public section. > > Then I think it's better to putting to the end of the classes. Btw, I had a related patch in bug #47887, but I closed it because I didn't get response and I wanted to go ahead with this big patch. Well, I'd support doing it in this patch (or I'm willing to re-visit that other patch). Hiding a change in access level makes reading code harder which is bad. (In reply to comment #30) > Created an attachment (id=75696) [details] > updated patch > > (In reply to comment #29) > > (From update of attachment 75113 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=75113&action=review > > > > It isn't clear why the noncopyable attribute wasn't retained in various places. > > > > It also isn't clear why noncopyable was converted to noncopyable+fastallocated in some places but only noncopyable in others. > > When we implemented custom allocation framework into WebKit we made a decision to add FastAllocBase as a base class of Noncopyable because it is a superclass and a lot of heap allocated classes are inherited from it, so some classes needlessly got FastAllocBase's methods. In this patch I corrected these things, because we can choose exactly which class has to be either fast allocated or noncopyable or both. I use our static code analyzer tool (called Columbus, check http://www.inf.u-szeged.hu/sed/softwarequality) to determine these things. Your ChangeLog has not indication of what is happening here. Also this is not reflected in the bug title. Ideally you'd keep equivalence with respect to this aspect for this patch (since the patch is not about fixing FastAllocBase inheritance -- it is about getting rid of those classes -- and there are enough changes in here to do that without introducing a separate change for something else at the same time). Immediately after this change, get rid of the places where it isn't needed with an appropriate bug, etc. Created attachment 75808 [details]
updated, ews test
(In reply to comment #33) > When I look at http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/Platform.h, it has no macros of the type that you are adding. It is solely focused on defining what platforms use, enable, and support. The added macros have nothing to do with that. Yeap, in the past we talked about to put these into platform.h. Currently, I don't know the bug's number. > It would be good to try to understand why you are doing what you are doing rather than just do something to shut up a compiler. Done. Patch fixed. More about the build problem: http://msdn.microsoft.com/en-us/library/tsbce2bh(VS.80).aspx > > > > This is the original Noncopyable macro implementation made by Anders Carlsson in bug #46589. > > I guess I'll have to take this one up with him. > > > > > > > > Instead how about just putting this macro at the end of classes and explicitly adding a private: section if there isn't one. > > > > > > > JavaScriptCore/wtf/Platform.h:1147 > > > > +public: \ > > > > > > Same comment as above but instead of putting this macro at the end of the class, one could put it at the end of the public section. > > > > Then I think it's better to putting to the end of the classes. Btw, I had a related patch in bug #47887, but I closed it because I didn't get response and I wanted to go ahead with this big patch. > > Well, I'd support doing it in this patch (or I'm willing to re-visit that other patch). Hiding a change in access level makes reading code harder which is bad. I have started to put the macro's to the end of the classes. It looks terrible. Do we really want to do that? (In reply to comment #34) > > When we implemented custom allocation framework into WebKit we made a decision to add FastAllocBase as a base class of Noncopyable because it is a superclass and a lot of heap allocated classes are inherited from it, so some classes needlessly got FastAllocBase's methods. In this patch I corrected these things, because we can choose exactly which class has to be either fast allocated or noncopyable or both. I use our static code analyzer tool (called Columbus, check http://www.inf.u-szeged.hu/sed/softwarequality) to determine these things. > > Your ChangeLog has not indication of what is happening here. Also this is not reflected in the bug title. Yes. What do you think about change the bug and the changelog title to Refactoring custom allocation framework and its inheritances? > Ideally you'd keep equivalence with respect to this aspect for this patch (since the patch is not about fixing FastAllocBase inheritance -- it is about getting rid of those classes -- and there are enough changes in here to do that without introducing a separate change for something else at the same time). Immediately after this change, get rid of the places where it isn't needed with an appropriate bug, etc. I'd like to do this one patch if it's possible. Changing from inheriting to use macros indicates extra changes than just a simple batch replace either way. Of course, I need to extend my changelog! Btw, custom allocation framework has effect only on platforms which defines ENABLE_GLOBAL_FASTMALLOC_NEW to 0. Attachment 75808 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808 [details] did not build on qt: Build output: http://queues.webkit.org/results/6774094 Attachment 75808 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75808 [details] did not build on win: Build output: http://queues.webkit.org/results/6853064 Attachment 75808 [details] did not build on win: Build output: http://queues.webkit.org/results/6735090 Attachment 75808 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6790080 Attachment 75808 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061
Died at WebKitTools/Scripts/update-webkit line 132.
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 75808 [details]
updated, ews test
Sorry about the style-bot noise. The stylebot has been fixed. Since this doens't pass any EWS (Except gtk) seems this is unlandable as-is.
Created attachment 77887 [details]
updated, ews test, for prereview
In this patch I don't remove Noncopyable.h, but marcos are still the same with 'private:'.
Attachment 77887 [details] did not build on win: Build output: http://queues.webkit.org/results/7236421 Created attachment 77993 [details]
proposed patch (review candidate)
Windows build and Chromium win build fixed. Patch is updated.
Hey, is there any thoughts on this patch? Z Created attachment 79429 [details]
updated to latest, playing with ews-s
Attachment 79429 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 193 in 443 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79429 [details] did not build on mac: Build output: http://queues.webkit.org/results/7551213 Created attachment 79564 [details]
proposed patch (review candidate)
Attachment 79564 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 445 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 79566 [details]
proposed patch (review candidate) + efl fix
Attachment 79564 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7613242 Attachment 79564 [details] did not build on qt: Build output: http://queues.webkit.org/results/7534250 Created attachment 79568 [details]
proposed patch (review candidate)
Attachment 79564 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7499244 Attachment 79568 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 445 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 79571 [details]
proposed patch (review candidate)
Attachment 79571 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 445 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79568 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7549242 Created attachment 79572 [details]
proposed patch (review candidate)
EFL fix.
Attachment 79571 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7591225 Attachment 79572 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 446 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 79571 [details] did not build on qt: Build output: http://queues.webkit.org/results/7613245 Attachment 79564 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7510220 Attachment 79572 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7493236 Attachment 79572 [details] did not build on qt: Build output: http://queues.webkit.org/results/7627207 Attachment 79572 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7584224 Created attachment 79581 [details]
proposed patch
Attachment 79581 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.h', u..." exit_code: 1
Last 3072 characters of output:
Core/loader/CrossOriginPreflightResultCache.h:68: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/css/MediaQueryEvaluator.h:53: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderBoxModelObject.cpp:67: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadingPrimitives.h:101: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/ValidityState.h:33: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGFontData.h:29: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/ThreadSafeShared.h:70: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/ColumnInfo.h:35: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/runtime/BatchedTransitionOptimizer.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Source/WebCore/dom/NodeRareData.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/dom/NodeRareData.h:73: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/page/UserStyleSheet.h:39: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLPreloadScanner.h:42: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/OriginQuotaManager.h:45: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/parser/Parser.h:49: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/canvas/CanvasRenderingContext.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/html/parser/HTMLElementStack.h:43: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/bridge/jsc/BridgeJSC.h:63: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/icon/IconDatabase.h:66: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathBlender.h:30: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/platform/ContextMenu.h:45: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/NavigationScheduler.cpp:57: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/ImageLoader.cpp:61: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/storage/DatabaseTracker.h:60: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/svg/SVGPathByteStream.h:47: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/rendering/RenderImageResource.h:38: More than one command on the same line [whitespace/newline] [4]
Source/WebCore/loader/cache/MemoryCache.h:74: More than one command on the same line [whitespace/newline] [4]
Total errors found: 192 in 446 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 79581 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=79581&action=review Zoltan fixed everything what David mentioned, except putting macros at the end of classes. I think these macros should be at the top to get quick if a class is fast allocated or not. Great work, r=me. Please be sure not to break any platform. > Source/JavaScriptCore/wtf/FastAllocBase.h:60 > // Example usage: > -// class Widget : public FastAllocBase { ... }; > +// class Widget { > +// WTF_MAKE_FAST_ALLOCATED > +// ... > +// }; > +// > +// struct Data { > +// WTF_MAKE_FAST_ALLOCATED > +// public: > +// ... > +// }; This documentation is should be enough for developers not to surprise at private struct members if they miss to use explicit public modifier. Additionally please write a mail to webkit-dev about this change. (and maybe a wiki page too) http://trac.webkit.org/changeset/76248 might have broken Windows Debug (Build) Patch landed in http://trac.webkit.org/changeset/76248 Qt-V8 buildfix landed in http://trac.webkit.org/changeset/76251 Mac and Win DRT buildfix landed in http://trac.webkit.org/changeset/76253 Comment on attachment 79581 [details]
proposed patch
remove r+ from landed patch
|