[webkit-reviews] review requested: [Bug 32565] [Qt] Initialization of a new API; QtScript. : [Attachment 47397] First steps of QtScript v7

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has asked  for review:
Bug 32565: [Qt] Initialization of a new API; QtScript.
https://bugs.webkit.org/show_bug.cgi?id=32565

Attachment 47397: First steps of QtScript v7
https://bugs.webkit.org/attachment.cgi?id=47397&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
(In reply to comment #22)
> (From update of attachment 47112 [details])
> > +++ b/JavaScriptCore/qt/api/QtScript.pro
> > @@ -0,0 +1,37 @@
> > +TARGET	= QtJavaScript
> 
> I think that should be QtScript.
Fixed.

> > +	     This library is free software; you can redistribute it and/or
> > +	 modify it under the terms of the GNU Library General Public
> 
> ^^^ Looks like a stray indentation here :)
Fixed.
 
> > +class QScriptEnginePrivate
> > +	     : public QSharedData {
Massive fix :-)

> > +public:
> > +	 static QScriptEnginePtr get(const QScriptEngine* q) { return q ?
q->d_ptr : QScriptEnginePtr(); }
> 
> Can it happen that get() is called with a null q pointer?
> 
> > +	 static QScriptEngine* get(const QScriptEnginePrivate* d) { return d ?
d->q_ptr : 0; }
> 
> Same question here. (Just wondering if the extra branch is necessary)
It was possible, but I changed QScriptValue::engine and all bounded
constructors of the QScriptValuePrivate. So corrected.

 
> > +void QScriptEnginePrivate::collectGarbage()
> > +{
> > +	 JSGarbageCollect(context());
> 
> I think this function and the other helper functions below could use
m_context
> directly.
Corrected.
 
> > +QScriptValuePrivate::QScriptValuePrivate(QString string)
> 
> I suggest to pass the QString by const reference here.
Done.

> > +bool QScriptValuePrivate::isBool()
> > +bool QScriptValuePrivate::isNumber()
> > +bool QScriptValuePrivate::isNull()
> > +bool QScriptValuePrivate::isString()
> > +bool QScriptValuePrivate::isUndefined()
> > +bool QScriptValuePrivate::isError()
> > +bool QScriptValuePrivate::isObject()
> > +bool QScriptValuePrivate::isFunction()
> > +QString QScriptValuePrivate::toString()
> 
> Shouldn't these functions be const?
Changed in toString(), others can change internal state of the
QScriptValuePrivate.
 
> > +bool QScriptValuePrivate::assignEngine(QScriptEnginePrivate* engine)
> > +{
> [...]
> > +	 m_engine = const_cast<QScriptEnginePrivate*>(engine);
> 
> Why is the const_cast needed here?
Fixed.
 
> > +QScriptValuePrivate* QScriptValuePrivate::call(const QScriptValuePrivate*
, const QScriptValueList &args)
> 
> Looks like a stray space before the comma here :)
Fixed.

> > +void tst_QScriptValue::toString_data()
> > +{
> > +	 QTest::addColumn<QString>("code");
> > +	 QTest::addColumn<QString>("result");
> > +
> > +	 QTest::newRow("string") << "'hello'" << "hello";
> > +	 QTest::newRow("string utf") << "'ąśćżźółńę'" <<
"ąśćżźółńę";
> 
> If this is a UTF-8 encoded string, then you have to construct it using
> QString::fromUtf8.
Fixed.