Bug 41667 - [Qt] QScriptEngine should have an API for creating Date objects
Summary: [Qt] QScriptEngine should have an API for creating Date objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-07-06 02:07 PDT by Kent Hansen
Modified: 2010-09-29 09:31 PDT (History)
4 users (show)

See Also:


Attachments
Patch (18.11 KB, patch)
2010-07-13 15:39 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch v2 (35.67 KB, patch)
2010-07-20 22:46 PDT, Caio Marcelo de Oliveira Filho
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Rebased patch (16.12 KB, patch)
2010-09-29 07:28 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Kent Hansen 2010-07-06 03:24:11 PDT
There should also be corresponding methods QScriptValue::isDate() and QScriptValue::toDateTime().
Comment 2 Caio Marcelo de Oliveira Filho 2010-07-13 15:39:07 PDT
Created attachment 61425 [details]
Patch
Comment 3 Jędrzej Nowacki 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.
Comment 4 Caio Marcelo de Oliveira Filho 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
Comment 5 Kenneth Rohde Christiansen 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
Comment 6 Andreas Kling 2010-09-17 19:40:58 PDT
@Jędrzej: Ping.
Comment 7 Jędrzej Nowacki 2010-09-20 00:17:12 PDT
Looks ok. Sorry for the lag.
Comment 8 Andreas Kling 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.
Comment 9 WebKit Commit Bot 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
Comment 10 Caio Marcelo de Oliveira Filho 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.
Comment 11 Andreas Kling 2010-09-29 08:06:49 PDT
Comment on attachment 69193 [details]
Rebased patch

Echoing Kenneth's r+
Comment 12 WebKit Commit Bot 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
Comment 13 Andreas Kling 2010-09-29 09:06:09 PDT
Comment on attachment 69193 [details]
Rebased patch

Let's try that once again.
Comment 14 WebKit Commit Bot 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>
Comment 15 WebKit Commit Bot 2010-09-29 09:31:37 PDT
All reviewed patches have been landed.  Closing bug.