[webkit-reviews] review requested: [Bug 32565] [Qt] Initialization of a new API; QtScript. : [Attachment 47112] First steps of QtScript v6

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has asked  for review:
Bug 32565: [Qt] Initialization of a new API; QtScript.
https://bugs.webkit.org/show_bug.cgi?id=32565

Attachment 47112: First steps of QtScript v6
https://bugs.webkit.org/attachment.cgi?id=47112&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
> As a JSC engineer I would r- this due to the QScriptValue constructors that
do
> not take a context, there seems to be a lot of work to make this doable but
it
> seems unnecessary - detaching the value from the engine implies that values
are
> transferrable, which they are not.
A QScriptValue object shouldn't be used in different instances of
QScriptEngine, so association between QScriptValue and QScriptEngine is
unbreakable.
Code created with context-less constructors is just nicer. For example:
foo.call(thisObject, QScriptValueList()<< "hello" << "world"); 
is cleaner then
foo.call(thisObject, QScriptValueList()<< QScriptValue(&engine, "hello") <<
QScriptValue(&engine, "world")); 

> Would be good to see assertions in the API to ensure at least debug code
> doesn't allow value made in one context to be passed another.
I added a few warnings in public API and a few asserts in private part of API.
In Qt we try to avoid asserts in public API.

> Also unsure how GC is expected to work, the JSValueRef for a value is held in
a
> QScriptValuePrivate, but QScriptValuePrivate is heap allocated so won't be on

> the stack -> GC won;t be aware of value, thus leading to collection at which
> point QScriptValuePrivate will be holding a dangling pointer.
Nice catch, fixed.

> How are exceptions meant to be caught/seen/handled?  All conversion functions

> may throw, yet there is no indication of this.
Right, this patch doesn't include exception handling. This feature is going to
be implemented in future. The last uncaught exception should be visible from
QScriptEngine::uncaughtException() (which is not implemented).