Bug 52327 - [GTK] Move text field painting out of gtk2drawing.c
Summary: [GTK] Move text field painting out of gtk2drawing.c
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 52258
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-12 15:07 PST by Martin Robinson
Modified: 2011-01-19 17:06 PST (History)
1 user (show)

See Also:


Attachments
Patch (16.41 KB, patch)
2011-01-12 18:53 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with clearer comments (16.87 KB, patch)
2011-01-13 10:37 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch rebased to latest (16.40 KB, patch)
2011-01-19 09:53 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-01-12 15:07:42 PST
This bug tracks moving text area painting out of gtk2drawing.c.
Comment 1 Martin Robinson 2011-01-12 18:53:46 PST
Created attachment 78773 [details]
Patch
Comment 2 Carlos Garcia Campos 2011-01-13 08:50:50 PST
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
Comment 3 Martin Robinson 2011-01-13 10:35:10 PST
(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.
Comment 4 Martin Robinson 2011-01-13 10:37:33 PST
Created attachment 78826 [details]
Patch with clearer comments
Comment 5 Gustavo Noronha (kov) 2011-01-18 04:57:00 PST
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?
Comment 6 Martin Robinson 2011-01-19 09:53:11 PST
Created attachment 79438 [details]
Patch rebased to latest
Comment 7 Martin Robinson 2011-01-19 17:03:48 PST
(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.
Comment 8 Martin Robinson 2011-01-19 17:06:02 PST
Committed r76182: <http://trac.webkit.org/changeset/76182>