[webkit-reviews] review denied: [Bug 32565] [Qt] Initialization of a new API; QJavaScript. : [Attachment 44961] Initialization of API v2

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Jędrzej Nowacki
<jedrzej.nowacki at nokia.com>'s request for review:
Bug 32565: [Qt] Initialization of a new API; QJavaScript.
https://bugs.webkit.org/show_bug.cgi?id=32565

Attachment 44961: Initialization of API v2
https://bugs.webkit.org/attachment.cgi?id=44961&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> diff --git a/ChangeLog b/ChangeLog
> index 98d5de4..02e4cef 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,24 @@
> +2009-12-15  Jedrzej Nowacki	<jedrzej.nowacki at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Introduction to QJavaScript API.

Suggestion:

"First steps of the QJavaScript API"

> +	   Two new classes were created; QJavaScriptEngine and
QJavaScriptValue.
> +	   The first should encapsulate a java script context and the second a
script

"The first should encapsulate a javascript context and the second a script


> +	   value.
> +	   
> +	   This is beginning of development, so by default the classes won't be
compiled.

"This API is still in development, so it isn't compiled by default."

> +	   To compile it qmake option should be provided. For example:
> +	   build-webkit --qt --qmakearg="CONFIG+=build-qjavascript"
> +	   

"To trigger compilation, pass --qmakearg="CONFIG+=build-qjavascript" to
build-webkit."

> +	   Changes are necessary to create a new Qtish C++ API on top of the
JSC public
> +	   C API (bug 31863).

Leave that part out.


  
>	   Reviewed by Simon Fraser.
> diff --git a/JavaScriptCore/qt/api/QtJavaScript.pro
b/JavaScriptCore/qt/api/QtJavaScript.pro
> new file mode 100755
> index 0000000..a1b97ee
> --- /dev/null
> +++ b/JavaScriptCore/qt/api/QtJavaScript.pro
> @@ -0,0 +1,35 @@
> +TARGET     = QtJavaScript
> +TEMPLATE = lib
> +QT	      = core

These should be aligned or not at all.


> +
> +/** Convertion from QString to JSStringRef
> +TODO make it nicer... maybe casting operator?
> + at internal
> +*/

The public API will be using qdoc syntax, so we should use the same syntax
within the same file for internal documentation,
marked with \internal.

> +JSStringRef qStringToJSStringRef(const QString& str)
> +{
> +    return JSStringCreateWithUTF8CString(str.toUtf8().constData());
> +}

If this function is called only from within qjavascriptengine.cpp, then it
should be static. Otherwise it should probably be a class function of
QJavaScriptEnginePrivate.

> +class QJavaScriptEngine {

QJavaScriptEngine should be a subclass of QObject.

> +
> +class QJavaScriptEnginePrivate
> +	   : public QSharedData {

This class would not be a subclass of QSharedData anymore, once
QJavaScriptEngine subclasses QObject.

> +    QJavaScriptEnginePrivate();
> +    ~QJavaScriptEnginePrivate();
> +    QJavaScriptValue evaluate(const QString&);
> +    void collectGarbage();
> +    JSGlobalContextRef context() { return m_context; }

context() should be const.

> +
> +QString jSStringRefToQString(const JSStringRef& str)
> +{
> +    size_t size = JSStringGetMaximumUTF8CStringSize(str);
> +    char* buffer = new char[size];
> +    JSStringGetUTF8CString(str, buffer, size);
> +    QString result = QString::fromUtf8(buffer);
> +    delete buffer;
> +    return  result;
> +}

It would be faster to use JSStringGetLength and JSStringGetCharactersPtr to do
the conversion without an intermediate conversion to utf-8:

return QString(reinterpret_cast<const QChar*>(JSStringGetCharactersPtr(str)),
JSStringGetLength(str));



> +/** Returns true if this QScriptValue is valid; otherwise returns false. */
> +bool QJavaScriptValue::isValid()

This function should be const.

> +/** Returns the string value of this QScriptValue, as defined in ECMA-262
section 9.8, "ToString".
> +
> +Note that if this QScriptValue is an object, calling this function has side
effects on the script
> +engine, since the engine will call the object's toString() function (and
possibly valueOf()) in an
> +attempt to convert the object to a primitive value (possibly resulting in an
uncaught script
> +exception).
> +*/
> +QString QJavaScriptValue::toQString()

This should be just called toString(), for consistency with the Qt API.


> +    QJavaScriptValuePrivate(QJavaScriptEnginePtr engine, JSValueRef& value)

I would pass the JSValueRef by value.


> +++ b/JavaScriptCore/qt/api/qtjavascriptglobal.h
> @@ -0,0 +1,25 @@
> +#ifndef QTJAVASCRIPTGLOBAL_H
> +#define QTJAVASCRIPTGLOBAL_H
> +
> +#include <QtCore/qglobal.h>

This file is missing a license header.