Bug 32565 – [Qt] Initialization of a new API; QtScript.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

I would like to initialize new Qtish API, for more general information please look at the bug 31863. 

New functionality needed: It should be possible to evaluate a java script function and convert returned value to QString.

[Jędrzej Nowacki]

Created attachment 44885 [details]
Initialization of API v1

Initialization of API v1, patch introduce QJavaScriptEngine and QJavaScriptValue. With instance of these two classes it is possible to evaluate a java script code.

[Jędrzej Nowacki]

Created attachment 44886 [details]
Autotests

This patch is a supplement to the first one (Initialization of API v1) and contains an autotest for the new functionality.

[WebKit Review Bot]

Attachment 44885 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/qt/api/qjavascriptengine.h:44:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/api/qjavascriptvalue.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/api/qjavascriptvalue.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/qt/api/qjavascriptvalue.h:41:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/api/qjavascriptengine.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 5

[Adam Barth]

> JavaScriptCore/qt/api/qjavascriptengine.h:44:  d_ptr is incorrectly named.
> Don't use underscores in your identifier names.  [readability/naming] [4]

This looks like a false positive.  Are there other identifiers we should whitelist besides d_ptr?

[Jędrzej Nowacki]

Comment on attachment 44885 [details]
Initialization of API v1

r- -> #include "config.h" is missing

[Jędrzej Nowacki]

Created attachment 44961 [details]
Initialization of API v2

I have added #include "config.h" add the beginning of each implementation file.

Another problem that will be pointed by bot:
JavaScriptCore/qt/api/qjavascriptvalue.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
JavaScriptCore/qt/api/qjavascriptengine.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
I think it is wrong as the file is an implementation of two classes (public and private). I believe that the problem with sorting is caused by this misunderstood. When I call sort for a new headers:
$ ls |grep .h |sort
qjavascriptengine.h
qjavascriptengine_p.h
qjavascriptvalue.h
qjavascriptvalue_p.h
qtjavascriptglobal.h
which is what i tried to put into .cpp and .h files

[WebKit Review Bot]

Attachment 44961 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/qt/api/qjavascriptengine.h:44:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/api/qjavascriptvalue.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
JavaScriptCore/qt/api/qjavascriptvalue.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
JavaScriptCore/qt/api/qjavascriptvalue.h:41:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/api/qjavascriptengine.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 5

[Jędrzej Nowacki]

(In reply to comment #4)
> > JavaScriptCore/qt/api/qjavascriptengine.h:44:  d_ptr is incorrectly named.
> > Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> This looks like a false positive.  Are there other identifiers we should
> whitelist besides d_ptr?

I'm not sure but it seems that the d_ptr and the q_ptr only.

[Simon Hausmann]

Comment on attachment 44961 [details]
Initialization of API v2

> diff --git a/ChangeLog b/ChangeLog
> index 98d5de4..02e4cef 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,24 @@
> [phone]  Jedrzej Nowacki  <[e-mail]>
> +
> +        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?
> +@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.

[Jędrzej Nowacki]

Created attachment 46913 [details]
 Initialization of API v3

Completely rewritten code.

[Jędrzej Nowacki]

Created attachment 46915 [details]
Autotests v3

Additional autotest suite

[WebKit Review Bot]

Attachment 46913 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/qt/api/qscriptengine_p.h:54:  q_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/api/qscriptengine.h:46:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/api/qscriptvalue.h:92:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jędrzej Nowacki]

Comment on attachment 46915 [details]
Autotests v3

"-" badly generated changelog can't be applied by bots

[Jędrzej Nowacki]

Comment on attachment 46913 [details]
 Initialization of API v3

Kent have found some issues with the code:
- use of C cast
- toString() method should return "true", "false", "null", "undefined" instead of "True", "False", "null", "undefined"
- one spelling mistake

I will create another version of the patch... If there are other issues please feel free to comment :-)

[Jędrzej Nowacki]

Created attachment 47004 [details]
Autotests v4

[Jędrzej Nowacki]

Created attachment 47005 [details]
Initialization of API v4

[Kenneth Rohde Christiansen]

Comment on attachment 47005 [details]
Initialization of API v4

This seems like a good start! Some minor comments

> +        
> +        This API is still in development, so it isn't compiled by default.
> +        To trigger compilation, pass --qmakearg="CONFIG+=build-qjavascript" to

why qjavascript, not qtscript or qtjavascript?

> +void QScriptEngine::collectGarbage()
> +{
> +    d_ptr->collectGarbage();
> +}

Maybe we should add a \sa to this method in the QWebSettings::clearMemoryCaches, so that people will know about it for mobile devices

> +/*!
> +  \since 4.5
> +
> +  Returns true if this QScriptValue is of the primitive type Boolean;
> +  otherwise returns false.
> +
> +  \sa toBool()
> +*/

Should these really have \since 4.5 ?

> +
> +    QScriptValue();
> +    QScriptValue(bool value);
> +    QScriptValue(int value);
> +    QScriptValue(uint value);
> +    QScriptValue(qreal value);
> +    QScriptValue(const QString& value);
> +    QScriptValue(const char* value);
> +    QScriptValue(SpecialValue val);
> +    QScriptValue(const QScriptValue& other);
> +
> +    QScriptValue(QScriptEngine* engine, bool val);
> +    QScriptValue(QScriptEngine* engine, int val);
> +    QScriptValue(QScriptEngine* engine, uint val);
> +    QScriptValue(QScriptEngine* engine, qreal val);
> +    QScriptValue(QScriptEngine* engine, const QString& val);
> +    QScriptValue(QScriptEngine* engine, const char* value);
> +    QScriptValue(QScriptEngine* engine, SpecialValue val);
> +


You don't need to add engine and value here, and mixing val and value doesn't seem right.


> +qreal QScriptValuePrivate::toInteger() const
> +{
> +    // TODO it is not true implementation!
> +    return toNumber();
> +}

maybe add notImplemented() ???

[Jędrzej Nowacki]

Created attachment 47015 [details]
First steps of QtScript v5 (init+tests)

> 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.

[Oliver Hunt]

Comment on attachment 47015 [details]
First steps of QtScript v5 (init+tests)

As a JSC engineer I would r- this due to the QScriptValue constructors that do not take a context, there seems to be a lot of work to make this doable but it seems unnecessary - detaching the value from the engine implies that values are transferrable, which they are not.

Would be good to see assertions in the API to ensure at least debug code doesn't allow value made in one context to be passed another.

Also unsure how GC is expected to work, the JSValueRef for a value is held in a QScriptValuePrivate, but QScriptValuePrivate is heap allocated so won't be on the stack -> GC won;t be aware of value, thus leading to collection at which point QScriptValuePrivate will be holding a dangling pointer.

How are exceptions meant to be caught/seen/handled?  All conversion functions may throw, yet there is no indication of this.

[Jędrzej Nowacki]

Created attachment 47112 [details]
First steps of QtScript v6

> As a JSC engineer I would r- this due to the QScriptValue constructors that do
> not take a context, there seems to be a lot of work to make this doable but it
> seems unnecessary - detaching the value from the engine implies that values are
> transferrable, which they are not.
A QScriptValue object shouldn't be used in different instances of QScriptEngine, so association between QScriptValue and QScriptEngine is unbreakable.
Code created with context-less constructors is just nicer. For example:
foo.call(thisObject, QScriptValueList()<< "hello" << "world"); 
is cleaner then
foo.call(thisObject, QScriptValueList()<< QScriptValue(&engine, "hello") << QScriptValue(&engine, "world")); 

> Would be good to see assertions in the API to ensure at least debug code
> doesn't allow value made in one context to be passed another.
I added a few warnings in public API and a few asserts in private part of API. In Qt we try to avoid asserts in public API.

> Also unsure how GC is expected to work, the JSValueRef for a value is held in a
> QScriptValuePrivate, but QScriptValuePrivate is heap allocated so won't be on
> the stack -> GC won;t be aware of value, thus leading to collection at which
> point QScriptValuePrivate will be holding a dangling pointer.
Nice catch, fixed.

> How are exceptions meant to be caught/seen/handled?  All conversion functions
> may throw, yet there is no indication of this.
Right, this patch doesn't include exception handling. This feature is going to be implemented in future. The last uncaught exception should be visible from QScriptEngine::uncaughtException() (which is not implemented).

[WebKit Review Bot]

Attachment 47112 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring JavaScriptCore/qt/api/qscriptconverter_p.h; This file is exempt from the style guide.
Ignoring JavaScriptCore/qt/api/qscriptvalue_p.h; This file is exempt from the style guide.
Ignoring JavaScriptCore/qt/api/qscriptvalue.cpp; This file is exempt from the style guide.
JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:21:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:40:  toString_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:42:  copyConstructor_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:44:  assignOperator_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:47:  constructors_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Ignoring JavaScriptCore/qt/api/qscriptvalue.h; This file is exempt from the style guide.
Ignoring JavaScriptCore/qt/api/qscriptengine_p.h; This file is exempt from the style guide.
Ignoring JavaScriptCore/qt/api/qscriptengine.cpp; This file is exempt from the style guide.
Ignoring JavaScriptCore/qt/api/qscriptengine.h; This file is exempt from the style guide.
Ignoring JavaScriptCore/qt/api/qtscriptglobal.h; This file is exempt from the style guide.
Ignoring JavaScriptCore/qt/api/qscriptengine_p.cpp; This file is exempt from the style guide.
Total errors found: 7


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 47112 [details]
First steps of QtScript v6


> +++ b/JavaScriptCore/qt/api/QtScript.pro
> @@ -0,0 +1,37 @@
> +TARGET     = QtJavaScript

I think that should be QtScript.


> +++ b/JavaScriptCore/qt/api/qscriptengine_p.h
> @@ -0,0 +1,99 @@
> +/*
> +    Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)
> +
> +        This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public

^^^ Looks like a stray indentation here :)

> +class QScriptEnginePrivate
> +        : public QSharedData {

I think it's more common in WebKit to have the ": public QSharedData {" on the previous line.

> +public:
> +    static QScriptEnginePtr get(const QScriptEngine* q) { return q ? q->d_ptr : QScriptEnginePtr(); }

Can it happen that get() is called with a null q pointer?

> +    static QScriptEngine* get(const QScriptEnginePrivate* d) { return d ? d->q_ptr : 0; }

Same question here. (Just wondering if the extra branch is necessary)

> +void QScriptEnginePrivate::collectGarbage()
> +{
> +    JSGarbageCollect(context());

I think this function and the other helper functions below could use m_context directly.


> +class QScriptValuePrivate
> +    : public QSharedData {

Same style comment as earlier (new newline before ": public")


> +QScriptValuePrivate::QScriptValuePrivate(QString string)

I suggest to pass the QString by const reference here.


> +bool QScriptValuePrivate::isBool()
> +bool QScriptValuePrivate::isNumber()
> +bool QScriptValuePrivate::isNull()
> +bool QScriptValuePrivate::isString()
> +bool QScriptValuePrivate::isUndefined()
> +bool QScriptValuePrivate::isError()
> +bool QScriptValuePrivate::isObject()
> +bool QScriptValuePrivate::isFunction()
> +QString QScriptValuePrivate::toString()

Shouldn't these functions be const?


> +bool QScriptValuePrivate::assignEngine(QScriptEnginePrivate* engine)
> +{
[...]
> +    m_engine = const_cast<QScriptEnginePrivate*>(engine);

Why is the const_cast needed here?

> +QScriptValuePrivate* QScriptValuePrivate::call(const QScriptValuePrivate* , const QScriptValueList &args)

Looks like a stray space before the comma here :)



> +void tst_QScriptValue::toString_data()
> +{
> +    QTest::addColumn<QString>("code");
> +    QTest::addColumn<QString>("result");
> +
> +    QTest::newRow("string") << "'hello'" << "hello";
> +    QTest::newRow("string utf") << "'ÄÅÄżźóÅÅÄ'" << "ÄÅÄżźóÅÅÄ";

If this is a UTF-8 encoded string, then you have to construct it using QString::fromUtf8.

The implicit QString(const char*) constructor works most of the time fine on Linux only because nowadays the locale encoding is usually utf-8.

r- until the above comments are addressed. The rest looks good to me!

[Jędrzej Nowacki]

Created attachment 47397 [details]
First steps of QtScript v7

(In reply to comment #22)
> (From update of attachment 47112 [details])
> > +++ b/JavaScriptCore/qt/api/QtScript.pro
> > @@ -0,0 +1,37 @@
> > +TARGET     = QtJavaScript
> 
> I think that should be QtScript.
Fixed.

> > +        This library is free software; you can redistribute it and/or
> > +    modify it under the terms of the GNU Library General Public
> 
> ^^^ Looks like a stray indentation here :)
Fixed.
 
> > +class QScriptEnginePrivate
> > +        : public QSharedData {
Massive fix :-)

> > +public:
> > +    static QScriptEnginePtr get(const QScriptEngine* q) { return q ? q->d_ptr : QScriptEnginePtr(); }
> 
> Can it happen that get() is called with a null q pointer?
> 
> > +    static QScriptEngine* get(const QScriptEnginePrivate* d) { return d ? d->q_ptr : 0; }
> 
> Same question here. (Just wondering if the extra branch is necessary)
It was possible, but I changed QScriptValue::engine and all bounded constructors of the QScriptValuePrivate. So corrected.

 
> > +void QScriptEnginePrivate::collectGarbage()
> > +{
> > +    JSGarbageCollect(context());
> 
> I think this function and the other helper functions below could use m_context
> directly.
Corrected.
 
> > +QScriptValuePrivate::QScriptValuePrivate(QString string)
> 
> I suggest to pass the QString by const reference here.
Done.

> > +bool QScriptValuePrivate::isBool()
> > +bool QScriptValuePrivate::isNumber()
> > +bool QScriptValuePrivate::isNull()
> > +bool QScriptValuePrivate::isString()
> > +bool QScriptValuePrivate::isUndefined()
> > +bool QScriptValuePrivate::isError()
> > +bool QScriptValuePrivate::isObject()
> > +bool QScriptValuePrivate::isFunction()
> > +QString QScriptValuePrivate::toString()
> 
> Shouldn't these functions be const?
Changed in toString(), others can change internal state of the QScriptValuePrivate.
 
> > +bool QScriptValuePrivate::assignEngine(QScriptEnginePrivate* engine)
> > +{
> [...]
> > +    m_engine = const_cast<QScriptEnginePrivate*>(engine);
> 
> Why is the const_cast needed here?
Fixed.
 
> > +QScriptValuePrivate* QScriptValuePrivate::call(const QScriptValuePrivate* , const QScriptValueList &args)
> 
> Looks like a stray space before the comma here :)
Fixed.

> > +void tst_QScriptValue::toString_data()
> > +{
> > +    QTest::addColumn<QString>("code");
> > +    QTest::addColumn<QString>("result");
> > +
> > +    QTest::newRow("string") << "'hello'" << "hello";
> > +    QTest::newRow("string utf") << "'ąśćżźółńę'" << "ąśćżźółńę";
> 
> If this is a UTF-8 encoded string, then you have to construct it using
> QString::fromUtf8.
Fixed.

[WebKit Review Bot]

Attachment 47397 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring "JavaScriptCore/qt/api/qscriptconverter_p.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptvalue_p.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptvalue.cpp": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/QtScript.pro": this file is exempt from the style guide.
JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:21:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:35:  toString_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:37:  copyConstructor_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:39:  assignOperator_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/qt/tests/qscriptvalue/tst_qscriptvalue.cpp:42:  constructors_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Ignoring "JavaScriptCore/qt/api/qscriptvalue.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptengine_p.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptengine.cpp": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptengine.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qtscriptglobal.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptengine_p.cpp": this file is exempt from the style guide.
Total errors found: 7


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 47397 [details]
First steps of QtScript v7

r=me

[WebKit Commit Bot]

Comment on attachment 47397 [details]
First steps of QtScript v7

Clearing flags on attachment: 47397

Committed r53850: <http://trac.webkit.org/changeset/53850>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.