[Qt] Avoid string operations in mobile theme's caching mechanism
Created attachment 120181 [details] Patch
Comment on attachment 120181 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120181&action=review We talked on irc... so here are only a few style comments > Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:175 > +void StylePainterMobile::insertInCache(const KeyIdentifier& keyId, const QPixmap& pixmap) into? > Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:468 > + id.width =imageSize.width(); missing space > Source/WebCore/platform/qt/RenderThemeQtMobile.h:96 > + , trait1(false) > + , trait2(false) why not just 0 :-)
Comment on attachment 120181 [details] Patch I like it. It is assured to be faster than string manipulation.
Created attachment 121011 [details] Patch
Comment on attachment 121011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121011&action=review > Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:175 > + static CacheKey emptyKey; > + CacheKey key = cacheKeys.value(keyId, emptyKey); > + if (key == emptyKey) > + return false; > + const bool ret = QPixmapCache::find(key, result); > + if (!ret) > + cacheKeys.remove(keyId); > + return ret; I would prefer something along these lines: QHash<KeyIdentifier, CacheKey>::Iterator it = cacheKeys.find(keyId); if (it == cacheKeys.end()) return false; bool ret = QPixmapCache::find(*it, result); if (!ret) cacheKeys.erase(it); instead of the empty key "hack".
Created attachment 121100 [details] Patch
Comment on attachment 121100 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121100&action=review > Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:180 > + const int size = pixmap.width() * pixmap.height() * pixmap.depth() / 8192; 8192 seems magical.. comment?
(In reply to comment #7) > (From update of attachment 121100 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121100&action=review > > > Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:180 > > + const int size = pixmap.width() * pixmap.height() * pixmap.depth() / 8192; > > 8192 seems magical.. comment? It's just to convert from bits to kilobytes. Does that really call for another comment ?
Created attachment 122205 [details] Patch Made the KB conversion more explicit (hopefully).
Comment on attachment 122205 [details] Patch Clearing flags on attachment: 122205 Committed r104828: <http://trac.webkit.org/changeset/104828>
All reviewed patches have been landed. Closing bug.
Comment on attachment 122205 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122205&action=review > Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:341 > + ASSERT(innerSize > padding.toSize()); > + const QSizeF innerSize = size - padding; These lines should be changed. :) That's why it broke debug build. Fix landed in http://trac.webkit.org/changeset/104830
Build is still broken. :-/ ../../../../Source/WebCore/platform/qt/RenderThemeQtMobile.cpp: In member function ‘QPixmap WebCore::StylePainterMobile::findComboButton(const QSize&, bool, bool) const’: ../../../../Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:341: error: no match for ‘operator>’ in ‘innerSize > padding.QSizeF::toSize()’ /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qchar.h:393: note: candidates are: bool operator>(QChar, QChar) /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qbytearray.h:551: note: bool operator>(const QByteArray&, const QByteArray&) /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qbytearray.h:553: note: bool operator>(const QByteArray&, const char*) /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qbytearray.h:555: note: bool operator>(const char*, const QByteArray&) /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qstring.h:942: note: bool operator>(const char*, const QString&) /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qstring.h:955: note: bool operator>(const char*, const QLatin1String&) /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qstring.h:970: note: bool operator>(const QLatin1String&, const QLatin1String&) /usr/local/Trolltech/Qt-4.8.0/include/QtCore/qstring.h:1221: note: bool operator>(const QStringRef&, const QStringRef&) Guys, could you fix it?
I Should have closed that when landing r104834.