Bug 37729 – [Qt] QScriptValuePrivate class needs some cleanup.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

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.

[Jędrzej Nowacki]

Created attachment 53562 [details]
Fix v1

[Kenneth Rohde Christiansen]

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 @@
> [phone]  Jedrzej Nowacki  <[e-mail]>
> +
> +        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]

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 on attachment 53844 [details]
Fix v2

Clearing flags on attachment: 53844

Committed r58483: <http://trac.webkit.org/changeset/58483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.