Bug 42174 - [Qt] Implement QScriptEngine::newFunction() parts that doesn't depend on QScriptContext
Summary: [Qt] Implement QScriptEngine::newFunction() parts that doesn't depend on QScr...
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
Keywords: Qt, QtTriaged
Depends on:
Blocks: 41662
  Show dependency treegraph
Reported: 2010-07-13 09:38 PDT by Caio Marcelo de Oliveira Filho
Modified: 2010-07-27 08:37 PDT (History)
5 users (show)

See Also:

Patch (26.91 KB, patch)
2010-07-13 09:47 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch v2 (32.71 KB, patch)
2010-07-23 23:28 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch v3 (32.40 KB, patch)
2010-07-26 06:22 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2010-07-13 09:38:48 PDT
[Qt] Implement QScriptEngine::newFunction() parts that doesn't depend on QScriptContext
Comment 1 Caio Marcelo de Oliveira Filho 2010-07-13 09:47:01 PDT
Created attachment 61389 [details]
Comment 2 Jędrzej Nowacki 2010-07-14 01:37:18 PDT
Comment on attachment 61389 [details]

Small feedback (some of issues are reported once but exists in a few places):

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

 +      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)

 +      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))

 +          : engine(engine), fun(fun), arg(arg) { }
If I remember correctly, this should be in separate line.

 +  static QScriptValue myFunction(QScriptContext *, QScriptEngine *eng)
* in wrong place (in all autotests).

 +              // 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?

 +  struct NativeFunctionData
Could you add Q to the name?

 +  JSClassDefinition qt_NativeFunctionClass = {
 +      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 :-)

You can remove this empty line.

 +      // 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).

 +  static QScriptValue myFunctionWithVoidArg(QScriptContext *, QScriptEngine *eng, void *)
I think that we should return void* value, just to check if arguments are passed correctly.
Comment 3 Kent Hansen 2010-07-14 02:08:41 PDT
(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.
Comment 4 Jędrzej Nowacki 2010-07-14 03:31:43 PDT
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?
Comment 5 Caio Marcelo de Oliveira Filho 2010-07-23 23:28:58 PDT
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.
Comment 6 Caio Marcelo de Oliveira Filho 2010-07-24 04:59:24 PDT
Bah. Late night typo, sorry :-P

Consider s/Jezdrej/Jędrzej/g in the last comment.
Comment 7 Jędrzej Nowacki 2010-07-25 23:57:48 PDT
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" ;-)

 +          return *new QScriptValuePrivate(engine, QScriptValue::UndefinedValue);
 +          return *new QScriptValuePrivate(engine, QScriptValue::UndefinedValue);
 +          return *new QScriptValuePrivate(engine, QScriptValue::UndefinedValue);
 +          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.

 +      QScriptValuePrivate* result = QScriptValuePrivate::get(data->fun(0, QScriptEnginePrivate::get(engine), data->arg));
 +      QScriptValuePrivate* result = QScriptValuePrivate::get(data->fun(0, QScriptEnginePrivate::get(engine)));
Can you add comment about 0 value?

 +  JSClassRef qt_createNativeFunctionClass()
 +  JSClassRef qt_createNativeFunctionWithArgClass()
Why you need these functions? Can't you use JSClassCreate directly?

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

 +              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 :-)
Comment 8 Caio Marcelo de Oliveira Filho 2010-07-26 06:22:57 PDT
Created attachment 62565 [details]
Patch v3

Fix issues pointed out by Jedrzej, in particular use JSClassCreate directly.
Comment 9 Jędrzej Nowacki 2010-07-27 00:10:32 PDT
(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 :-)
Comment 10 Kenneth Rohde Christiansen 2010-07-27 07:54:27 PDT
Comment on attachment 62565 [details]
Patch v3

Nice work!
Comment 11 WebKit Commit Bot 2010-07-27 08:37:39 PDT
Comment on attachment 62565 [details]
Patch v3

Clearing flags on attachment: 62565

Committed r64130: <http://trac.webkit.org/changeset/64130>
Comment 12 WebKit Commit Bot 2010-07-27 08:37:46 PDT
All reviewed patches have been landed.  Closing bug.