Bug 39115

Summary:

[Qt] QScriptEngine API should contain a newArray function

Product:

WebKit

Reporter:

Jędrzej Nowacki <jedrzej.nowacki>

Component:

JavaScriptCore

Assignee:

QtWebKit Unassigned <webkit-qt-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, commit-queue, hausmann, jedrzej.nowacki, kent.hansen, webkit.review.bot

Priority:

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+

Jędrzej Nowacki

Reported 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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Jędrzej Nowacki

Comment 1 2010-05-28 09:25:03 PDT

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

Caio Marcelo de Oliveira Filho

Comment 2 2010-06-18 08:23:56 PDT

Created attachment 59109 [details] Patch v1

WebKit Review Bot

Comment 3 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.

Caio Marcelo de Oliveira Filho

Comment 4 2010-06-18 10:37:40 PDT

Created attachment 59133 [details] Patch v2

Simon Hausmann

Comment 5 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.

Kent Hansen

Comment 6 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.

Caio Marcelo de Oliveira Filho

Comment 7 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.

Caio Marcelo de Oliveira Filho

Comment 8 2010-06-28 18:19:28 PDT

Created attachment 59969 [details] Patch

Caio Marcelo de Oliveira Filho

Comment 9 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.

Kenneth Rohde Christiansen

Comment 10 2010-06-28 18:43:46 PDT

Comment on attachment 59969 [details] Patch

WebKit Commit Bot

Comment 11 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>

WebKit Commit Bot

Comment 12 2010-06-28 20:04:51 PDT

All reviewed patches have been landed. Closing bug.

Jędrzej Nowacki

Comment 13 2010-06-29 01:24:47 PDT

Could you add a small benchmark?

Caio Marcelo de Oliveira Filho

Comment 14 2010-07-01 07:04:46 PDT

Created attachment 60242 [details] Simple benchmark test

Kenneth Rohde Christiansen

Comment 15 2010-07-01 11:26:32 PDT

Comment on attachment 60242 [details] Simple benchmark test Next time reopen the bug :-)

Note You need to log in before you can comment on or make changes to this bug.