[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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78392|review?                     |review-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2011-01-10 08:51:00 PST ---
(From update of attachment 78392)
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+?

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

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

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

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

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

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:514
> +        paintInfo.context->save();

I'd much rather there be a cairo_clip here for consistency.

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

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:519
> +        paintInfo.context->restore();

I'd prefer cairo_restore here.

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