Bug 51828 - [GTK] Port combo box painting to GtkStyleContext
Summary: [GTK] Port combo box painting to GtkStyleContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 51764
Blocks: 50820
  Show dependency treegraph
 
Reported: 2011-01-03 10:16 PST by Carlos Garcia Campos
Modified: 2011-01-10 12:24 PST (History)
3 users (show)

See Also:


Attachments
Paint combo boxes using GtkStyleContext API (11.97 KB, patch)
2011-01-03 10:20 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch for current git master (15.52 KB, patch)
2011-01-10 04:21 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (16.17 KB, patch)
2011-01-10 11:57 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-01-03 10:16:35 PST
This depends on bug #51764 since the patch for stock icons contains the initial GtkStyleContext port
Comment 1 Carlos Garcia Campos 2011-01-03 10:20:15 PST
Created attachment 77818 [details]
Paint combo boxes using GtkStyleContext API
Comment 2 Carlos Garcia Campos 2011-01-10 04:21:29 PST
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.
Comment 3 WebKit Review Bot 2011-01-10 04:24:25 PST
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.
Comment 4 Martin Robinson 2011-01-10 08:51:00 PST
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.
Comment 5 Carlos Garcia Campos 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.
Comment 6 Martin Robinson 2011-01-10 09:42:15 PST
(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.
Comment 7 Carlos Garcia Campos 2011-01-10 11:15:55 PST
(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.
Comment 8 Martin Robinson 2011-01-10 11:57:25 PST
Created attachment 78422 [details]
Patch
Comment 9 Martin Robinson 2011-01-10 11:58:31 PST
(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.
Comment 10 WebKit Review Bot 2011-01-10 12:00:19 PST
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.
Comment 11 Xan Lopez 2011-01-10 12:11:08 PST
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?
Comment 12 Martin Robinson 2011-01-10 12:19:03 PST
(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!
Comment 13 Martin Robinson 2011-01-10 12:23:59 PST
Committed r75409: <http://trac.webkit.org/changeset/75409>