Created attachment 53562 [details] Fix v1

Comment on attachment 53562 [details] Fix v1 > diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog > index 12327e4..d5621a3 100644 > --- a/JavaScriptCore/ChangeLog > +++ b/JavaScriptCore/ChangeLog > @@ -1,3 +1,20 @@ > +2010-04-16 Jedrzej Nowacki <jedrzej.nowacki@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + QtScript implementation is placed in a private classes. a classes? Wrong English > + use only other private classes. QScriptValuePrivate breaks this rule, which > + is wrong. No need to say it is wrong, you already made that clear. Remove dedundant information. > QScriptValuePrivate constructor shouldn't take QScriptEngine pointer > + as a parameter. Is this another comment? or are you fixing a second thing? > + if (engine) > + d_ptr = new QScriptValuePrivate(QScriptEnginePrivate::get(engine).data(), value); > + else > + d_ptr = new QScriptValuePrivate(value); > } I do not understand this get method? It gets the private of the engine? I guess you use a kind of smart pointer inside it, couldn't the method to the data() internally in that method. The code is not easy readable.

Created attachment 53844 [details] Fix v2 (In reply to comment #2) Changelog changed. > I do not understand this get method? It gets the private of the engine? Yes and the class do not have to be a friend, In general the get method is used for conversion between a public and a private class. In QtScript I decided to separete a public implementation and a private implementation. Public implementation is used only for converting to a private one. All real work is done in a private implementation. The design adventage is that we can inline nearby all methods in private imlementation and we do not waste time for converting values inside private implementation. If you want I can describe it in detail. > I guess > you use a kind of smart pointer inside it, couldn't the method to the data() > internally in that method. The code is not easy readable. You are right. It is done.

Comment on attachment 53844 [details] Fix v2 Clearing flags on attachment: 53844 Committed r58483: <http://trac.webkit.org/changeset/58483>

All reviewed patches have been landed. Closing bug.