[webkit-reviews] review denied: [Bug 37729] [Qt] QScriptValuePrivate class needs some cleanup. : [Attachment 53562] Fix v1

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Jędrzej Nowacki
<jedrzej.nowacki at nokia.com>'s request for review:
Bug 37729: [Qt] QScriptValuePrivate class needs some cleanup.
https://bugs.webkit.org/show_bug.cgi?id=37729

Attachment 53562: Fix v1
https://bugs.webkit.org/attachment.cgi?id=53562&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
> 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 at 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.