Bug 74293 – [Qt] Mobile theme could use a little refresh

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

[Pierre Rossi]

[Qt] Mobile theme could use a little refresh

[Pierre Rossi]

Created attachment 118786 [details]
Patch

[Kenneth Rohde Christiansen]

We always love screenshots :-)

[Benjamin Poulain]

Comment on attachment 118786 [details]
Patch

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

I just went quickly over the patch. Sorry, no real review.

> Source/WebCore/platform/qt/RenderThemeQt.cpp:121
>      String result = RenderTheme::extraDefaultStyleSheet();
> -    if (useMobileTheme())
> +    if (useMobileTheme()) {
>          result += String(themeQtNoListboxesUserAgentStyleSheet, sizeof(themeQtNoListboxesUserAgentStyleSheet));
> +        result += String(mobileThemeQtUserAgentStyleSheet, sizeof(mobileThemeQtUserAgentStyleSheet));
> +    }
>      return result;

We have StringBuilder to build string effectively.

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:145
> +void StylePainterMobile::drawChecker(QPainter* painter, const QRect& rect, QColor color) const

QColor -> const QColor&

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:158
> +    path.moveTo(0.18, 0.47);
> +    path.lineTo(0.25, 0.4);
> +    path.lineTo(0.4, 0.55);
> +    path.quadTo(0.64, 0.29, 0.78, 0.2);
> +    path.lineTo(0.8, 0.25);
> +    path.quadTo(0.53, 0.55, 0.45, 0.75);

Better give name for those magic numbers :)

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:164
> +// Assumption here is that we're working with a square size

What about removing the comment and adding an ASSERT? :)

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:170
> +                  + (checked ? QLatin1String("-checked") : QLatin1String(""));

QLatin1String("") ? Not QLatin1String()?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:171
> +    if (true || !QPixmapCache::find(key, result)) {

true || ?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:184
> +// Assumption here is that we're working with a square size

ASSERT? :)

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:190
> +    const int border = size.width()/ 4;

Coding style: " / " :)

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:213
> +//    painter->fillRect(QRect(QPoint(0,0), size), Qt::red);

Uh?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:214
> +    const int height = size.height() - 1;

Probably "side" instead of "height" or something like that?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:406
> +    // TODO: cache ?

We use FIXME.

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:631
> +    // our animation goes back and forth so we need to make it last twice as long

our -> Our

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:632
> +    // and we need the numerator to be an odd number in order to ensure we get a progress value of 0.5

PERIOD at the end of the sentence!!!!!
You are lucky Kenneth did not see the patch yet ;)

[Kenneth Rohde Christiansen]

Comment on attachment 118786 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        The look and feel of the "mobile theme" we're
> +        using in QtWebKit dates back to the maemo 5 days.

Not entirely true :-) It was way uglier in the Maemo 5 days, and it got a mini update for Grob.

> Source/WebCore/css/mobileThemeQt.css:30
> +input[type="submit"], select{
> +  font-family: Arial, sans-serif;
> +}

What about using Nokia font if available?

> Source/WebCore/css/mobileThemeQt.css:32
> +input[type="submit"]:disabled, input[type="submit"]:disabled:active, select:disabled, input[type="text"]:disabled{

space before {

> Source/WebCore/platform/qt/RenderThemeQt.cpp:874
>  {
>      Q_UNUSED(theme);
> -    init(paintInfo.context ? paintInfo.context : 0);
> +    if (paintInfo.context)
> +        init(paintInfo.context);
>  }

So when does this happen that we dont have a context? Assert? Comment?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:95
>      painter->drawRoundedRect(rect.adjusted(line, line, -line, -line),
> -            /* xRadius */ 5.0, /* yRadious */ 5.0);
> +            xRadius, yRadius);

Just put that on one line

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:100
> +static inline QRect squareFromRect(const QRect& rect)

expandRectToSquare ?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:116
> +    oldSmoothPixmapTransform = painter->testRenderHint(QPainter::SmoothPixmapTransform);
> +    if (!oldSmoothPixmapTransform)
> +        painter->setRenderHint(QPainter::SmoothPixmapTransform);

is old stored somewhere? or could this be two lines? what about adding a comment and also I dislike old* :-) previous sounds more clear

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:124
> +void StylePainterMobile::drawCheckableBackground(QPainter *painter, const QRect& rect, bool checked, bool enabled) const

* alignment

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:139
> +QSize StylePainterMobile::sizeForPainterScale(const QRect &rect) const

& alighment

Also the name is not very clear, but I dont have any better suggestion. but I do like At better than For

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:200
> +                  + (checked ? QLatin1String("-checked") : QLatin1String(""));

Doesn't QLatin1String() work?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:226
> +    const int width = size.width();

why do you do with this width and not height which you use multiple places

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:227
> +    const int middle = width / 2;

horizontalCenter? confusing with center below

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:228
> +    const int vCenter = size.height() / 2;

v? vertical?

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:267
> +    static const QString prefix = QLatin1String("$qt-webkit-mobile-theme-combo-");
> +    QPixmap result;
>      QString key = prefix + (multiple ? QLatin1String("multiple-") : QLatin1String("simple-"))
> -        + QString::number(imageSize.width()) + QLatin1String("-") + QString::number(imageSize.height())
> -        + QLatin1String("-") + (disabled ? QLatin1String("disabled") : QLatin1String("enabled"));
> +        + QString::number(size.width()) + QLatin1String("-") + QString::number(size.height())
> +        + QLatin1String("-") + (enabled ? QLatin1String("on") : QLatin1String("off"));

You seem to do this multiple places? maybe we need a dedicated method for doing this

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:288
> +        const int border = focusPen.width() - 1;

comment? why - 1?

> Source/WebCore/platform/qt/RenderThemeQtMobile.h:115
> +    bool oldSmoothPixmapTransform;

m_previousSmoothPixmapTransform ?

[Pierre Rossi]

Comment on attachment 118786 [details]
Patch

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

>> Source/WebCore/css/mobileThemeQt.css:30
>> +}
> 
> What about using Nokia font if available?

@Kenneth: that's a actually not a bad idea at all... ;)

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:100
>> +static inline QRect squareFromRect(const QRect& rect)
> 
> expandRectToSquare ?

Actually it shrinks, so I'd probably go with shrinkRectToSquare. Your comment makes it clear that the name is not clear enough :)

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:116
>> +        painter->setRenderHint(QPainter::SmoothPixmapTransform);
> 
> is old stored somewhere? or could this be two lines? what about adding a comment and also I dislike old* :-) previous sounds more clear

I basically named it that to be consistent with what was already there in stylePainter (oldBrush and oldAntiAliasing).

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:171
>> +    if (true || !QPixmapCache::find(key, result)) {
> 
> true || ?

duh, I should really read things again sometimes. Remainders from testing something...

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:190
>> +    const int border = size.width()/ 4;
> 
> Coding style: " / " :)

Interesting that this wasn't caught by check-webkit-style...

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:213
>> +//    painter->fillRect(QRect(QPoint(0,0), size), Qt::red);
> 
> Uh?

yup, I should definitely have proof-read that...

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:226
>> +    const int width = size.width();
> 
> why do you do with this width and not height which you use multiple places

true. Should have done that when I changed the spacing to not be constant.

>> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:267
>> +        + QLatin1String("-") + (enabled ? QLatin1String("on") : QLatin1String("off"));
> 
> You seem to do this multiple places? maybe we need a dedicated method for doing this

While I agree there seems to be a pattern, it's done in 4 places with a different number and type of arguments. I'm just not sure it's worth over-engineering...

[Pierre Rossi]

Created attachment 119021 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119021 [details]
Patch

Clearing flags on attachment: 119021

Committed r102768: <http://trac.webkit.org/changeset/102768>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

The patch has broken the Qt-Minimal bot: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/38829/steps/compile-webkit/logs/stdio. Could you take a look?

[Pierre Rossi]

(In reply to comment #9)
> The patch has broken the Qt-Minimal bot: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/38829/steps/compile-webkit/logs/stdio. Could you take a look?

Fixed in r102773 :)

[Balazs Kelemen]

(In reply to comment #10)
> (In reply to comment #9)
> > The patch has broken the Qt-Minimal bot: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/38829/steps/compile-webkit/logs/stdio. Could you take a look?
> 
> Fixed in r102773 :)

Thanks!