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.
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).
style-queue ran check-webkit-style on attachment 45630 [details] without any errors.
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.
style-queue ran check-webkit-style on attachment 45631 [details] without any errors.
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 @@ > +2009-12-29 Carol Szabo <carol.szabo@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@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
Created attachment 45665 [details] Proposed Patch; Addressed Kenneth's concerns
style-queue ran check-webkit-style on attachment 45665 [details] without any errors.
Comment on attachment 45665 [details] Proposed Patch; Addressed Kenneth's concerns LGTM.
Comment on attachment 45665 [details] Proposed Patch; Addressed Kenneth's concerns Clearing flags on attachment: 45665 Committed r52664: <http://trac.webkit.org/changeset/52664>
All reviewed patches have been landed. Closing bug.