[Webkit-unassigned] [Bug 51828] [GTK] Port combo box painting to GtkStyleContext

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-01-10 09:29:02 PST ---
(In reply to comment #4)
> (From update of attachment 78392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78392&action=review
> 
> Look good, but there are a few minor things that you could possibly address. Do all comboboxes have separators now? Menulists are very complicated. :)
> 
> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:53
> > +// Default value defined by GTK+.
> > +static const int minArrowSize = 15;
> 
> Do you mind including a little bit more information to help future readers find where this is defined in GTK+?

Sure, I'll do

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:321
> > +    GtkBorder borderWidth;
> > +    gtk_style_context_get_border(context, static_cast<GtkStateFlags>(0), &borderWidth);
> > +    border = borderWidth;
> 
> I think if you used gtk_style_context_get_border(context, static_cast<GtkStateFlags>(0), &border); here you could do away totally with borderWidth.

Ok

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:348
> > +    if (!wideSeparators)
> > +        separatorWidth = border.left;
> 
> Why the left border? Is this arbitrary of does it depend on the text direction?

it doesn't depend on text direction, it's border.left because we know the separator will be vertical. See how it's done in gtk+

http://git.gnome.org/browse/gtk+/tree/gtk/gtkseparator.c#n156

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:362
> > +    if (style->direction() == RTL)
> > +        left += separatorWidth + minArrowSize;
> 
> If I'm not mistaken, if the combo box doesn't have a separator we technically shouldn't do this. Is there any way to check for that?

if the combo box doesn't have a separator is becase separatorWidth == 0. 

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:464
> > +    gfloat xalign = 0.5;
> > +    gfloat yalign = 0.5;
> > +    if (direction != GTK_TEXT_DIR_LTR)
> > +        xalign = 1.0 - xalign;
> 
> Doesn't this mean that xalign is always 0.5? If so wouldn't it just be better to use / 2 below and avoid xalign and yalign completely? In any case, yalign can go.

Both xalign and yalign are 0.5 by default in gtk+, I left the code this way to make it closer to gtk+ code, so that it will be easier to maintain in case gtk+ code changes. 

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:465
> > +    arrowPosition.move(floor((arrowSize.width() - extent) * xalign), floor((arrowSize.height() - extent) * yalign));
> 
> Why is this adjustment necessary. A comment might be good here.

To center the arrow, this is also based on gtk+ code:

http://git.gnome.org/browse/gtk+/tree/gtk/gtkarrow.c#n306

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:514
> > +        paintInfo.context->save();
> 
> I'd much rather there be a cairo_clip here for consistency.

what do you mean?

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:515
> > +        cairo_rectangle(paintInfo.context->platformContext(), separatorPosition.x(), separatorPosition.y(), border.left, innerRect.height());
> 
> Why is this extra clip necessary? A comment is probably needed.

Otherwhise there will be an extra vertical white line, I think it's because of pixel alignment, but I'm not sure.

> > Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:519
> > +        paintInfo.context->restore();
> 
> I'd prefer cairo_restore here.

Me too :-), I thought I should use WebCore stuff always when possible.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.