Bug 46236 – [BREWMP] Port RenderTheme to BREWMP.

• Component: Platform
• Status: RESOLVED WONTFIX

[Hyung Song]

Enable drawing Button, Checkbox, Radio, MenuList for BREWMP.
Used Android implementations as reference.

[Hyung Song]

Created attachment 68322 [details]
Patch

[Kwang Yul Seo]

(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.

[Hyung Song]

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

[Kwang Yul Seo]

(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?

[Hyung Song]

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

[Dimitri Glazkov (Google)]

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?

[Hyung Song]

Created attachment 70879 [details]
patch

Fix year in license.

[Kwang Yul Seo]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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?

[Kwang Yul Seo]

Brew MP port is no longer maintained.