[Webkit-unassigned] [Bug 46236] [BREWMP] Port RenderTheme to BREWMP.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=46236


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #70879|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #10 from Eric Seidel <eric at webkit.org>  2010-10-19 14:36:54 PST ---
(From update of attachment 70879)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.