Bug 37729

Summary: [Qt] QScriptValuePrivate class needs some cleanup.
Product: WebKit Reporter: Jędrzej Nowacki <jedrzej.nowacki>
Component: JavaScriptCoreAssignee: Jędrzej Nowacki <jedrzej.nowacki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jedrzej.nowacki, kenneth, kent.hansen
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 31863, 36594    
Attachments:
Description Flags
Fix v1
kenneth: review-, kenneth: commit-queue-
Fix v2 none

Jędrzej Nowacki
Reported 2010-04-16 14:00:40 PDT
QtScript implementation is placed in a private classes. Private classes should use only other private classes to avoid unnecessary conversions. QScriptValuePrivate breaks this rule, which is wrong. QScriptValuePrivate constructor shouldn't take QScriptEngine pointer as a parameter.
Attachments
Fix v1 (10.37 KB, patch)
2010-04-16 14:05 PDT, Jędrzej Nowacki 		kenneth: review-
kenneth: commit-queue-
DetailsFormatted DiffDiff
Fix v2 (11.13 KB, patch)
2010-04-20 10:43 PDT, Jędrzej Nowacki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jędrzej Nowacki
Comment 1 2010-04-16 14:05:50 PDT
Created attachment 53562 [details] Fix v1
Kenneth Rohde Christiansen
Comment 2 2010-04-19 19:45:57 PDT
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.
Jędrzej Nowacki
Comment 3 2010-04-20 10:43:51 PDT
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.
WebKit Commit Bot
Comment 4 2010-04-29 01:14:59 PDT
Comment on attachment 53844 [details] Fix v2 Clearing flags on attachment: 53844 Committed r58483: <http://trac.webkit.org/changeset/58483>
WebKit Commit Bot
Comment 5 2010-04-29 01:15:05 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.