Bug 74727 – [Qt] Mobile theme refinements

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

[Pierre Rossi]

[Qt] Mobile theme refinements

[Pierre Rossi]

Created attachment 119629 [details]
Patch

As Kenneth rightfully pointed out, we should probably try to use the squircle shape for most of the controls in the mobile theme.

[Alexis Menard (darktears)]

Comment on attachment 119629 [details]
Patch

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

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:326
> +                      + (focused ? QLatin1String("-focused") : QString());

Usually string manipulation in the painting routine is a bad idea. It was one of the bottleneck in QGraphicsView back then.

I recommend to try using the Key API of QPixmapCache (that is way faster). http://developer.qt.nokia.com/doc/qt-4.8/qpixmapcache.html

This benchmark shows the improvements and implement a complicated use case such as pixmaps caching for our styles (and it is way faster). I'm sure we can use it here in a way.

http://qt.gitorious.org/qt/qt/blobs/4.7/tests/benchmarks/gui/image/qpixmapcache/tst_qpixmapcache.cpp

[Kenneth Rohde Christiansen]

(In reply to comment #2)
> (From update of attachment 119629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119629&action=review
> 
> > Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:326
> > +                      + (focused ? QLatin1String("-focused") : QString());
> 
> Usually string manipulation in the painting routine is a bad idea. It was one of the bottleneck in QGraphicsView back then.
> 
> I recommend to try using the Key API of QPixmapCache (that is way faster). http://developer.qt.nokia.com/doc/qt-4.8/qpixmapcache.html
> 
> This benchmark shows the improvements and implement a complicated use case such as pixmaps caching for our styles (and it is way faster). I'm sure we can use it here in a way.
> 
> http://qt.gitorious.org/qt/qt/blobs/4.7/tests/benchmarks/gui/image/qpixmapcache/tst_qpixmapcache.cpp

I agree, but that is a separate patch - this is the way it was done by Luiz way back.

[Kenneth Rohde Christiansen]

Comment on attachment 119629 [details]
Patch

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

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:99
> +    // Translating the painter has a tendency to mess with gradients.

The comment is not clear. So you do thing differently here because of that, or something that should be fixed later? I think the comment could make that more clear

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:129
> +        return 0;

Can't a 0 mess things up? or is that considered invalid?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:271
> +    QPolygon upArrow, downArrow;
> +    upArrow << QPoint(0, arrowHeight) << QPoint(arrowHeight, 0) << QPoint(right, arrowHeight);
> +    downArrow << QPoint(0, bottomBaseline) << QPoint(arrowHeight, bottomBaseline + arrowHeight)
> +              << QPoint(right, bottomBaseline);

Is this more effective? I think you can mention such changes in the changelog

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:680
> +        }
> +
> +    } else

unneeded newline

> Source/WebCore/platform/qt/RenderThemeQtMobile.h:113
> +    QPixmap findLineEdit(const QSize&, bool focused) const;

Maybe we should keep the names closer to the html form counterparts?

[Pierre Rossi]

Created attachment 120024 [details]
Patch

[WebKit Review Bot]

Comment on attachment 120024 [details]
Patch

Clearing flags on attachment: 120024

Committed r103404: <http://trac.webkit.org/changeset/103404>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.