Bug 75010 - [Qt] Avoid string operations in mobile theme's caching mechanism
Summary: [Qt] Avoid string operations in mobile theme's caching mechanism
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pierre Rossi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-21 08:38 PST by Pierre Rossi
Modified: 2012-03-02 07:37 PST (History)
5 users (show)

See Also:


Attachments
Patch (14.42 KB, patch)
2011-12-21 08:49 PST, Pierre Rossi
no flags Details | Formatted Diff | Diff
Patch (21.09 KB, patch)
2012-01-03 15:53 PST, Pierre Rossi
no flags Details | Formatted Diff | Diff
Patch (21.07 KB, patch)
2012-01-04 04:42 PST, Pierre Rossi
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2012-01-12 03:31 PST, Pierre Rossi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Rossi 2011-12-21 08:38:29 PST
[Qt] Avoid string operations in mobile theme's caching mechanism
Comment 1 Pierre Rossi 2011-12-21 08:49:11 PST
Created attachment 120181 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2011-12-21 09:02:47 PST
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 3 Alexis Menard (darktears) 2012-01-02 12:51:07 PST
Comment on attachment 120181 [details]
Patch

I like it. It is assured to be faster than string manipulation.
Comment 4 Pierre Rossi 2012-01-03 15:53:46 PST
Created attachment 121011 [details]
Patch
Comment 5 Simon Hausmann 2012-01-04 00:49:48 PST
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".
Comment 6 Pierre Rossi 2012-01-04 04:42:48 PST
Created attachment 121100 [details]
Patch
Comment 7 Kenneth Rohde Christiansen 2012-01-05 08:45:01 PST
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?
Comment 8 Pierre Rossi 2012-01-09 07:14:57 PST
(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 ?
Comment 9 Pierre Rossi 2012-01-12 03:31:45 PST
Created attachment 122205 [details]
Patch

Made the KB conversion  more explicit (hopefully).
Comment 10 WebKit Review Bot 2012-01-12 08:33:29 PST
Comment on attachment 122205 [details]
Patch

Clearing flags on attachment: 122205

Committed r104828: <http://trac.webkit.org/changeset/104828>
Comment 11 WebKit Review Bot 2012-01-12 08:33:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Csaba Osztrogonác 2012-01-12 08:43:09 PST
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
Comment 13 Csaba Osztrogonác 2012-01-12 08:50:13 PST
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?
Comment 14 Pierre Rossi 2012-03-02 07:37:52 PST
I Should have closed that when landing r104834.