Bug 36123 – [Qt] QtScript functionality should be extended by syntax checking.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

QtScript API should contain a function for checking a code syntax (http://doc.trolltech.com/4.6/qscriptengine.html#checkSyntax).

[Jędrzej Nowacki]

Created attachment 50714 [details]
Fix v1

[Eric Seidel (no email)]

Comment on attachment 50714 [details]
Fix v1

I'm not sure this copyright line makes sense to me:
 2     Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

Why not just Nokia?

[Jędrzej Nowacki]

(In reply to comment #2)
> (From update of attachment 50714 [details])
> I'm not sure this copyright line makes sense to me:
>  2     Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
> 
> Why not just Nokia?

Because our Legal Department said so :-)

[Kent Hansen]

Comment on attachment 50714 [details]
Fix v1

>  /*!
> +  Checks the syntax of the given \a program. Returns a
> +  QScriptSyntaxCheckResult object that contains the result of the check.
> +*/
> +QScriptSyntaxCheckResult QScriptEngine::checkSyntax(const QString &program)
> +{
> +    QScriptEnginePrivate* engine = new QScriptEnginePrivate(0);
> +    return QScriptSyntaxCheckResultPrivate::get(engine->checkSyntax(program));
> +}

I would put a comment here saying that we construct a temporary engine because this is a static function (for compatibility with the corresponding API that's part of Qt) and the JSC C API requires a JSContextRef in order to perform syntax checking. That's a restriction we should try to get rid of. Could also mention, then, that the QScriptSyntaxCheckResult object takes ownership of the engine (=context). It needs to keep the engine around in order to do the lazy querying of the message and line number properties on the JS error object... This is not so obvious.

> diff --git a/JavaScriptCore/qt/api/qscriptsyntaxcheckresult_p.cpp b/JavaScriptCore/qt/api/qscriptsyntaxcheckresult_p.cpp
> new file mode 100644
> index 0000000..c26c324
> --- /dev/null
> +++ b/JavaScriptCore/qt/api/qscriptsyntaxcheckresult_p.cpp

I would just put the contents of this file in qscriptsyntaxcheckresult.cpp, no need for a separate _p.cpp.

The rest looks good to me.

[Jędrzej Nowacki]

Created attachment 51016 [details]
Fix v2

Default constructor of the QScriptSyntaxCheckResult was removed as it was useless.

(In reply to comment #4)
> I would put a comment here saying that we construct a temporary engine because
> this is a static function (for compatibility with the corresponding API that's
> part of Qt) and the JSC C API requires a JSContextRef in order to perform
> syntax checking. That's a restriction we should try to get rid of. Could also
> mention, then, that the QScriptSyntaxCheckResult object takes ownership of the
> engine (=context). It needs to keep the engine around in order to do the lazy
> querying of the message and line number properties on the JS error object...
> This is not so obvious.
Done.

> I would just put the contents of this file in qscriptsyntaxcheckresult.cpp, no
> need for a separate _p.cpp.
Done.

> The rest looks good to me.
Cool.

[Simon Hausmann]

Comment on attachment 51016 [details]
Fix v2


> +QScriptSyntaxCheckResult QScriptEngine::checkSyntax(const QString &program)
> +{
> +    // FIXME This is not optimal.
> +    // The JSC C API needs a context to perform a syntax check, it means that a QScriptEnginePrivate
> +    // had to be created. This function is static so we have to create QScriptEnginePrivate for each
> +    // call. We can't remove the "static" for compatibility reason, at least up to Qt5.
> +    // QScriptSyntaxCheckResultPrivate takes ownership of newly created engine. The engine will be
> +    // kept as long as it is needed for lazy evaluation of properties of
> +    // the QScriptSyntaxCheckResultPrivate.
> +    QScriptEnginePrivate* engine = new QScriptEnginePrivate(/* q_ptr */ 0);
> +    return QScriptSyntaxCheckResultPrivate::get(engine->checkSyntax(program));
> +}

Doesn't this leak the engine pointer? Shouldn't it be deleted after the check?

[Jędrzej Nowacki]

(In reply to comment #6)
> > +    // FIXME This is not optimal.
> > +    // The JSC C API needs a context to perform a syntax check, it means that a QScriptEnginePrivate
> > +    // had to be created. This function is static so we have to create QScriptEnginePrivate for each
> > +    // call. We can't remove the "static" for compatibility reason, at least up to Qt5.
> > +    // QScriptSyntaxCheckResultPrivate takes ownership of newly created engine. The engine will be
> > +    // kept as long as it is needed for lazy evaluation of properties of
> > +    // the QScriptSyntaxCheckResultPrivate.
> > +    QScriptEnginePrivate* engine = new QScriptEnginePrivate(/* q_ptr */ 0);
> > +    return QScriptSyntaxCheckResultPrivate::get(engine->checkSyntax(program));
> > +}
> 
> Doesn't this leak the engine pointer? Shouldn't it be deleted after the check?

No, the QScriptEnginePrivate inherits from the QSharedData and the QScriptSyntaxCheckResultPrivate keeps a reference to it (in the same way as a QScriptValuePrivate could). 
This question means that comment that you quoted is not clear enough. Any suggestions how to improve it ?

[Jędrzej Nowacki]

> This question means that comment that you quoted is not clear enough. Any
> suggestions how to improve it ?

What about ?

"QScriptSyntaxCheckResultPrivate takes ownership of newly created engine. The engine will be kept as long as it is needed for lazy evaluation of properties of the QScriptSyntaxCheckResultPrivate. It will be automatically deleted by the QScriptSyntaxCheckResultPrivate."

[Simon Hausmann]

(In reply to comment #8)
> > This question means that comment that you quoted is not clear enough. Any
> > suggestions how to improve it ?
> 
> What about ?
> 
> "QScriptSyntaxCheckResultPrivate takes ownership of newly created engine. The
> engine will be kept as long as it is needed for lazy evaluation of properties
> of the QScriptSyntaxCheckResultPrivate. It will be automatically deleted by the
> QScriptSyntaxCheckResultPrivate."

I overlooked the comment, sorry :)

[WebKit Commit Bot]

Comment on attachment 51016 [details]
Fix v2

Rejecting patch 51016 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Simon Hausmann', '--force']" exit_code: 1
Last 500 characters of output:
t 40 with fuzz 2.
patching file JavaScriptCore/qt/api/qscriptsyntaxcheckresult.cpp
patching file JavaScriptCore/qt/api/qscriptsyntaxcheckresult.h
patching file JavaScriptCore/qt/api/qscriptsyntaxcheckresult_p.h
patching file JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp
Hunk #1 FAILED at 18.
Hunk #2 FAILED at 38.
Hunk #3 succeeded at 178 with fuzz 1 (offset 102 lines).
2 out of 3 hunks FAILED -- saving rejects to file JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp.rej

Full output: http://webkit-commit-queue.appspot.com/results/1046090

[Jędrzej Nowacki]

Aghr, I have forgotten about 36008. I should be an easy fix.

[Jędrzej Nowacki]

Created attachment 51281 [details]
Fix v3

Patch was updated, it should apply clearly now.

[WebKit Commit Bot]

Comment on attachment 51281 [details]
Fix v3

Clearing flags on attachment: 51281

Committed r56333: <http://trac.webkit.org/changeset/56333>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.