[webkit-reviews] review denied: [Bug 51828] [GTK] Port combo box painting to GtkStyleContext : [Attachment 78392] Updated patch for current git master

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 51828: [GTK] Port combo box painting to GtkStyleContext
https://bugs.webkit.org/show_bug.cgi?id=51828

Attachment 78392: Updated patch for current git master
https://bugs.webkit.org/attachment.cgi?id=78392&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.