Bug 39115 - [Qt] QScriptEngine API should contain a newArray function
Summary: [Qt] QScriptEngine API should contain a newArray function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: QtWebKit Unassigned
URL:
Keywords: Qt, QtTriaged
Depends on: 40970
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-05-14 07:24 PDT by Jędrzej Nowacki
Modified: 2010-07-01 11:26 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (14.51 KB, patch)
2010-06-18 08:23 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch v2 (14.51 KB, patch)
2010-06-18 10:37 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (5.29 KB, patch)
2010-06-28 18:19 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Simple benchmark test (3.62 KB, patch)
2010-07-01 07:04 PDT, Caio Marcelo de Oliveira Filho
kenneth: review+
kenneth: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 :-)