Bug 75010 – [Qt] Avoid string operations in mobile theme's caching mechanism

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

[Pierre Rossi]

[Qt] Avoid string operations in mobile theme's caching mechanism

[Pierre Rossi]

Created attachment 120181 [details]
Patch

[Kenneth Rohde Christiansen]

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 :-)

[Alexis Menard (darktears)]

Comment on attachment 120181 [details]
Patch

I like it. It is assured to be faster than string manipulation.

[Pierre Rossi]

Created attachment 121011 [details]
Patch

[Simon Hausmann]

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

[Pierre Rossi]

Created attachment 121100 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Pierre Rossi]

(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 ?

[Pierre Rossi]

Created attachment 122205 [details]
Patch

Made the KB conversion  more explicit (hopefully).

[WebKit Review Bot]

Comment on attachment 122205 [details]
Patch

Clearing flags on attachment: 122205

Committed r104828: <http://trac.webkit.org/changeset/104828>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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?

[Pierre Rossi]

I Should have closed that when landing r104834.