[webkit-reviews] review granted: [Bug 33035] [Qt] RenderThemeQt::applyTheme is a misnomer and is suboptimally coded. : [Attachment 45631] Proposed Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 30 01:59:24 PST 2009
Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Carol Szabo
<carol.szabo at nokia.com>'s request for review:
Bug 33035: [Qt] RenderThemeQt::applyTheme is a misnomer and is suboptimally
coded.
https://bugs.webkit.org/show_bug.cgi?id=33035
Attachment 45631: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=45631&action=review
------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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 @@
> +2009-12-29 Carol Szabo <carol.szabo at nokia.com>
> +
> + 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 <andreip at google.com>
>
> 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
More information about the webkit-reviews
mailing list