[webkit-reviews] review denied: [Bug 29564] [Qt] Webkit Button Size : [Attachment 46689] Patch with test case

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has denied	review:
Bug 29564: [Qt] Webkit Button Size
https://bugs.webkit.org/show_bug.cgi?id=29564

Attachment 46689: Patch with test case
https://bugs.webkit.org/attachment.cgi?id=46689&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
Thanks for the updated patch Daniel!

> +#include <QMacStyle>

QMacStyle is only available on mac, so this has to be wrapped in Q_WS_MAC

>      QFont defaultButtonFont = QApplication::font(&button);
>      QFontInfo fontInfo(defaultButtonFont);
>      m_buttonFontFamily = defaultButtonFont.family();
> -#ifdef Q_WS_MAC
> -    m_buttonFontPixelSize = fontInfo.pixelSize();
> -#endif
> +    if (qobject_cast<QMacStyle*>(qStyle()))
> +	   m_buttonFontPixelSize = fontInfo.pixelSize();
>  }

We can't reference the style in the ctor like this, it's too early. You could
move the init code into adjustButtonSizes() where those two members are used
and use static variables there to cache it.

the QMacStyle check also has to be wrapped in Q_WS_MAC like the include

> +    if (qobject_cast<QMacStyle*>(qStyle()) && style->appearance() ==
PushButtonPart) {

Same

> +    if (qobject_cast<QMacStyle*>(qStyle()) {

Same


> Index: LayoutTests/fast/css/button-height-expected.txt
> ===================================================================
> --- LayoutTests/fast/css/button-height-expected.txt	(revision 0)
> +++ LayoutTests/fast/css/button-height-expected.txt	(revision 0)
> @@ -0,0 +1,13 @@

This is a cross-platform expected file, which means it must be valid for mac,
win, qt, gtk, qt-mac, etc if there are no overrides in platform/port-spesific
results.

> +PASS document.getElementById('button1').offsetHeight is
document.getElementById('button2').offsetHeight
> +PASS document.getElementById('button3').offsetHeight is 40
> +PASS document.getElementById('button4').offsetHeight is 40
> +PASS document.getElementById('button5').offsetHeight is 18

The hard-coded result of 18 here will not match for example qt, gtk, where the
button will have a height of 40.

> +    shouldEvaluateTo("document.getElementById('button5').offsetHeight", 
> +	   (navigator.platform.indexOf("Mac") !== -1) ?
document.getElementById("button1").offsetHeight : 40);

This one is tricky. For Qt we run the DRT with -style windows on all platforms,
which means that with the runtime check in RenderThemeQt we'll render the
button 40 pixel high, but navigator.platform will still return MacIntel, so
we'll expect the height to be button1.

I guess the easiest here would be to change this line into dumping the height
of the button5, and then using the expected-file to detect failure, with the
cross-platform expected file expecting 40, and then a platform-specific
expected file with 18 in the mac results.

(or a number of platform-specific expected files with 40 instead of a shared
cross-platform one) + the mac one with 18