| Summary: | [Qt] Make conversion from QString to WTF::String (and back again) ~free | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tor Arne Vestbø <vestbo> | ||||||||||||
| Component: | New Bugs | Assignee: | Tor Arne Vestbø <vestbo> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | barraclough, cmarcelo, darin, hausmann, zalan | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Tor Arne Vestbø
2012-05-30 06:33:28 PDT
Created attachment 144794 [details]
Patch
Created attachment 144800 [details]
Patch
Created attachment 144803 [details]
Patch
Created attachment 144808 [details]
Patch
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.
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? 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. (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. (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. Comment on attachment 144808 [details] Patch Attachment 144808 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12842890 Created attachment 145089 [details] Patch Rebased to ToT, HAVE(QT5) landed in r119098 Darin, Gavin, what do you guys think? 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.
Committed r119217: <http://trac.webkit.org/changeset/119217> |