[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