Bug 39115

Summary: [Qt] QScriptEngine API should contain a newArray function
Product: WebKit Reporter: Jędrzej Nowacki <jedrzej.nowacki>
Component: JavaScriptCoreAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, hausmann, jedrzej.nowacki, kent.hansen, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 40970    
Bug Blocks: 31863    
Attachments:
Description Flags
Patch v1
none
Patch v2
none
Patch
none
Simple benchmark test kenneth: review+, kenneth: commit-queue+

Description Jędrzej Nowacki 2010-05-14 07:24:15 PDT
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
Comment 1 Jędrzej Nowacki 2010-05-28 09:25:03 PDT
To test the functionality it would be nice to have implemented QScriptValue::prototype() and QScriptValue::property().
Comment 2 Caio Marcelo de Oliveira Filho 2010-06-18 08:23:56 PDT
Created attachment 59109 [details]
Patch v1
Comment 3 WebKit Review Bot 2010-06-18 08:28:16 PDT
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.
Comment 4 Caio Marcelo de Oliveira Filho 2010-06-18 10:37:40 PDT
Created attachment 59133 [details]
Patch v2
Comment 5 Simon Hausmann 2010-06-20 04:25:33 PDT
Caio, I suggest so split out the JSC C API change into a separate bug report, to be reviewed by the Apple guys.
Comment 6 Kent Hansen 2010-06-21 00:56:12 PDT
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.
Comment 7 Caio Marcelo de Oliveira Filho 2010-06-21 22:17:58 PDT
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.
Comment 8 Caio Marcelo de Oliveira Filho 2010-06-28 18:19:28 PDT
Created attachment 59969 [details]
Patch
Comment 9 Caio Marcelo de Oliveira Filho 2010-06-28 18:26:15 PDT
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.
Comment 10 Kenneth Rohde Christiansen 2010-06-28 18:43:46 PDT
Comment on attachment 59969 [details]
Patch
Comment 11 WebKit Commit Bot 2010-06-28 20:04:46 PDT
Comment on attachment 59969 [details]
Patch

Clearing flags on attachment: 59969

Committed r62078: <http://trac.webkit.org/changeset/62078>
Comment 12 WebKit Commit Bot 2010-06-28 20:04:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Jędrzej Nowacki 2010-06-29 01:24:47 PDT
Could you add a small benchmark?
Comment 14 Caio Marcelo de Oliveira Filho 2010-07-01 07:04:46 PDT
Created attachment 60242 [details]
Simple benchmark test
Comment 15 Kenneth Rohde Christiansen 2010-07-01 11:26:32 PDT
Comment on attachment 60242 [details]
Simple benchmark test

Next time reopen the bug :-)