Bug 41667 – [Qt] QScriptEngine should have an API for creating Date objects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Kent Hansen]

See http://doc.trolltech.com/latest/qscriptengine.html#newDate

[Kent Hansen]

There should also be corresponding methods QScriptValue::isDate() and QScriptValue::toDateTime().

[Caio Marcelo de Oliveira Filho]

Created attachment 61425 [details]
Patch

[Jędrzej Nowacki]

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]

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 on attachment 62146 [details]
Patch v2

Looks good to me. If Jedrej is OK with it, please set cq

[Andreas Kling]

@Jędrzej: Ping.

[Jędrzej Nowacki]

Looks ok. Sorry for the lag.

[Andreas Kling]

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 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 [e-mail]...
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]

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 on attachment 69193 [details]
Rebased patch

Echoing Kenneth's r+

[WebKit Commit Bot]

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 on attachment 69193 [details]
Rebased patch

Let's try that once again.

[WebKit Commit Bot]

Comment on attachment 69193 [details]
Rebased patch

Clearing flags on attachment: 69193

Committed r68651: <http://trac.webkit.org/changeset/68651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.