Bug 39115 – [Qt] QScriptEngine API should contain a newArray function

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jędrzej Nowacki]

QScriptEngine API should contain a newArray function, that return an object of the Array class.
It should look like http://doc.trolltech.com/4.6/qscriptengine.html#newArray

[Jędrzej Nowacki]

To test the functionality it would be nice to have implemented QScriptValue::prototype() and QScriptValue::property().

[Caio Marcelo de Oliveira Filho]

Created attachment 59109 [details]
Patch v1

[WebKit Review Bot]

Attachment 59109 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptvalue_p.h"
WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptvalue.cpp"
WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptengine_p.cpp"
JavaScriptCore/ChangeLog:24:  Line contains tab character.  [whitespace/tab] [5]
WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptengine_p.h"
WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptengine.cpp"
WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptengine.h"
WARNING: File exempt from style guide. Skipping: "JavaScriptCore/qt/api/qscriptvalue.h"
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Marcelo de Oliveira Filho]

Created attachment 59133 [details]
Patch v2

[Simon Hausmann]

Caio, I suggest so split out the JSC C API change into a separate bug report, to be reviewed by the Apple guys.

[Kent Hansen]

Comment on attachment 59133 [details]
Patch v2

Hi Caio,
Very cool that you're helping with this!
QScriptEngine::newArray(uint) should construct an array with the given length, but without creating any elements. It should behave like described in 15.4.2.2 in ECMA-262.
You don't have to modify the C API for achieving this; constructing an empty array and then setting the "length" property should be fine too.
That said, I think the C API would be more useful if it does what you propose. Like Simon said, that is best done in a separate change, and it would need a test added to JavaScriptCore/API/tests/testapi.c.
I also think prototype() and property() should be implemented separately (even if only done partially in the first commit); there are separate bugs for that, https://bugs.webkit.org/show_bug.cgi?id=39356 and https://bugs.webkit.org/show_bug.cgi?id=40903.

[Caio Marcelo de Oliveira Filho]

Thanks for the comments, guys. I'll follow the suggestions. To begin, I already uploaded a (partial but already useful) patch for the property() bug. :)

The idea here was to enhance the C API to give us the same code path that we have for the one-argument constructor, and use that. This would give us the same optimization that the one-argument ctor would get in JavaScript (today this means an allocation of the internal vector in the construction time).

Anyway, I'll raise that question in a different bug/patch for JSC people to look at.

[Caio Marcelo de Oliveira Filho]

Created attachment 59969 [details]
Patch

[Caio Marcelo de Oliveira Filho]

After discussion in bug 40970, modifying the C API to support the "odd" one argument constructor is not the best approach.

So this new patch do the simplest thing which is setting the length property on an empty array. As Kent pointed out, this fulfill our requirements. If the improvements suggested in 40970 are implemented (use "length" property as storage size hint), QScriptEngine::newArray() will benefit automatically.

[Kenneth Rohde Christiansen]

Comment on attachment 59969 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 59969 [details]
Patch

Clearing flags on attachment: 59969

Committed r62078: <http://trac.webkit.org/changeset/62078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jędrzej Nowacki]

Could you add a small benchmark?

[Caio Marcelo de Oliveira Filho]

Created attachment 60242 [details]
Simple benchmark test

[Kenneth Rohde Christiansen]

Comment on attachment 60242 [details]
Simple benchmark test

Next time reopen the bug :-)