Bug 33035 – [Qt] RenderThemeQt::applyTheme is a misnomer and is suboptimally coded.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carol Szabo]

RenderThemeQt::applyTheme, does not apply any Theme settings to the CSS Style of the control as it might be interpreted, on the contrary, the larger part of it initializes values in the QStyleOption used to paint controls from the CSS Style settings in WebKit and the status of the control to be painted.
Also, the checks for null style are inconsistent. calls to renderer->style() and RendererStyle->controlPart() are repeated needlessly.
Other checks for null pointers are missing.

[Carol Szabo]

Created attachment 45630 [details]
Proposed Patch

This patch fixes the issues mentioned in this bug and also fixes some style issues detected by check-webkit-style (header inclusion order and header guard name).

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45630 [details] without any errors.

[Carol Szabo]

Created attachment 45631 [details]
Proposed Patch

Minor fixes in ChangeLog comments, also inlined extracted function since it is used from only one place for readability only.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45631 [details] without any errors.

[Kenneth Rohde Christiansen]

Comment on attachment 45631 [details]
Proposed Patch

The patch basically looks fine, but would be better if you split it up, as it is hard to read.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 52638)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,28 @@
> [phone]  Carol Szabo  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +         [Qt] RenderThemeQt::applyTheme is a misnomer and is suboptimally coded.
> +        https://bugs.webkit.org/show_bug.cgi?id=33035

Misses space after this

> +        This patch:
> +        - renames RenderThemeQt::applyTheme to initializeCommonQStyleOptions,
> +        - extracts the palette initialization code to a separate function in order to
> +        provide for readable pointer checking and moves this code up in the function to
> +        allow for future changes to the palette brushes needed for bug 30173,
> +        - optimizes some of the code in the function for readability, speed and size.
> +        - fixes some minor style issues
> +
> +        No new tests because code behavior is not changed.
> +
> +        * platform/qt/RenderThemeQt.cpp:
> +        (WebCore::RenderThemeQt::paintButton):
> +        (WebCore::RenderThemeQt::paintTextField):
> +        (WebCore::RenderThemeQt::paintMenuList):
> +        (WebCore::RenderThemeQt::paintMenuListButton):
> +        (WebCore::initPaletteFromPageClientIfExists):
> +        (WebCore::RenderThemeQt::initializeCommonQStyleOptions):
> +        * platform/qt/RenderThemeQt.h:
> +
>  2009-12-29  Andrei Popescu  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> Index: WebCore/platform/qt/RenderThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/RenderThemeQt.cpp	(revision 52596)
> +++ WebCore/platform/qt/RenderThemeQt.cpp	(working copy)
> @@ -43,10 +43,10 @@
>  #include "HTMLNames.h"
>  #include "NotImplemented.h"
>  #include "Page.h"
> +#include "QWebPageClient.h"
>  #include "RenderBox.h"
>  #include "RenderTheme.h"
>  #include "UserAgentStyleSheets.h"
> -#include "QWebPageClient.h"
>  #include "qwebpage.h"
>  
>  #include <QApplication>
> @@ -477,7 +477,7 @@ bool RenderThemeQt::paintButton(RenderOb
>      option.rect = r;
>      option.state |= QStyle::State_Small;
>  
> -    ControlPart appearance = applyTheme(option, o);
> +    ControlPart appearance = initializeCommonQStyleOptions(option, o);
>      if (appearance == PushButtonPart || appearance == ButtonPart) {
>          option.rect = inflateButtonRect(option.rect, qStyle());
>          p.drawControl(QStyle::CE_PushButton, option);
> @@ -513,7 +513,7 @@ bool RenderThemeQt::paintTextField(Rende
>      panel.features = QStyleOptionFrameV2::None;
>  
>      // Get the correct theme data for a text field
> -    ControlPart appearance = applyTheme(panel, o);
> +    ControlPart appearance = initializeCommonQStyleOptions(panel, o);
>      if (appearance != TextFieldPart
>          && appearance != SearchFieldPart
>          && appearance != TextAreaPart
> @@ -575,7 +575,7 @@ bool RenderThemeQt::paintMenuList(Render
>      QStyleOptionComboBox opt;
>      if (p.widget)
>          opt.initFrom(p.widget);
> -    applyTheme(opt, o);
> +    initializeCommonQStyleOptions(opt, o);
>  
>      const QPoint topLeft = r.topLeft();
>      p.painter->translate(topLeft);
> @@ -615,7 +615,7 @@ bool RenderThemeQt::paintMenuListButton(
>      QStyleOptionComboBox option;
>      if (p.widget)
>          option.initFrom(p.widget);
> -    applyTheme(option, o);
> +    initializeCommonQStyleOptions(option, o);
>      option.rect = r;
>  
>      // for drawing the combo box arrow, rely only on the fallback style
> @@ -712,7 +712,25 @@ bool RenderThemeQt::supportsFocus(Contro
>      }
>  }
>  
> -ControlPart RenderThemeQt::applyTheme(QStyleOption& option, RenderObject* o) const
> +static inline void initPaletteFromPageClientIfExists(QPalette &palette, const RenderObject *o)

Maybe setPalette...

> +{
> +    // If the webview has a custom palette, use it
> +    Page* page = o->document()->page();
> +    if (!page)
> +        return;
> +    Chrome* chrome = page->chrome();
> +    if (!chrome)
> +        return;
> +    ChromeClient* chromeClient = chrome->client();
> +    if (!chromeClient)
> +        return;
> +    QWebPageClient* pageClient = chromeClient->platformPageClient();
> +    if (!pageClient)
> +        return;
> +    palette = pageClient->palette();
> +}
> +
> +ControlPart RenderThemeQt::initializeCommonQStyleOptions(QStyleOption& option, RenderObject* o) const
>  {
>      // Default bits: no focus, no mouse over
>      option.state &= ~(QStyle::State_HasFocus | QStyle::State_MouseOver);
> @@ -724,19 +742,24 @@ ControlPart RenderThemeQt::applyTheme(QS
>          // Readonly is supported on textfields.
>          option.state |= QStyle::State_ReadOnly;
>  
> -    if (supportsFocus(o->style()->appearance()) && isFocused(o)) {
> -        option.state |= QStyle::State_HasFocus;
> -        option.state |= QStyle::State_KeyboardFocusChange;
> -    }
> +    option.direction = Qt::LeftToRight;
>  
>      if (isHovered(o))
>          option.state |= QStyle::State_MouseOver;
>  
> -    option.direction = Qt::LeftToRight;
> -    if (o->style() && o->style()->direction() == WebCore::RTL)
> -        option.direction = Qt::RightToLeft;
> +    initPaletteFromPageClientIfExists(option.palette, o);
> +    RenderStyle* style = o->style();
> +    if (!style)
> +        return NoControlPart;
>  
> -    ControlPart result = o->style()->appearance();
> +    ControlPart result = style->appearance();
> +    if (supportsFocus(result) && isFocused(o)) {
> +        option.state |= QStyle::State_HasFocus;
> +        option.state |= QStyle::State_KeyboardFocusChange;
> +    }
> +
> +    if (style->direction() == WebCore::RTL)
> +        option.direction = Qt::RightToLeft;
>  
>      switch (result) {
>      case PushButtonPart:
> @@ -753,18 +776,9 @@ ControlPart RenderThemeQt::applyTheme(QS
>              option.state |= QStyle::State_Raised;
>          break;
>      }
> -    }
> -
> -    if (result == RadioPart || result == CheckboxPart)
> +    case RadioPart:
> +    case CheckboxPart:
>          option.state |= (isChecked(o) ? QStyle::State_On : QStyle::State_Off);
> -
> -    // If the owner widget has a custom palette, use it
> -    Page* page = o->document()->page();
> -    if (page) {
> -        ChromeClient* client = page->chrome()->client();
> -        QWebPageClient* pageClient = client->platformPageClient();
> -        if (pageClient)
> -            option.palette = pageClient->palette();
>      }
>  
>      return result;
> Index: WebCore/platform/qt/RenderThemeQt.h
> ===================================================================
> --- WebCore/platform/qt/RenderThemeQt.h	(revision 52596)
> +++ WebCore/platform/qt/RenderThemeQt.h	(working copy)
> @@ -19,8 +19,8 @@
>   * Boston, MA 02110-1301, USA.
>   *
>   */
> -#ifndef RenderThemeQt_H
> -#define RenderThemeQt_H
> +#ifndef RenderThemeQt_h
> +#define RenderThemeQt_h
>  
>  #include "RenderTheme.h"
>  
> @@ -132,7 +132,7 @@ private:
>  private:
>      bool supportsFocus(ControlPart) const;
>  
> -    ControlPart applyTheme(QStyleOption&, RenderObject*) const;
> +    ControlPart initializeCommonQStyleOptions(QStyleOption&, RenderObject*) const;
>  
>      void setButtonPadding(RenderStyle*) const;
>      void setPopupPadding(RenderStyle*) const;
> @@ -180,4 +180,4 @@ private:
>  
>  }
>  
> -#endif
> +#endif // RenderThemeQt_h

[Carol Szabo]

Created attachment 45665 [details]
Proposed Patch; Addressed Kenneth's concerns

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45665 [details] without any errors.

[Laszlo Gombos]

Comment on attachment 45665 [details]
Proposed Patch; Addressed Kenneth's concerns

LGTM.

[WebKit Commit Bot]

Comment on attachment 45665 [details]
Proposed Patch; Addressed Kenneth's concerns

Clearing flags on attachment: 45665

Committed r52664: <http://trac.webkit.org/changeset/52664>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.