[webkit-reviews] review denied: [Bug 32565] [Qt] Initialization of a new API; QtScript. : [Attachment 47112] First steps of QtScript v6
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 25 05:38:12 PST 2010
Simon Hausmann <hausmann at webkit.org> has denied Jędrzej Nowacki
<jedrzej.nowacki at nokia.com>'s request for review:
Bug 32565: [Qt] Initialization of a new API; QtScript.
https://bugs.webkit.org/show_bug.cgi?id=32565
Attachment 47112: First steps of QtScript v6
https://bugs.webkit.org/attachment.cgi?id=47112&action=review
------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> +++ b/JavaScriptCore/qt/api/QtScript.pro
> @@ -0,0 +1,37 @@
> +TARGET = QtJavaScript
I think that should be QtScript.
> +++ b/JavaScriptCore/qt/api/qscriptengine_p.h
> @@ -0,0 +1,99 @@
> +/*
> + Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)
> +
> + 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 :)
> +class QScriptEnginePrivate
> + : public QSharedData {
I think it's more common in WebKit to have the ": public QSharedData {" on the
previous line.
> +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)
> +void QScriptEnginePrivate::collectGarbage()
> +{
> + JSGarbageCollect(context());
I think this function and the other helper functions below could use m_context
directly.
> +class QScriptValuePrivate
> + : public QSharedData {
Same style comment as earlier (new newline before ": public")
> +QScriptValuePrivate::QScriptValuePrivate(QString string)
I suggest to pass the QString by const reference here.
> +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?
> +bool QScriptValuePrivate::assignEngine(QScriptEnginePrivate* engine)
> +{
[...]
> + m_engine = const_cast<QScriptEnginePrivate*>(engine);
Why is the const_cast needed here?
> +QScriptValuePrivate* QScriptValuePrivate::call(const QScriptValuePrivate* ,
const QScriptValueList &args)
Looks like a stray space before the comma here :)
> +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.
The implicit QString(const char*) constructor works most of the time fine on
Linux only because nowadays the locale encoding is usually utf-8.
r- until the above comments are addressed. The rest looks good to me!
More information about the webkit-reviews
mailing list