[webkit-reviews] review denied: [Bug 126907] [GTK] Hardcoded text color in input fields : [Attachment 341468] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Carlos Eduardo Ramalho
<cadubentzen at gmail.com>'s request for review:
Bug 126907: [GTK] Hardcoded text color in input fields
https://bugs.webkit.org/show_bug.cgi?id=126907

Attachment 341468: Patch

https://bugs.webkit.org/attachment.cgi?id=341468&action=review




--- Comment #57 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 341468
  --> https://bugs.webkit.org/attachment.cgi?id=341468
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341468&action=review

Patch looks good to me, but I think it can be improved a bit. Thanks!

> Source/WebCore/platform/gtk/RenderThemeGadget.cpp:78
> +

I would remove this empty line.

> Source/WebCore/platform/gtk/RenderThemeGadget.cpp:92
> +    static NeverDestroyed<GRefPtr<GtkStyleContext>> baseContext =
createBaseStyleContext();

This is weird, we have a create function that is always going to be used with a
NeverDestroyed. I would rename createBaseStyleContext() as baseStyleContext()
and make it return a plain pointer GtkStyleContext*. Then make the path created
inside the function static and return .get().

> Source/WebCore/platform/gtk/RenderThemeGadget.cpp:101
> +    m_context = createStyleContext(path.get(), parent ? parent->context() :
baseContext.get().get());

And here you could simply use baseStyleContext()

> Source/WebCore/rendering/RenderThemeGtk.cpp:497
> +static Color inputColor(const Element* element, RenderThemePart part)

This should receive a reference instead of a pointer.

> Source/WebCore/rendering/RenderThemeGtk.cpp:512
> +    GtkStateFlags state = element->isDisabledFormControl() ?
GTK_STATE_FLAG_INSENSITIVE : GTK_STATE_FLAG_NORMAL;
> +    gadget->setState(state);

We can probably make this a single line:

gadget->setState(element.isDisabledFormControl() ? GTK_STATE_FLAG_INSENSITIVE :
GTK_STATE_FLAG_NORMAL);

> Source/WebCore/rendering/RenderThemeGtk.cpp:530
> +    if (element)
> +	   style.setColor(inputColor(element, Button));

I wonder why we need inputColor, couldn't this be:

if (element)
    style.setColor(styleColor(Button, element->isDisabledFormControl() ?
GTK_STATE_FLAG_INSENSITIVE : GTK_STATE_FLAG_NORMAL, StyleColorForeground));

> Source/WebCore/rendering/RenderThemeGtk.cpp:982
> +    if (element)
> +	   style.setColor(inputColor(element, Entry));

I think we should move this after the next if. Again, I think this could be
done with current styleColor().

> Source/WebCore/rendering/RenderThemeGtk.cpp:1123
> +    if (element)
> +	   style.setColor(inputColor(element, Entry));

Same here.

> Source/WebCore/rendering/RenderThemeGtk.cpp:1248
> +    if (element)
> +	   style.setColor(inputColor(element, Entry));

And here.