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

[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 62488: Patch v2
https://bugs.webkit.org/attachment.cgi?id=62488&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
(In reply to comment #5)
> The property "constructor" of the newly created prototype fails because of
bug 40631 (can't change the property flags).
Typo, it should be "bug 40613" ;-)

JavaScriptCore/qt/api/qscriptfunction.cpp:112
 +	    return *new QScriptValuePrivate(engine,
QScriptValue::UndefinedValue);
JavaScriptCore/qt/api/qscriptfunction.cpp:121
 +	    return *new QScriptValuePrivate(engine,
QScriptValue::UndefinedValue);
JavaScriptCore/qt/api/qscriptfunction.cpp:57
 +	    return *new QScriptValuePrivate(engine,
QScriptValue::UndefinedValue);
JavaScriptCore/qt/api/qscriptfunction.cpp:48
 +	    return *new QScriptValuePrivate(engine,
QScriptValue::UndefinedValue);
You should use engine->makeJSValue(QScriptValue::UndefinedValue) here. It would
be faster and it wouldn't cause a memory leak.

JavaScriptCore/qt/api/qscriptfunction.cpp:109
 +	QScriptValuePrivate* result = QScriptValuePrivate::get(data->fun(0,
QScriptEnginePrivate::get(engine), data->arg));
JavaScriptCore/qt/api/qscriptfunction.cpp:45
 +	QScriptValuePrivate* result = QScriptValuePrivate::get(data->fun(0,
QScriptEnginePrivate::get(engine)));
Can you add comment about 0 value?

JavaScriptCore/qt/api/qscriptfunction.cpp:86
 +  JSClassRef qt_createNativeFunctionClass()
JavaScriptCore/qt/api/qscriptfunction.cpp:150
 +  JSClassRef qt_createNativeFunctionWithArgClass()
Why you need these functions? Can't you use JSClassCreate directly?

JavaScriptCore/qt/api/qscriptengine_p.cpp:112
 +  QScriptValuePrivate* QScriptEnginePrivate::newFunction_helper(JSObjectRef
funJS, QScriptValuePrivate* prototype)
Could you rename it? Underscore should be avoided. I think that newFunction is
the correct name.

JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:128
 +		QEXPECT_FAIL("", "JSCallbackObject::getOwnPropertyDescriptor()
doesn't return correct information yet", Continue);
Can you provide a bug number? I have seen that the bug 33946 got fixed, maybe
it works now :-)