QtScript API should contain a function for checking a code syntax (http://doc.trolltech.com/4.6/qscriptengine.html#checkSyntax).
Created attachment 50714 [details] Fix v1
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?
(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 :-)
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.
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.
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?
(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 ?
> 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."
(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 :)
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
Aghr, I have forgotten about 36008. I should be an easy fix.
Created attachment 51281 [details] Fix v3 Patch was updated, it should apply clearly now.
Comment on attachment 51281 [details] Fix v3 Clearing flags on attachment: 51281 Committed r56333: <http://trac.webkit.org/changeset/56333>
All reviewed patches have been landed. Closing bug.