[webkit-reviews] review canceled: [Bug 41667] [Qt] QScriptEngine should have an API for creating Date objects : [Attachment 61425] Patch

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has canceled Caio Marcelo de
Oliveira Filho <caio.oliveira at openbossa.org>'s request for review:
Bug 41667: [Qt] QScriptEngine should have an API for creating Date objects
https://bugs.webkit.org/show_bug.cgi?id=41667

Attachment 61425: Patch
https://bugs.webkit.org/attachment.cgi?id=61425&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
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.