[webkit-reviews] review canceled: [Bug 42174] [Qt] Implement QScriptEngine::newFunction() parts that doesn't depend on QScriptContext : [Attachment 61389] Patch

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has canceled Caio Marcelo de
Oliveira Filho <caio.oliveira at openbossa.org>'s request for review:
Bug 42174: [Qt] Implement QScriptEngine::newFunction() parts that doesn't
depend on QScriptContext
https://bugs.webkit.org/show_bug.cgi?id=42174

Attachment 61389: Patch
https://bugs.webkit.org/attachment.cgi?id=61389&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
Small feedback (some of issues are reported once but exists in a few places):

Documentation is badly indented (2 instead of 4 spaces).

JavaScriptCore/qt/api/qscriptengine_p.cpp:141
 +	result->setProperty(QString::fromAscii("prototype"), proto,
QScriptValue::Undeletable);
 +	proto->setProperty(QString::fromAscii("constructor"), result,
QScriptValue::PropertyFlags(QScriptValue::Undeletable |
QScriptValue::SkipInEnumeration));
Try to avoid the data conversion. In this place this function would be faster:
inline void QScriptValuePrivate::setProperty(T name, QScriptValuePrivate*
value, const QScriptValue::PropertyFlags& flags)

JavaScriptCore/qt/api/qscriptfunction.cpp:104
 +	Q_ASSERT(result->engine() == engine);
What will happen if fun() would return an invalid QSCP? I think it should be a
qWarning. You can use an value returned from assignEngine() to trigger it. It
would be nice to have an autotest to cover this case. Actually there is no test
that checks what happen if unbounded value is returned (like for example
QScriptValue(1024))

JavaScriptCore/qt/api/qscriptfunction_p.h:42
 +	    : engine(engine), fun(fun), arg(arg) { }
If I remember correctly, this should be in separate line.

JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:63
 +  static QScriptValue myFunction(QScriptContext *, QScriptEngine *eng)
* in wrong place (in all autotests).

JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:94
 +		// QCOMPARE(fun.propertyFlags("prototype"),
QScriptValue::Undeletable);
Could you leave it as QEXPECT_FAIL instead of a comment? The API is there, so
it would compile, even if a result is bad we know that it doesn't crash (it
shouldn't). Btw. do you know why it doesn't work?

JavaScriptCore/qt/api/qscriptfunction_p.h:28
 +  struct NativeFunctionData
Could you add Q to the name?

JavaScriptCore/qt/api/qscriptfunction.cpp:54
 +  JSClassDefinition qt_NativeFunctionClass = {
JavaScriptCore/qt/api/qscriptengine_p.cpp:131
 +	static JSClassRef nativeFunctionClass = qt_createNativeFunctionClass();

I like symmetry what about qt_NativeFunctionWithoutArgClass, but it is not a
strong opinion. If you like it it can stay :-)

JavaScriptCore/qt/api/qscriptengine.h:68
 +
You can remove this empty line.

JavaScriptCore/qt/api/qscriptengine_p.cpp:133
 +	// Note that this private data will be deleted in the class' finalize
function.
It is rather an object finalize function (which is defined in a class).

JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:68
 +  static QScriptValue myFunctionWithVoidArg(QScriptContext *, QScriptEngine
*eng, void *)
I think that we should return void* value, just to check if arguments are
passed correctly.