Bug 34843

Summary:

[Qt] QtScript should provide QScriptString

Product:

WebKit

Reporter:

Jędrzej Nowacki <jedrzej.nowacki>

Component:

JavaScriptCore

Assignee:

Jędrzej Nowacki <jedrzej.nowacki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ariya.hidayat, commit-queue, jedrzej.nowacki, webkit.review.bot

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://qt.nokia.com/doc/4.6/qscriptstring.html

Bug Depends on:

Bug Blocks:

31863, 34850

Attachments:

Description	Flags
Fix v1	hausmann: review-, hausmann: commit-queue-
Fix v2	none
Fix v3	none
Fix v4	none

Jędrzej Nowacki

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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jędrzej Nowacki

Comment 1 2010-02-11 08:11:40 PST

Created attachment 48563 [details] Fix v1

WebKit Review Bot

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

Simon Hausmann

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

Jędrzej Nowacki

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

WebKit Review Bot

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

Jędrzej Nowacki

Comment 6 2010-02-19 04:13:56 PST

Created attachment 49065 [details] Fix v3 Export macro was added. toArrayIndex() caching was removed.

WebKit Review Bot

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

Ariya Hidayat

Comment 8 2010-02-22 19:39:35 PST

Jedrzej, shouldn't you fix the style error?

Jędrzej Nowacki

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

Jędrzej Nowacki

Comment 10 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).

WebKit Review Bot

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

WebKit Commit Bot

Comment 12 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>

WebKit Commit Bot

Comment 13 2010-03-02 13:20:33 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.