Bug 49897 - Refactoring of the custom allocation framework
Summary: Refactoring of the custom allocation framework
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on: 42998 46589
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-22 01:47 PST by Zoltan Horvath
Modified: 2011-01-20 09:33 PST (History)
11 users (show)

See Also:


Attachments
proposed patch (315.62 KB, patch)
2010-11-22 01:59 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (318.15 KB, patch)
2010-11-23 04:10 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
playing with EWSs (319.00 KB, patch)
2010-11-23 05:40 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
playing with EWSs round #2 (319.48 KB, patch)
2010-11-23 07:15 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
playing with Chromium's EWS bots round #3 (319.49 KB, patch)
2010-11-23 07:55 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
playing with Chromium's EWS bots round #3 (319.50 KB, patch)
2010-11-23 07:58 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
playing with Chromium's EWS bot round #4 (319.53 KB, patch)
2010-11-24 00:20 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (review candidate) (321.32 KB, patch)
2010-11-25 07:23 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated rc (321.80 KB, patch)
2010-11-26 05:53 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated proposed patch rc (319.97 KB, patch)
2010-11-30 01:29 PST, Zoltan Horvath
levin: review-
Details | Formatted Diff | Diff
updated patch (320.26 KB, patch)
2010-12-06 07:41 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated proposed patch (320.29 KB, patch)
2010-12-06 08:43 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated, ews test (319.84 KB, patch)
2010-12-07 06:43 PST, Zoltan Horvath
eric: review-
Details | Formatted Diff | Diff
updated, ews test, for prereview (275.29 KB, patch)
2011-01-04 07:51 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (review candidate) (276.88 KB, patch)
2011-01-05 06:09 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated to latest, playing with ews-s (288.28 KB, patch)
2011-01-19 08:08 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (review candidate) (289.67 KB, patch)
2011-01-20 01:55 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (review candidate) + efl fix (289.87 KB, patch)
2011-01-20 02:21 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (review candidate) (289.88 KB, patch)
2011-01-20 02:35 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (review candidate) (289.88 KB, patch)
2011-01-20 02:59 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (review candidate) (290.49 KB, patch)
2011-01-20 03:24 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (290.49 KB, patch)
2011-01-20 04:41 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2010-11-22 01:47:09 PST
Inheriting from FastAllocBase can result in objects getting larger (bug #33896, bug #46589). Change Noncopyable and FastAllocBase inheriting to their macro implementation.
Comment 1 Zoltan Horvath 2010-11-22 01:59:13 PST
Created attachment 74534 [details]
proposed patch

All in one patch. A bit large.
Comment 2 Zoltan Horvath 2010-11-22 02:06:20 PST
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.
Comment 3 Zoltan Horvath 2010-11-22 02:09:04 PST
I won't update the patch until Darin's comment.
Comment 4 Csaba Osztrogonác 2010-11-22 02:12:50 PST
(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. ;)
Comment 5 Zoltan Horvath 2010-11-22 02:14:03 PST
We would not want to know yet.
Comment 6 Darin Adler 2010-11-22 08:24:09 PST
(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.
Comment 7 Zoltan Horvath 2010-11-23 04:10:47 PST
Created attachment 74640 [details]
proposed patch
Comment 8 WebKit Review Bot 2010-11-23 04:40:41 PST
Attachment 74640 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6404012
Comment 9 Early Warning System Bot 2010-11-23 04:56:12 PST
Attachment 74640 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6373010
Comment 10 Eric Seidel (no email) 2010-11-23 05:02:15 PST
Attachment 74640 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6420010
Comment 11 Zoltan Horvath 2010-11-23 05:40:44 PST
Created attachment 74652 [details]
playing with EWSs
Comment 12 WebKit Review Bot 2010-11-23 06:22:32 PST
Attachment 74652 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6389012
Comment 13 Early Warning System Bot 2010-11-23 06:27:22 PST
Attachment 74652 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6408014
Comment 14 Eric Seidel (no email) 2010-11-23 07:01:02 PST
Attachment 74652 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6391012
Comment 15 Zoltan Horvath 2010-11-23 07:15:17 PST
Created attachment 74660 [details]
playing with EWSs round #2

I can't do anything on win applying.
Comment 16 WebKit Review Bot 2010-11-23 07:53:17 PST
Attachment 74660 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6348018
Comment 17 Zoltan Horvath 2010-11-23 07:55:37 PST
Created attachment 74664 [details]
playing with Chromium's EWS bots round #3
Comment 18 Zoltan Horvath 2010-11-23 07:58:18 PST
Created attachment 74665 [details]
playing with Chromium's EWS bots round #3

FontCustomPlatformDataCairo.h fixed.
Comment 19 Zoltan Horvath 2010-11-23 08:00:07 PST
To be continued tomorrow.
Comment 20 Eric Seidel (no email) 2010-11-23 08:03:26 PST
Attachment 74660 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6404015
Comment 21 WebKit Review Bot 2010-11-23 09:31:18 PST
Attachment 74665 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6337018
Comment 22 Zoltan Horvath 2010-11-24 00:20:49 PST
Created attachment 74729 [details]
playing with Chromium's EWS bot round #4
Comment 23 Zoltan Horvath 2010-11-25 07:23:17 PST
Created attachment 74876 [details]
proposed patch (review candidate)

I tested it with Linux-Chrome, Linux-Gtk and others.
Comment 24 David Levin 2010-11-25 07:53:20 PST
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
Comment 25 Eric Seidel (no email) 2010-11-25 08:39:15 PST
Attachment 74876 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6342043
Comment 26 Zoltan Horvath 2010-11-26 05:53:35 PST
Created attachment 74926 [details]
updated rc
Comment 27 Zoltan Horvath 2010-11-26 05:54:36 PST
(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.
Comment 28 Zoltan Horvath 2010-11-30 01:29:29 PST
Created attachment 75113 [details]
updated proposed patch rc

Updated to latest.
Comment 29 David Levin 2010-12-03 10:38:01 PST
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?
Comment 30 Zoltan Horvath 2010-12-06 07:41:43 PST
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.
Comment 31 Early Warning System Bot 2010-12-06 08:31:01 PST
Attachment 75696 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6746081
Comment 32 Zoltan Horvath 2010-12-06 08:43:35 PST
Created attachment 75700 [details]
updated proposed patch
Comment 33 David Levin 2010-12-06 11:01:29 PST
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.
Comment 34 David Levin 2010-12-06 11:06:15 PST
(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.
Comment 35 Zoltan Horvath 2010-12-07 06:43:40 PST
Created attachment 75808 [details]
updated, ews test
Comment 36 Zoltan Horvath 2010-12-07 06:48:41 PST
(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?
Comment 37 Zoltan Horvath 2010-12-07 06:56:39 PST
(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.
Comment 38 WebKit Review Bot 2010-12-07 08:49:23 PST
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.
Comment 39 WebKit Review Bot 2010-12-07 09:50:38 PST
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.
Comment 40 Early Warning System Bot 2010-12-07 10:31:49 PST
Attachment 75808 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6774094
Comment 41 WebKit Review Bot 2010-12-07 10:51:40 PST
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.
Comment 42 WebKit Review Bot 2010-12-07 11:52:56 PST
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.
Comment 43 Build Bot 2010-12-07 12:02:37 PST
Attachment 75808 [details] did not build on win:
Build output: http://queues.webkit.org/results/6853064
Comment 44 Build Bot 2010-12-07 13:08:35 PST
Attachment 75808 [details] did not build on win:
Build output: http://queues.webkit.org/results/6735090
Comment 45 WebKit Review Bot 2010-12-07 13:26:42 PST
Attachment 75808 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6790080
Comment 46 WebKit Review Bot 2010-12-07 21:35:05 PST
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 47 Eric Seidel (no email) 2010-12-24 09:47:08 PST
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.
Comment 48 Zoltan Horvath 2011-01-04 07:51:49 PST
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:'.
Comment 49 Build Bot 2011-01-04 08:21:08 PST
Attachment 77887 [details] did not build on win:
Build output: http://queues.webkit.org/results/7236421
Comment 50 Zoltan Horvath 2011-01-05 06:09:35 PST
Created attachment 77993 [details]
proposed patch (review candidate)

Windows build and Chromium win build fixed. Patch is updated.
Comment 51 Zoltan Horvath 2011-01-12 04:53:52 PST
Hey, 

is there any thoughts on this patch?

Z
Comment 52 Zoltan Horvath 2011-01-19 08:08:25 PST
Created attachment 79429 [details]
updated to latest, playing with ews-s
Comment 53 WebKit Review Bot 2011-01-19 08:11:22 PST
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.
Comment 54 WebKit Review Bot 2011-01-19 11:10:04 PST
Attachment 79429 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7551213
Comment 55 Zoltan Horvath 2011-01-20 01:55:50 PST
Created attachment 79564 [details]
proposed patch (review candidate)
Comment 56 WebKit Review Bot 2011-01-20 01:59:06 PST
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.
Comment 57 Zoltan Horvath 2011-01-20 02:21:30 PST
Created attachment 79566 [details]
proposed patch (review candidate) + efl fix
Comment 58 WebKit Review Bot 2011-01-20 02:22:59 PST
Attachment 79564 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7613242
Comment 59 Early Warning System Bot 2011-01-20 02:35:51 PST
Attachment 79564 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7534250
Comment 60 Zoltan Horvath 2011-01-20 02:35:58 PST
Created attachment 79568 [details]
proposed patch (review candidate)
Comment 61 WebKit Review Bot 2011-01-20 02:36:23 PST
Attachment 79564 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7499244
Comment 62 WebKit Review Bot 2011-01-20 02:39:30 PST
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.
Comment 63 Zoltan Horvath 2011-01-20 02:59:35 PST
Created attachment 79571 [details]
proposed patch (review candidate)
Comment 64 WebKit Review Bot 2011-01-20 03:02:47 PST
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.
Comment 65 WebKit Review Bot 2011-01-20 03:15:40 PST
Attachment 79568 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7549242
Comment 66 Zoltan Horvath 2011-01-20 03:24:09 PST
Created attachment 79572 [details]
proposed patch (review candidate)

EFL fix.
Comment 67 WebKit Review Bot 2011-01-20 03:25:16 PST
Attachment 79571 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7591225
Comment 68 WebKit Review Bot 2011-01-20 03:26:37 PST
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.
Comment 69 Early Warning System Bot 2011-01-20 03:51:48 PST
Attachment 79571 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7613245
Comment 70 WebKit Review Bot 2011-01-20 03:55:56 PST
Attachment 79564 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7510220
Comment 71 WebKit Review Bot 2011-01-20 04:10:28 PST
Attachment 79572 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7493236
Comment 72 Early Warning System Bot 2011-01-20 04:30:10 PST
Attachment 79572 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7627207
Comment 73 WebKit Review Bot 2011-01-20 04:38:49 PST
Attachment 79572 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7584224
Comment 74 Zoltan Horvath 2011-01-20 04:41:04 PST
Created attachment 79581 [details]
proposed patch
Comment 75 WebKit Review Bot 2011-01-20 04:44:40 PST
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 76 Csaba Osztrogonác 2011-01-20 07:55:15 PST
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)
Comment 77 WebKit Review Bot 2011-01-20 09:12:37 PST
http://trac.webkit.org/changeset/76248 might have broken Windows Debug (Build)
Comment 78 Csaba Osztrogonác 2011-01-20 09:32:31 PST
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 79 Csaba Osztrogonác 2011-01-20 09:33:12 PST
Comment on attachment 79581 [details]
proposed patch

remove r+ from landed patch