[webkit-reviews] review denied: [Bug 34843] [Qt] QtScript should provide QScriptString : [Attachment 48563] Fix v1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 15 04:14:49 PST 2010


Simon Hausmann <hausmann at webkit.org> has denied Jędrzej Nowacki
<jedrzej.nowacki at nokia.com>'s request for review:
Bug 34843: [Qt] QtScript should provide QScriptString
https://bugs.webkit.org/show_bug.cgi?id=34843

Attachment 48563: Fix v1
https://bugs.webkit.org/attachment.cgi?id=48563&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

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


More information about the webkit-reviews mailing list