Bug 34843 - [Qt] QtScript should provide QScriptString
Summary: [Qt] QtScript should provide QScriptString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Jędrzej Nowacki
URL: http://qt.nokia.com/doc/4.6/qscriptst...
Keywords: Qt
Depends on:
Blocks: 31863 34850
  Show dependency treegraph
 
Reported: 2010-02-11 08:08 PST by Jędrzej Nowacki
Modified: 2010-03-02 13:20 PST (History)
4 users (show)

See Also:


Attachments
Fix v1 (20.63 KB, patch)
2010-02-11 08:11 PST, Jędrzej Nowacki
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
Fix v2 (21.84 KB, patch)
2010-02-16 04:02 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v3 (21.74 KB, patch)
2010-02-19 04:13 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v4 (22.05 KB, patch)
2010-02-23 06:29 PST, 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-02-11 08:08:36 PST
The QtScript should provide a class which could be used as a handler to "interned" strings in a QScriptEngine.
Comment 1 Jędrzej Nowacki 2010-02-11 08:11:40 PST
Created attachment 48563 [details]
Fix v1
Comment 2 WebKit Review Bot 2010-02-11 08:14:43 PST
Attachment 48563 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring "JavaScriptCore/qt/api/qscriptstring.h": 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/qscriptstring/tst_qscriptstring.cpp:23:  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/qscriptstring/tst_qscriptstring.cpp:24:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
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/qscriptstring_p.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide.
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Hausmann 2010-02-15 04:14:50 PST
Comment on attachment 48563 [details]
Fix v1


> +bool QScriptString::operator==(const QScriptString& other) const
> +{
> +    return d_ptr == other.d_ptr ? true : *d_ptr == *(other.d_ptr);

I personally find it easier to read the following:

    return d_ptr == other.d_ptr || *d_ptr == *other.d_ptr;


> +class QScriptStringPrivate : public QSharedData {
> +public:
> +    inline QScriptStringPrivate();
> +    inline QScriptStringPrivate(const QString& qtstring);
> +    inline ~QScriptStringPrivate();
> +
> +    static inline QScriptString get(QScriptStringPrivate* d);
> +    static inline QScriptStringPtr get(const QScriptString& p);
> +
> +    inline bool isValid() const;
> +
> +    inline bool operator==(const QScriptStringPrivate& other) const;
> +    inline bool operator!=(const QScriptStringPrivate& other) const;
> +
> +    inline quint32 toArrayIndex(bool* ok = 0) const;
> +
> +    inline QString toString() const;
> +
> +    inline quint64 id() const;
> +
> +private:
> +    JSStringRef m_jsstring;
> +    QString m_qtstring;
> +};

I don't think we should store a QString here.

If the goal is to speed up toArrayIndex(), then I suggest to either add the functionality to the C API or by implementing it on top without the expensive conversion by for example using JSStringGetCharactersPtr.
Comment 4 Jędrzej Nowacki 2010-02-16 04:02:54 PST
Created attachment 48803 [details]
Fix v2

(In reply to comment #3)
> (From update of attachment 48563 [details])
> 
> > +bool QScriptString::operator==(const QScriptString& other) const
> > +{
> > +    return d_ptr == other.d_ptr ? true : *d_ptr == *(other.d_ptr);
> 
> I personally find it easier to read the following:
> 
>     return d_ptr == other.d_ptr || *d_ptr == *other.d_ptr;
Fixed (in both; == and != operators).

> > +class QScriptStringPrivate : public QSharedData {
> > (...)
> > +    JSStringRef m_jsstring;
> > +    QString m_qtstring;
> > +};
> 
> I don't think we should store a QString here.
> 
> If the goal is to speed up toArrayIndex(), then I suggest to either add the
> functionality to the C API or by implementing it on top without the expensive
> conversion by for example using JSStringGetCharactersPtr.
I replaced the QString by a quint32. In this implementation conversion is needed only once. It is not possible to avoid it without JSC C API changes.
Comment 5 WebKit Review Bot 2010-02-16 04:07:17 PST
Attachment 48803 [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/qscriptstring.h": 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/qscriptstring/tst_qscriptstring.cpp:23:  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/qscriptstring/tst_qscriptstring.cpp:24:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
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/qscriptstring_p.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide.
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Jędrzej Nowacki 2010-02-19 04:13:56 PST
Created attachment 49065 [details]
Fix v3

Export macro was added.
toArrayIndex() caching was removed.
Comment 7 WebKit Review Bot 2010-02-19 04:18:46 PST
Attachment 49065 [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/qscriptstring.h": 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/qscriptstring/tst_qscriptstring.cpp:23:  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/qscriptstring/tst_qscriptstring.cpp:24:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
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/qscriptstring_p.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide.
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Ariya Hidayat 2010-02-22 19:39:35 PST
Jedrzej, shouldn't you fix the style error?
Comment 9 Jędrzej Nowacki 2010-02-23 00:42:11 PST
(In reply to comment #8)
> Jedrzej, shouldn't you fix the style error?

I believe that they are false positives. I registered two bugs; 35143 and 35151.
Comment 10 Jędrzej Nowacki 2010-02-23 06:29:19 PST
Created attachment 49286 [details]
Fix v4

Changelog:
Algorithm was simplified.
Code was moved to the QScriptConverter class (there is high probability that it will be reused from QScriptValue).
New test cases were added.
Fix conversion from float (ex. 0.3 is not a correct index array).
Comment 11 WebKit Review Bot 2010-02-23 06:31:24 PST
Attachment 49286 [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/qscriptstring.h": 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/qscriptstring/tst_qscriptstring.cpp:23:  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/qscriptstring/tst_qscriptstring.cpp:24:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
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/qscriptstring_p.h": this file is exempt from the style guide.
Ignoring "JavaScriptCore/qt/api/qscriptstring.cpp": this file is exempt from the style guide.
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 WebKit Commit Bot 2010-03-02 13:20:28 PST
Comment on attachment 49286 [details]
Fix v4

Clearing flags on attachment: 49286

Committed r55426: <http://trac.webkit.org/changeset/55426>
Comment 13 WebKit Commit Bot 2010-03-02 13:20:33 PST
All reviewed patches have been landed.  Closing bug.