[webkit-reviews] review granted: [Bug 67773] [Qt] Remove the QStyle dependency in Qt's mobile theme : [Attachment 114691] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted Pierre Rossi
<pierre.rossi at gmail.com>'s request for review:
Bug 67773: [Qt] Remove the QStyle dependency in Qt's mobile theme
https://bugs.webkit.org/show_bug.cgi?id=67773

Attachment 114691: Patch
https://bugs.webkit.org/attachment.cgi?id=114691&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114691&action=review


r=me with the proposed changes.

> Source/WebCore/Target.pri:-25
> -contains(DEFINES, WTF_USE_QT_MOBILE_THEME=1) {
> -    DEFINES += ENABLE_NO_LISTBOX_RENDERING=1
> -}

IT looks like that we may be able to replace the NO_LISTBOX_RENDERING feature
define with PLATFORM(QT) if we want to.

> Source/WebCore/html/HTMLSelectElement.h:208
> +#if PLATFORM(QT)
> +    if (RenderThemeQt::useMobileTheme())
> +#endif
>      return true;

I think this is easier to read if the PLATFORM(QT) branch had its own return
statement.

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:400
> +#else
> +	   qputenv("QT_WEBKIT_USE_MOBILE_THEME", QByteArray("1"));
> +#endif

This should be removed, useMobileTheme() should return true if we don't have
QStyle.

> Source/WebCore/platform/qt/RenderThemeQtMobile.cpp:570
> +void RenderThemeQtMobile::adjustMenuListStyle(CSSStyleSelector*,
RenderStyle* style, Element*) const
> +{
> +    style->resetBorder();
> +
> +    // Height is locked to auto.
> +    style->setHeight(Length(Auto));
> +
> +    // White-space is locked to pre
> +    style->setWhiteSpace(PRE);
> +
> +    computeSizeBasedOnStyle(style);
> +
> +    // Add in the padding that we'd like to use.
> +    setPopupPadding(style);
> +    style->setPaddingLeft(Length(menuListPadding, Fixed));
> +}

It would be nicer if the QtMobileImplementation would call the base class
implementation and then do the padding adjustment. This can be fixed in a
separate patch.

> Source/WebKit/qt/Api/qwebpage.cpp:3276
> +    if (qgetenv("QT_WEBKIT_USE_MOBILE_THEME").isNull()) {

I think this should be replaced with a call to RenderThemeQt::useMobileTheme().