Bug 46236

Created attachment 68322 [details] Patch

(In reply to comment #1) > Created an attachment (id=68322) [details] > Patch ImageBrew.cpp is already in bug 45873. Please remove the file from the patch.

Created attachment 68380 [details] Patch. Removed ImageBrew.cpp

(In reply to comment #3) > Created an attachment (id=68380) [details] > Patch. Removed ImageBrew.cpp RenderThemeBrew::paintCombo is not implemented and used only by RenderThemeBrew::paintTextArea. Can you just get rid of RenderThemeBrew::paintCombo?

Created attachment 68384 [details] Patch. Removed paintCombo()

Comment on attachment 68384 [details] Patch. Removed paintCombo() View in context: https://bugs.webkit.org/attachment.cgi?id=68384&action=review ok otherwise. > WebCore/platform/brew/RenderThemeBrew.cpp:3 > + * Copyright (C) 2009 Company 100, Inc. 2010?

Created attachment 70879 [details] patch Fix year in license.

ChangeLog is a bit misleading because this patch implements all of RenderTheme methods, not just button, checkbox, radio and menu list.

Comment on attachment 68384 [details] Patch. Removed paintCombo() Cleared Dimitri Glazkov's review+ from obsolete attachment 68384 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 70879 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=70879&action=review > WebCore/platform/brew/RenderThemeBrew.cpp:87 > + static RenderTheme* renderTheme = RenderThemeBrew::create().releaseRef(); I thought we used leakPtr here? or leakRef()? > WebCore/platform/brew/RenderThemeBrew.cpp:93 > + int i; I would just put it in the fors. > WebCore/platform/brew/RenderThemeBrew.cpp:106 > + for (i = 0; i < ButtonStates; ++i) { > + s_buttonImages[i] = Image::loadPlatformResource(s_buttonNames[i].name); > + ASSERT(!s_buttonImages[i]->isNull()); > + } > + > + for (i = 0; i < OnOffStates; ++i) { > + s_checkboxImages[i] = Image::loadPlatformResource(s_checkboxNames[i]); > + ASSERT(!s_checkboxImages[i]->isNull()); > + > + s_radioImages[i] = Image::loadPlatformResource(s_radioNames[i]); > + ASSERT(!s_radioImages[i]->isNull()); > + } Seems this whole thing should be a separate loadResources function which the constructor calls, no? > WebCore/platform/brew/RenderThemeBrew.cpp:115 > + return Color(selectionColor); is Color(RGBA32) explicit? Or can you just return selectionColor? > WebCore/platform/brew/RenderThemeBrew.cpp:120 > + return Color(Color::transparent); Don't need to construct it directly. > WebCore/platform/brew/RenderThemeBrew.cpp:135 > + return Color(Color::transparent); Same here. > WebCore/platform/brew/RenderThemeBrew.cpp:146 > + return RenderTheme::baselinePosition(obj) - 5; Constants are generally better with descriptive variable names. > WebCore/platform/brew/RenderThemeBrew.cpp:233 > + ButtonState state; > + if (isEnabled(obj)) { > + if (isPressed(obj)) > + state = ButtonPressed; > + else if (isFocused(obj)) > + state = ButtonFocused; > + else > + state = ButtonNormal; > + } else > + state = ButtonDisabled; This feels like a helper function. buttonStateForObject(obj) > WebCore/platform/brew/RenderThemeBrew.cpp:299 > + const int listboxPadding = 5; Nice. Using a constant here helps readability, thank you. > WebCore/platform/brew/RenderThemeBrew.cpp:326 > + if (lightness > 1.0) > + lightness = 1.0; > + if (lightness < 0.0) > + lightness = 0.0; Don't we already have a clamp function for this? > WebCore/platform/brew/RenderThemeBrew.cpp:338 > + SkColor baseColor = SkColorSetARGB(0xff, 0xdd, 0xdd, 0xdd); Does this color not have a constant already? > WebCore/platform/brew/RenderThemeBrew.cpp:360 > + canvas->drawLine(rect.x() + 1, rect.y(), right - 1, rect.y(), paint); > + canvas->drawLine(right - 1, rect.y() + 1, right - 1, bottom - 1, paint); > + canvas->drawLine(rect.x() + 1, bottom - 1, right - 1, bottom - 1, paint); > + canvas->drawLine(rect.x(), rect.y() + 1, rect.x(), bottom - 1, paint); Why all the +1/-1? That seems super error-prone. > WebCore/platform/brew/RenderThemeBrew.cpp:376 > + shader->unref(); Do we not have a smart pointer for these? > WebCore/platform/brew/RenderThemeBrew.cpp:378 > + skrect.set(rect.x() + 1, rect.y() + 1, right - 1, bottom - 1); Don't we have more descriptive ways of doing these +1/-1 bits? > WebCore/platform/brew/RenderThemeBrew.cpp:381 > + paint.setShader(0); I assume the paint was smart enoguh to keep a ref to the shader? > WebCore/platform/brew/RenderThemeBrew.cpp:386 > + canvas->drawPoint(rect.x() + 1, rect.y() + 1, paint); > + canvas->drawPoint(right - 2, rect.y() + 1, paint); > + canvas->drawPoint(rect.x() + 1, bottom - 2, paint); > + canvas->drawPoint(right - 2, bottom - 2, paint); What's all the +1/-2 junk? > WebCore/platform/brew/RenderThemeBrew.cpp:457 > + && style->hasAppearance() > + && style->appearance() != TextFieldPart > + && style->appearance() != SearchFieldPart > + && style->appearance() != TextAreaPart > + && style->appearance() != CheckboxPart > + && style->appearance() != RadioPart > + && style->appearance() != PushButtonPart > + && style->appearance() != SquareButtonPart > + && style->appearance() != ButtonPart > + && style->appearance() != ButtonBevelPart > + && style->appearance() != MenulistPart > + && style->appearance() != MenulistButtonPart; Should this be a switch?

Brew MP port is no longer maintained.