Bug 80128

Summary: [Qt] Move QStyle theming code out of WebCore into WebKit1
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: New BugsAssignee: Simon Hausmann <hausmann>
Status: RESOLVED FIXED    
Severity: Normal CC: kenneth, ossy, pierre.rossi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80147    
Bug Blocks: 78109    
Attachments:
Description Flags
Patch kenneth: review+

Description Simon Hausmann 2012-03-02 01:51:28 PST
[Qt] Move QStyle theming code out of WebCore into WebKit1
Comment 1 Simon Hausmann 2012-03-02 01:54:02 PST
Created attachment 129846 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-03-02 02:15:54 PST
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?
Comment 3 Simon Hausmann 2012-03-02 02:42:02 PST
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.
Comment 4 Simon Hausmann 2012-03-02 02:43:51 PST
Committed r109542: <http://trac.webkit.org/changeset/109542>
Comment 5 Csaba Osztrogon√°c 2012-03-02 04:16:31 PST
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%)
Comment 6 Simon Hausmann 2012-03-02 04:45:50 PST
Fixed with bug #80147