[Webkit-unassigned] [Bug 42174] [Qt] Implement QScriptEngine::newFunction() parts that doesn't depend on QScriptContext

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42174


Jędrzej Nowacki <jedrzej.nowacki at nokia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #61389|review?                     |
               Flag|                            |




--- Comment #2 from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>  2010-07-14 01:37:18 PST ---
(From update of attachment 61389)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.