Bug 80128 – [Qt] Move QStyle theming code out of WebCore into WebKit1

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

[Simon Hausmann]

[Qt] Move QStyle theming code out of WebCore into WebKit1

[Simon Hausmann]

Created attachment 129846 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 129846 [details]
Patch

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

> Source/WebKit/qt/ChangeLog:9
> +        Install the QStyle factory functions in initWebCoreQt.cpp.

InitWebCoreQt.cpp right

> Source/WebCore/platform/qt/RenderThemeQt.cpp:96
> +void RenderThemeQt::setCustomTheme(QtThemeFactoryFunction factory, ScrollbarTheme *customScrollbarTheme)

* placement :-), nit though

> Source/WebCore/platform/qt/RenderThemeQt.cpp:142
> +    // If we are not using a custom theme but the "Mobile Qt" default theme, then we also need
> +    // the extra stylesheet for it.
> +    if (!themeFactory) {

I think it is time we stop calling it Mobile theme :-) but I guess that is for another time 

"When no theme factory is provided we default to using our platform independent "Mobile Qt" theme, which required the following stylesheets.

> Source/WebKit/qt/WebCoreSupport/RenderThemeQStyle.cpp:4
> + * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)

time to update? I am sure we made changes after 2008 :-)

> Source/WebKit/qt/WebCoreSupport/RenderThemeQStyle.cpp:83
> +inline static void initStyleOption(QWidget *widget, QStyleOption& option)

* placement. Now you are moving the code it might be good to just fix these things...

> Source/WebKit/qt/WebCoreSupport/RenderThemeQStyle.cpp:91
> +        /*
> +          If a widget is not directly available for rendering, we fallback to default
> +          value for an active widget.
> +         */

like you real C++ comments //

> Source/WebKit/qt/WebCoreSupport/ScrollbarThemeQStyle.h:43
> +    virtual bool paint(ScrollbarThemeClient*, GraphicsContext*, const IntRect& damageRect);

dirytRect?

[Simon Hausmann]

Comment on attachment 129846 [details]
Patch

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

>> Source/WebKit/qt/ChangeLog:9
>> +        Install the QStyle factory functions in initWebCoreQt.cpp.
> 
> InitWebCoreQt.cpp right

Oops, yes.

>> Source/WebCore/platform/qt/RenderThemeQt.cpp:96
>> +void RenderThemeQt::setCustomTheme(QtThemeFactoryFunction factory, ScrollbarTheme *customScrollbarTheme)
> 
> * placement :-), nit though

Oops, will also fix before landing.

>> Source/WebCore/platform/qt/RenderThemeQt.cpp:142
>> +    if (!themeFactory) {
> 
> I think it is time we stop calling it Mobile theme :-) but I guess that is for another time 
> 
> "When no theme factory is provided we default to using our platform independent "Mobile Qt" theme, which required the following stylesheets.

Good idea, that sentence sounds better. Will include.

>> Source/WebKit/qt/WebCoreSupport/RenderThemeQStyle.cpp:4
>> + * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
> 
> time to update? I am sure we made changes after 2008 :-)

Oops, good catch!

>> Source/WebKit/qt/WebCoreSupport/RenderThemeQStyle.cpp:83
>> +inline static void initStyleOption(QWidget *widget, QStyleOption& option)
> 
> * placement. Now you are moving the code it might be good to just fix these things...

Agreed.

>> Source/WebKit/qt/WebCoreSupport/ScrollbarThemeQStyle.h:43
>> +    virtual bool paint(ScrollbarThemeClient*, GraphicsContext*, const IntRect& damageRect);
> 
> dirytRect?

Agreed.

[Simon Hausmann]

Committed r109542: <http://trac.webkit.org/changeset/109542>

[Csaba Osztrogonác]

Reopen, because it made almost all tests fail on Qt-WK2: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r109542%20%2821021%29/results.html

(Now the test coverage is near 0%)

[Simon Hausmann]

Fixed with bug #80147