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: | P2 | Keywords: | Qt, QtTriaged | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 40970 | ||||||||||||
Bug Blocks: | 31863 | ||||||||||||
Attachments: |
|
Description
Jędrzej Nowacki
2010-05-14 07:24:15 PDT
To test the functionality it would be nice to have implemented QScriptValue::prototype() and QScriptValue::property(). Created attachment 59109 [details]
Patch v1
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.
Created attachment 59133 [details]
Patch v2
Caio, I suggest so split out the JSC C API change into a separate bug report, to be reviewed by the Apple guys. 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. 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. Created attachment 59969 [details]
Patch
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 on attachment 59969 [details]
Patch
Comment on attachment 59969 [details] Patch Clearing flags on attachment: 59969 Committed r62078: <http://trac.webkit.org/changeset/62078> All reviewed patches have been landed. Closing bug. Could you add a small benchmark? Created attachment 60242 [details]
Simple benchmark test
Comment on attachment 60242 [details]
Simple benchmark test
Next time reopen the bug :-)
|