[webkit-reviews] review granted: [Bug 51454] [GTK] Implement spin buttons in RenderThemeGtk : [Attachment 79250] Updated patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 51454: [GTK] Implement spin buttons in RenderThemeGtk
https://bugs.webkit.org/show_bug.cgi?id=51454

Attachment 79250: Updated patch
https://bugs.webkit.org/attachment.cgi?id=79250&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79250&action=review

Looks good. Consider my suggestions below.

> Source/WebCore/ChangeLog:10
> +	   Paint inner up/down buttons for spin button elements when building
> +	   with GTK+ 3.x.
> +

Please make a note here that test results will land with the GTK+ 2.x version
of this patch.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:761
> +    const PangoFontDescription* fontDesc =
gtk_style_context_get_font(context, static_cast<GtkStateFlags>(0));

Please don't abbreviate fontDesc.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:837
> +    // Paint arrow centered inside button.
> +    IntRect arrowRect;
> +    gdouble angle;
> +    if (arrowType == GTK_ARROW_UP) {
> +	   angle = 0;
> +	   arrowRect.setY(rect.y());
> +	   arrowRect.setHeight(rect.height() / 2 - 2);
> +    } else {
> +	   angle = G_PI;
> +	   arrowRect.setY(rect.y() + buttonRect.y());
> +	   arrowRect.setHeight(rect.height() - arrowRect.y() - 2);
> +    }
> +    arrowRect.setWidth(rect.width() - 3);
> +    if (direction == GTK_TEXT_DIR_LTR)
> +	   arrowRect.setX(rect.x() + 1);
> +    else
> +	   arrowRect.setX(rect.x() + 2);
> +
> +    gint width = arrowRect.width() / 2;
> +    width -= width % 2 - 1; // Force odd.
> +    gint height = (width + 1) / 2;

For quite some time, I wondered why this logic was so hairy and thought surely
there must be a better way. Then I set out to implement this code for GTK+ 2.x.
After some time doing that and looking at the GTK+ code, I understand
completely now.

I think we should would leave a comment at the top explaining that we're trying
to emulate gtkspinbutton.c as closely as possible here. I'd also prefer "int"
to "gint" in most places. :)