RESOLVED FIXED 41667
[Qt] QScriptEngine should have an API for creating Date objects
https://bugs.webkit.org/show_bug.cgi?id=41667
Summary [Qt] QScriptEngine should have an API for creating Date objects
Attachments
Patch (18.11 KB, patch)
2010-07-13 15:39 PDT, Caio Marcelo de Oliveira Filho
no flags
Patch v2 (35.67 KB, patch)
2010-07-20 22:46 PDT, Caio Marcelo de Oliveira Filho
kenneth: review+
commit-queue: commit-queue-
Rebased patch (16.12 KB, patch)
2010-09-29 07:28 PDT, Caio Marcelo de Oliveira Filho
no flags
Kent Hansen
Comment 1 2010-07-06 03:24:11 PDT
There should also be corresponding methods QScriptValue::isDate() and QScriptValue::toDateTime().
Caio Marcelo de Oliveira Filho
Comment 2 2010-07-13 15:39:07 PDT
Jędrzej Nowacki
Comment 3 2010-07-14 02:17:29 PDT
Comment on attachment 61425 [details] Patch Documentation is badly indented (2 instead of 4 spaces). Could you remove code duplication? both QSEP::newDate() do the same with the same algoritm, only one difference is an argument data type.JavaScriptCore/qt/api/qscriptvalue_p.h:692 + m_engine->setException(exception); exception is not null, so NotNullException could be added. JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:560 + Could you remove the space? JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:590 + #ifndef Q_WS_WIN // TODO: Test and remove this since 169701 has been fixed Is it still needed? JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue_generated_istype.cpp:576 + "engine->evaluate(\"Date.prototype\")", + "engine->newDate(QDateTime())"}; This won't work. You haven't added this values to tst_QScriptValue::initScriptValues() and to expected values for different generated functions. In the Qt repository is a generator for these files, ping me on IRC I will explain how to use it.
Caio Marcelo de Oliveira Filho
Comment 4 2010-07-20 22:46:05 PDT
Created attachment 62146 [details] Patch v2 Thanks for the review, Jedrzej. New version, taking into account the comments: - QSEP::newDate code duplication removed - Indentation/style fixed - Tests properly generated using tools in Qt upstream And also: - Make use of the new QScriptOriginalGlobalObject instead of fetching/storing manually the Date constructor/prototype - If QDateTime passed in the newDate() is not valid, return an invalid date
Kenneth Rohde Christiansen
Comment 5 2010-08-16 05:16:05 PDT
Comment on attachment 62146 [details] Patch v2 Looks good to me. If Jedrej is OK with it, please set cq
Andreas Kling
Comment 6 2010-09-17 19:40:58 PDT
@Jędrzej: Ping.
Jędrzej Nowacki
Comment 7 2010-09-20 00:17:12 PDT
Looks ok. Sorry for the lag.
Andreas Kling
Comment 8 2010-09-28 07:40:26 PDT
Comment on attachment 62146 [details] Patch v2 LGTM, too. The new API needs "\since 4.8" tags though, please fix that before landing.
WebKit Commit Bot
Comment 9 2010-09-28 08:44:47 PDT
Comment on attachment 62146 [details] Patch v2 Rejecting patch 62146 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 62146]" exit_code: 2 Cleaning working directory Updating working directory Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=62146&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=41667&ctype=xml Processing 1 patch from 1 bug. Processing patch 62146 from bug 41667. Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/3996176
Caio Marcelo de Oliveira Filho
Comment 10 2010-09-29 07:28:18 PDT
Created attachment 69193 [details] Rebased patch Rebased and removed the changes to autogenerated tasks. There are some manual tests already and talking to Jedrzej, we agreed that the auto-generated tests can be independently updated later, in a batch update.
Andreas Kling
Comment 11 2010-09-29 08:06:49 PDT
Comment on attachment 69193 [details] Rebased patch Echoing Kenneth's r+
WebKit Commit Bot
Comment 12 2010-09-29 08:56:02 PDT
Comment on attachment 69193 [details] Rebased patch Rejecting patch 69193 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: s successful. Files=14, Tests=304, 1 wallclock secs ( 0.72 cusr + 0.16 csys = 0.88 CPU) Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Projects/CommitQueue/LayoutTests Testing 21477 test cases. plugins/plugin-initiate-popup-window.html -> failed Exiting early after 1 failures. 17960 tests run. 322.64s total testing time 17959 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 31 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/4184018
Andreas Kling
Comment 13 2010-09-29 09:06:09 PDT
Comment on attachment 69193 [details] Rebased patch Let's try that once again.
WebKit Commit Bot
Comment 14 2010-09-29 09:31:30 PDT
Comment on attachment 69193 [details] Rebased patch Clearing flags on attachment: 69193 Committed r68651: <http://trac.webkit.org/changeset/68651>
WebKit Commit Bot
Comment 15 2010-09-29 09:31:37 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.