Bug 51828 – [GTK] Port combo box painting to GtkStyleContext

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

This depends on bug #51764 since the patch for stock icons contains the initial GtkStyleContext port

[Carlos Garcia Campos]

Created attachment 77818 [details]
Paint combo boxes using GtkStyleContext API

[Carlos Garcia Campos]

Created attachment 78392 [details]
Updated patch for current git master

Patch updated to current git master, cleaned it up, fixed some minor issues and refactored paintButton to reuse the code to paint the combobox button.

[WebKit Review Bot]

Attachment 78392 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp']" exit_code: 1
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:328:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:345:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:416:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:436:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:453:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:485:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 78392 [details]
Updated patch for current git master

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.

[Carlos Garcia Campos]

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

[Martin Robinson]

(In reply to comment #5)

> > > 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. 
> To center the arrow, this is also based on gtk+ code:
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkarrow.c#n306

I still think it's better to use "/ 2" then here. We should favor clarity. The GTK+ code fetches the xalign and valign via gtk_misc_get_alignment. I think that's why it uses xalign and valign. Wouldn't it be safer to do the same thing here?

> > I'd prefer cairo_restore here.
> Me too :-), I thought I should use WebCore stuff always when possible.

Hrm. Interesting point. In my opinion, if code can be made simpler by using WebCore conveniences such as IntRect (which provides niceties like inflate), then I say we should use them. On the other hand, some WebCore classes exist only to wrap platform differences (like GraphicsContext). In platform-dependent code there is little reason to use them and it might add unecessary and unexpected overhead.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > > > 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. 
> > To center the arrow, this is also based on gtk+ code:
> > http://git.gnome.org/browse/gtk+/tree/gtk/gtkarrow.c#n306
> 
> I still think it's better to use "/ 2" then here. We should favor clarity.

Ok.

> The GTK+ code fetches the xalign and valign via gtk_misc_get_alignment. I think that's why it uses xalign and valign.

Yes, indeed.

> Wouldn't it be safer to do the same thing here?

We can't, we don't have a widget to get the alignment from, that's why I'm using the default values. 

> > > I'd prefer cairo_restore here.
> > Me too :-), I thought I should use WebCore stuff always when possible.
> 
> Hrm. Interesting point. In my opinion, if code can be made simpler by using WebCore conveniences such as IntRect (which provides niceties like inflate), then I say we should use them. On the other hand, some WebCore classes exist only to wrap platform differences (like GraphicsContext). In platform-dependent code there is little reason to use them and it might add unecessary and unexpected overhead.

ok, I'll use cairo api directly here which is much cleaner indeed.

[Martin Robinson]

Created attachment 78422 [details]
Patch

[Martin Robinson]

(In reply to comment #8)
> Created an attachment (id=78422) [details]
> Patch

I'm the interest of making sure this work is complete for the release, I've made the changes I suggested to the patch and also changed the generic variable "context" to "buttonStyleContext", "separatorStyleContext" and "arrowStyleContext" for the sake of clarity.

[WebKit Review Bot]

Attachment 78422 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp']" exit_code: 1
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:385:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:402:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:471:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:493:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:510:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:540:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 6 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Comment on attachment 78422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78422&action=review

r=me

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:48
> +using namespace std;

Why is this needed?

PS: Martin says it might be for 'floor'. Better to include whatever is needed and/or do std::floor instead of doing this for just two calls?

[Martin Robinson]

(In reply to comment #11)
> PS: Martin says it might be for 'floor'. Better to include whatever is needed and/or do std::floor instead of doing this for just two calls?

Okay!

[Martin Robinson]

Committed r75409: <http://trac.webkit.org/changeset/75409>