Bug 36123 - [Qt] QtScript functionality should be extended by syntax checking.
Summary: [Qt] QtScript functionality should be extended by syntax checking.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords: Qt
Depends on:
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-03-15 09:29 PDT by Jędrzej Nowacki
Modified: 2010-03-22 07:38 PDT (History)
4 users (show)

See Also:


Attachments
Fix v1 (20.48 KB, patch)
2010-03-15 09:44 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v2 (21.17 KB, patch)
2010-03-18 03:05 PDT, Jędrzej Nowacki
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fix v3 (21.33 KB, patch)
2010-03-22 06:46 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 2010-03-15 09:29:50 PDT
QtScript API should contain a function for checking a code syntax (http://doc.trolltech.com/4.6/qscriptengine.html#checkSyntax).
Comment 1 Jędrzej Nowacki 2010-03-15 09:44:32 PDT
Created attachment 50714 [details]
Fix v1
Comment 2 Eric Seidel (no email) 2010-03-15 15:13:48 PDT
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?
Comment 3 Jędrzej Nowacki 2010-03-17 05:18:16 PDT
(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 4 Kent Hansen 2010-03-17 07:02:50 PDT
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.
Comment 5 Jędrzej Nowacki 2010-03-18 03:05:40 PDT
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 6 Simon Hausmann 2010-03-18 03:18:15 PDT
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?
Comment 7 Jędrzej Nowacki 2010-03-18 03:43:52 PDT
(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 ?
Comment 8 Jędrzej Nowacki 2010-03-18 03:48:28 PDT
> 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."
Comment 9 Simon Hausmann 2010-03-21 15:15:09 PDT
(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 10 WebKit Commit Bot 2010-03-21 16:02:28 PDT
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
Comment 11 Jędrzej Nowacki 2010-03-22 02:51:06 PDT
Aghr, I have forgotten about 36008. I should be an easy fix.
Comment 12 Jędrzej Nowacki 2010-03-22 06:46:17 PDT
Created attachment 51281 [details]
Fix v3

Patch was updated, it should apply clearly now.
Comment 13 WebKit Commit Bot 2010-03-22 07:38:00 PDT
Comment on attachment 51281 [details]
Fix v3

Clearing flags on attachment: 51281

Committed r56333: <http://trac.webkit.org/changeset/56333>
Comment 14 WebKit Commit Bot 2010-03-22 07:38:05 PDT
All reviewed patches have been landed.  Closing bug.