[webkit-reviews] review requested: [Bug 32565] [Qt] Initialization of a new API; QtScript. : [Attachment 47015] First steps of QtScript v5 (init+tests)

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has asked  for review:
Bug 32565: [Qt] Initialization of a new API; QtScript.
https://bugs.webkit.org/show_bug.cgi?id=32565

Attachment 47015: First steps of QtScript v5 (init+tests)
https://bugs.webkit.org/attachment.cgi?id=47015&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
> This seems like a good start! Some minor comments
Thanks for review :-),

> why qjavascript, not qtscript or qtjavascript?
Changed to qtscript.

> > +void QScriptEngine::collectGarbage()
> Maybe we should add a \sa to this method in the
> QWebSettings::clearMemoryCaches, so that people will know about it for mobile

> devices
There is no connection between QWebXXX and QScriptXXX. The QtScript is not
accessible from the QtWebkit, but it is one of the long term goals. So I think
it is a bit to early for documentation :-)

> > +/*!
> > +  \since 4.5
> Should these really have \since 4.5 ?
Our goal is to create a new implementation of QtScript source and binary
compatible with the current one. But yes it looks strange to put \since tag in
new API :-)
The tag was removed. Documentation was updated.

> You don't need to add engine and value here, and mixing val and value doesn't

> seem right.
All parameters names were unified.

> > +qreal QScriptValuePrivate::toInteger() const
> > +{
> > +	 // TODO it is not true implementation!
> > +	 return toNumber();
> > +}
> maybe add notImplemented() ???
notImplemented() is a private API, so I can't use it. Actually, toNumber()
cover more then 99% of use cases (difference will be for +-infinity, +-0, NaN),
and it is enough to test constructors.