Bug 87847 – [Qt] Make conversion from QString to WTF::String (and back again) ~free

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tor Arne Vestbø]

[Qt] Make conversion from QString to WTF::String (and back again) ~free

[Tor Arne Vestbø]

Created attachment 144794 [details]
Patch

[Tor Arne Vestbø]

Created attachment 144800 [details]
Patch

[Tor Arne Vestbø]

Created attachment 144803 [details]
Patch

[Tor Arne Vestbø]

Created attachment 144808 [details]
Patch

[Simon Hausmann]

Comment on attachment 144808 [details]
Patch

I think this looks good from a Qt perspective. I think we should run the idea also past some other String folks, to see if they have any concerns with doing this Qt specific optimization.

[zalan]

Comment on attachment 144808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144808&action=review

> w/Source/WTF/wtf/text/StringImpl.h:406
> +    QStringData* qStringData() { return bufferOwnership() == BufferAdoptedQString ? m_qStringData : 0; }

shouldnt m_qStringData be NULL anyway when it's not adoptedQString?

[Simon Hausmann]

Comment on attachment 144808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144808&action=review

>> w/Source/WTF/wtf/text/StringImpl.h:406
>> +    QStringData* qStringData() { return bufferOwnership() == BufferAdoptedQString ? m_qStringData : 0; }
> 
> shouldnt m_qStringData be NULL anyway when it's not adoptedQString?

It could have a different value since it's part of a union.

[zalan]

(In reply to comment #7)
> (From update of attachment 144808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144808&action=review
> 
> >> w/Source/WTF/wtf/text/StringImpl.h:406
> >> +    QStringData* qStringData() { return bufferOwnership() == BufferAdoptedQString ? m_qStringData : 0; }
> > 
> > shouldnt m_qStringData be NULL anyway when it's not adoptedQString?
> 
> It could have a different value since it's part of a union.

oh right. that was cut off of the review context. should have looked at the original code.

[Tor Arne Vestbø]

(In reply to comment #5)
> (From update of attachment 144808 [details])
> I think this looks good from a Qt perspective. I think we should run the idea also past some other String folks, to see if they have any concerns with doing this Qt specific optimization.

Come to think of it, if more ports would like this we can always make the ownership BufferAdoptedPlatformString instead.

[Early Warning System Bot]

Comment on attachment 144808 [details]
Patch

Attachment 144808 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12842890

[Tor Arne Vestbø]

Created attachment 145089 [details]
Patch

Rebased to ToT, HAVE(QT5) landed in r119098

[Tor Arne Vestbø]

Darin, Gavin, what do you guys think?

[Gavin Barraclough]

Comment on attachment 145089 [details]
Patch

This seems fine to me.  We've discussed refactoring the various backing buffers behind an extra layer of abstraction at some point, with a virtual interface so we can move knowledge of the set of supported types out of backing buffers out of StringImpl.  But if we do want to make a change like that, we could easily make QStringData one of the types of supported buffers.

[Tor Arne Vestbø]

Committed r119217: <http://trac.webkit.org/changeset/119217>