[Webkit-unassigned] [Bug 126907] [GTK] Hardcoded text color in input fields

[bugzilla-daemon at webkit.org]

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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #341468|review?                     |review-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180529/61c1a4bd/attachment.html>