Bug 52327 – [GTK] Move text field painting out of gtk2drawing.c

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This bug tracks moving text area painting out of gtk2drawing.c.

[Martin Robinson]

Created attachment 78773 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 78773 [details]
Patch

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

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:363
> +    backgroundRect.inflateX(-style->xthickness);
> +    backgroundRect.inflateY(-style->ythickness);

GTK+ doesn't take into account thickness (only when drawing spin buttons) when drawing both bg and frame, so I think we can use the same rectangle. See gtk+ code:

http://git.gnome.org/browse/gtk+/tree/gtk/gtkentry.c?h=gtk-2-24#n3282
http://git.gnome.org/browse/gtk+/tree/gtk/gtkentry.c?h=gtk-2-24#n3440

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:369
> +    gboolean interiorFocus = 0;

We don't need to initialize this, gtk_widget_style_get() always returns a value for booleans

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 78773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78773&action=review
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:363
> > +    backgroundRect.inflateX(-style->xthickness);
> > +    backgroundRect.inflateY(-style->ythickness);
> 
> GTK+ doesn't take into account thickness (only when drawing spin buttons) when drawing both bg and frame, so I think we can use the same rectangle. See gtk+ code:

After talking with Carlos, we agreed that this is the right approach to take (though messy). I'll upload a patch which explains the reasoning a bit more clearly via comments.

> http://git.gnome.org/browse/gtk+/tree/gtk/gtkentry.c?h=gtk-2-24#n3282
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkentry.c?h=gtk-2-24#n3440
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:369
> > +    gboolean interiorFocus = 0;

Removed.

[Martin Robinson]

Created attachment 78826 [details]
Patch with clearer comments

[Gustavo Noronha (kov)]

Comment on attachment 78826 [details]
Patch with clearer comments

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

Other than improving the comment, I think this patch is good to go!

> Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:372
> +    widgetContext.gtkPaintShadow(textFieldRect, widget, GTK_STATE_NORMAL, GTK_SHADOW_IN, "entry");

I'm not sure I understand what this comment says to be honest. So GTK+ relies on having the unfocused entry previously rendered, and then renders a smaller but complete text entry on top of it?

[Martin Robinson]

Created attachment 79438 [details]
Patch rebased to latest

[Martin Robinson]

(In reply to comment #5)
> (From update of attachment 78826 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78826&action=review
> 
> Other than improving the comment, I think this patch is good to go!
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk2.cpp:372
> > +    widgetContext.gtkPaintShadow(textFieldRect, widget, GTK_STATE_NORMAL, GTK_SHADOW_IN, "entry");
> 
> I'm not sure I understand what this comment says to be honest. So GTK+ relies on having the unfocused entry previously rendered, and then renders a smaller but complete text entry on top of it?

Thanks for the review! I clarified the comment and moved it closer to the actual special case for focus.

[Martin Robinson]

Committed r76182: <http://trac.webkit.org/changeset/76182>