[webkit-reviews] review denied: [Bug 46236] [BREWMP] Port RenderTheme to BREWMP. : [Attachment 70879] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Hyung Song <beergun at company100.net>'s
request for review:
Bug 46236: [BREWMP] Port RenderTheme to BREWMP.
https://bugs.webkit.org/show_bug.cgi?id=46236

Attachment 70879: patch
https://bugs.webkit.org/attachment.cgi?id=70879&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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?