Comment 131 for bug 219385

Comment on attachment 563434
gtk drawing patch v.2 + comments

>>> if (interior_focus) {
>>>+ GtkBorder border;
>>>+ gtk_style_context_get_border(style, 0, &border);
>>Don't we want to pass the state flags here?
>
> What do you mean?

gtk_style_context_get_border() "Gets the border for a given state [...]" and
uses the "state" parameter rather than the state on the StyleContext. However,
this code is not passing anything for the GtkStateFlags state parameter.

Most, but not all, gtk_style_context_get_border calls in GTK widgets pass the
state flags.

>> gtk_tree_view_column_create_button uses GTK_SHADOW_IN.
>
> gtk_tree_view_column_create_button()? I don't see it anywhere.

http://git.gnome.org/browse/gtk+/tree/gtk/gtktreeviewcolumn.c?id=3.2.0#n876

>> In moz_gtk_tab_paint:
>>+ gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL);
>> I assume _NORMAL is the default, so this is unnecessary.
>
> I don't see GTK_STATE_FLAG_NORMAL in moz_gtk_tab_paint(), it's only in:
>
> moz_gtk_progressbar_paint
> moz_gtk_progress_chunk_paint
> moz_gtk_tabpanels_paint
> moz_gtk_check_menu_item_paint

I must have pasted something in the wrong place there.

I suspect all the gtk_style_context_set_state(style, GTK_STATE_FLAG_NORMAL)
calls are unnecessary. The pattern used in GTK is to create the style context
with default state and only ever set the state with a save/restore during
draw. Removing these would save a little code, but I don't feel strongly
about it, if you prefer to leave them in.

>>>+ gtk_render_frame_gap(style, cr,
>>>+ rect->x - gap_loffset,
>>>+ rect->y + gap_voffset - 3 * gap_height,
>>>+ rect->width + gap_loffset + gap_roffset,
>>>+ 3 * gap_height, GTK_POS_BOTTOM,
>>>+ gap_loffset, rect->width);
>>Need to also add gap_loffset here as you did below.
>
> Where the gap_loffset should be?

The last parameter is a position rather than a width, and so needs to be
gap_loffset + rect->width.

>>>+ gtk_render_frame_gap(style, cr,
>>>+ rect->x - gap_loffset,
>>>+ rect->y + rect->height - gap_voffset,
>>>+ rect->width + gap_loffset + gap_roffset,
>>>+ 3 * gap_height, GTK_POS_TOP,
>>>+ gap_loffset, gap_loffset+rect->width);
>> Can you add spaces around the "+", please?
>
> Where? All "+" I see have spaces around.

The last parameter is correct here, but there are no spaces around its "+".

>>+ gtk_render_frame(style, cr, rect->x, rect->y, rect->width, rect->height);
>> gtk_notebook_paint usually uses gtk_render_frame_gap.
>> Why choose render_frame over render_frame_gap here?
>
> I haven't been able to decrypt the former code logic plus gtk_render_frame_gap()
> does not allow to set negative gap start. gtk_render_frame() seems to work fine
> too but if you have an idea how to transport the gap size to moz_gtk_tabpanels_paint()
> I can modify it.

The gap size is not needed in moz_gtk_tabpanels_paint because the gap will be
painted with the foreground tab in moz_gtk_tab_paint.

However, if moz_gtk_tabpanels_paint just uses gtk_render_frame(), the theme
will think that there are no tabs and may draw something different. (Themes
have done this in the past at least.) Hence the trick of using two clip
regions, and drawing the gap outside each clip region, to get the correct
frame for a tabpanel with tabs.

With the API change from a cliprect parameter to a cairo_t, the clip
rectangles for each half will need to be applied to the cairo_t,
necessitating the use of cairo_save/restore. I suggest dividing rect into
left and right halves instead of the approach for gtk2 of dividing the
cliprect into two halves. (Getting the cliprect from cairo is harder and
unnecessary.)

For smaller tabpanels, the logic in the gtk2 port could lead to the gap being
outside the rect. I suggest just using rect->width as the gap position in the
render_frame_gap() call for the left half and 0 for the right half.

>+static GtkStateFlags
>+GetStateFlagFromGtkWidgetState(GtkWidgetState* state)

Can you call this "GetStateFlagsFromGtkWidgetState", please?
That would make it clearer that multiple flags may be set.

>+ else
>+ stateFlags |= GTK_STATE_FLAG_NORMAL;

No need for this as GTK_STATE_FLAG_NORMAL is not a flag but 0, the empty state
with no flags set.