[webkit-reviews] review requested: [Bug 34843] [Qt] QtScript should provide QScriptString : [Attachment 48803] Fix v2

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
(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.