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

• Component: New Bugs
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

[Qt] Implement QScriptEngine::newFunction() parts that doesn't depend on QScriptContext

[Caio Marcelo de Oliveira Filho]

Created attachment 61389 [details]
Patch

[Jędrzej Nowacki]

Comment on attachment 61389 [details]
Patch

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.

[Kent Hansen]

(In reply to comment #1)
> Created an attachment (id=61389) [details]
> Patch

Nice start.
Why the "### test return value!" in the test?
Like Jedrzej said, it's nicer to use QEXPECT_FAIL or QSKIP, ideally with a link to the bug in the message.

[Jędrzej Nowacki]

Can we use the same instance of JSClassRef in a different context?

It is not necessary to release JSClassRef? If you don't release protected JSValueRef JSC would assert, it is not similar?

[Caio Marcelo de Oliveira Filho]

Created attachment 62488 [details]
Patch v2

Thanks Jezdrej and Kent for the reviews. This patch fixes the issues pointed out, add suggested tests and added a new internal function to share code between the two newFunction() implementations. I'll be more careful with code style when importing the comments and test cases from Qt's upstream from now on ;-)

Some comments on specific issues:

- QEXPECT_FAIL was added for the commented tests. If I understood the issues right, the two cases of checking property flags fail due to different reasons:

The property "constructor" of the newly created prototype fails because of bug 40631 (can't change the property flags).
On the other hand, when checking the flags of the property "prototype" of the function object, it enters the if-statement in JSCallbackObject::getOwnPropertyDescriptor, which returns hardcoded values. If I understood correctly, this issue is related to the bug 33946 and the bug 41937.

- JSClassRef can be shared by different contexts, but after discussion with Jezdrej, I changed to instantiate it for each QScriptEngine, so we can properly release it. We can implementing the sharing of this later if necessary.

[Caio Marcelo de Oliveira Filho]

Bah. Late night typo, sorry :-P

Consider s/Jezdrej/Jędrzej/g in the last comment.

[Jędrzej Nowacki]

Comment on attachment 62488 [details]
Patch v2

(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 :-)

[Caio Marcelo de Oliveira Filho]

Created attachment 62565 [details]
Patch v3

Fix issues pointed out by Jedrzej, in particular use JSClassCreate directly.

[Jędrzej Nowacki]

(In reply to comment #8)
> Created an attachment (id=62565) [details]
> Patch v3
> 
> Fix issues pointed out by Jedrzej, in particular use JSClassCreate directly.

Looks good to me, thanks :-)

[Kenneth Rohde Christiansen]

Comment on attachment 62565 [details]
Patch v3

Nice work!

[WebKit Commit Bot]

Comment on attachment 62565 [details]
Patch v3

Clearing flags on attachment: 62565

Committed r64130: <http://trac.webkit.org/changeset/64130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.