1282753 - Move ComboBoxes WidgetCache

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P4)

Description

[Martin Stránský [:stransky] (ni? me)]

Let's move other widgets to WidgetCache and use them in other widget code.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: combo-entry-button.patch

There's complete patch for buttons, combo boxes, entry. If you agree with the overall concept I'll divide it to smaller parts for better review.

There are some implementation details which may be discussed:

- I used only one universal routine to get inner widgets GetInnerWidget(), it uses G_TYPE_CHECK_INSTANCE_TYPE(). I think it's okay to use that.

- The g_object_add_weak_pointer() call needs to point directly at sWidgetStorage. Do we actually need that? Does the internal layout ever change?

- possible null combo box separator means we need a null check in GetCssNodeStyleInternal()/GetWidgetStyleInternal().

- moz_gtk_entry_paint has been reworked accept style instead of the widget and the style is adjusted at moz_gtk_widget_paint() level (direction, state).

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: button and entry patch

There's a less complicated first part of this bug, Thanks.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41094d53e960

Comment 3

[Andrew Comminos [:acomminos]]

Comment on attachment 8772352 [details] [diff] [review]
button and entry patch

Review of attachment 8772352 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few things:

::: widget/gtk/WidgetStyleCache.cpp
@@ +173,5 @@
> +  return widget;
> +}
> +
> +static GtkWidget*
> +CreateToogleButtonWidget()

Typo. s/Toogle/Toggle.

@@ +185,5 @@
> +CreateButtonArrowWidget()
> +{
> +  GtkWidget* widget = gtk_arrow_new(GTK_ARROW_DOWN, GTK_SHADOW_OUT);
> +  gtk_container_add(GTK_CONTAINER(GetWidget(MOZ_GTK_TOGGLE_BUTTON)), widget);
> +  gtk_widget_realize(widget);

Is this realize still necessary? It was there previously, but I'm not sure why.

@@ +193,5 @@
> +
> +static GtkWidget*
> +CreateSpinWidget()
> +{
> +  GtkWidget* widget = gtk_spin_button_new(NULL, 1, 0);

Prefer nullptr.

::: widget/gtk/gtk3drawing.cpp
@@ +206,5 @@
>      } else {
>          /* Shouldn't be reached with current internal gtk implementation; we
>           * use a generic toggle button as last resort fallback to avoid
>           * crashing. */
> +        gComboBoxButtonWidget = GetWidget(MOZ_GTK_TOGGLE_BUTTON);

Any reason we have to keep this global around instead of just calling GetWidget(MOZ_GTK_TOGGLE_BUTTON)? Same goes for the other widgets in this patch (e.g. gComboBoxArrowWidget).

@@ +2948,2 @@
>          return moz_gtk_entry_paint(cr, rect, state,
> +                                   GetWidget(MOZ_GTK_SPINBUTTON), direction);

Should the argument to GetWidget be MOZ_GTK_SPINBUTTON_ENTRY here?

We don't seem to use the code in WidgetStyleCache for MOZ_GTK_SPINBUTTON_ENTRY otherwise.

::: widget/gtk/gtkdrawing.h
@@ +86,5 @@
>    /* Paints a GtkButton. flags is a GtkReliefStyle. */
>    MOZ_GTK_BUTTON,
>    /* Paints a button with image and no text */
>    MOZ_GTK_TOOLBAR_BUTTON,
> +  /* Paints a toogle button */

s/toogle/toggle

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Thanks, I've updated it.

> @@ +185,5 @@
> > +CreateButtonArrowWidget()
> > +{
> > +  GtkWidget* widget = gtk_arrow_new(GTK_ARROW_DOWN, GTK_SHADOW_OUT);
> > +  gtk_container_add(GTK_CONTAINER(GetWidget(MOZ_GTK_TOGGLE_BUTTON)), widget);
> > +  gtk_widget_realize(widget);
> 
> Is this realize still necessary? It was there previously, but I'm not sure
> why.

All widgets are realized in AddToWindowContainer() when are added to our global container. We don't use that for GtkArrow (we insert it as a child of already realized MOZ_GTK_TOGGLE_BUTTON) so we realize it here.

We can investigate and remove the realize from all widgets in another patch/but but I'd prefer to keep it here for now to create the widgets in the same way.

> ::: widget/gtk/gtk3drawing.cpp
> @@ +206,5 @@
> >      } else {
> >          /* Shouldn't be reached with current internal gtk implementation; we
> >           * use a generic toggle button as last resort fallback to avoid
> >           * crashing. */
> > +        gComboBoxButtonWidget = GetWidget(MOZ_GTK_TOGGLE_BUTTON);
> 
> Any reason we have to keep this global around instead of just calling
> GetWidget(MOZ_GTK_TOGGLE_BUTTON)? Same goes for the other widgets in this
> patch (e.g. gComboBoxArrowWidget).

The gComboBoxButtonWidget and co. will be moved to WidgetCache in another patch - I used the GetWidget(MOZ_GTK_TOGGLE_BUTTON) here to keep the code build/run before we do that.

> @@ +2948,2 @@
> >          return moz_gtk_entry_paint(cr, rect, state,
> > +                                   GetWidget(MOZ_GTK_SPINBUTTON), direction);
> 
> Should the argument to GetWidget be MOZ_GTK_SPINBUTTON_ENTRY here?
> 
> We don't seem to use the code in WidgetStyleCache for
> MOZ_GTK_SPINBUTTON_ENTRY otherwise.

Yes, that's another TODO here and I'll add a comment for it. Recent moz_gtk_entry_paint() gets a widget and adds _ENTRY class to base style and draws entry for various elements - GtkEntry, ComboBoxes, Spin buttons.

moz_gtk_entry_paint() may be converted to accept style directly and then we use MOZ_GTK_SPINBUTTON_ENTRY, MOZ_GTK_COMBOBOX_ENTRY, MOZ_GTK_ENTRY styles as params.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: button and entry v.2 patch

New one, Thanks!

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8772735 [details] [diff] [review]
button and entry v.2 patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef4a9aaa8c26

Comment 7

[Andrew Comminos [:acomminos]]

Comment on attachment 8772735 [details] [diff] [review]
button and entry v.2 patch

Review of attachment 8772735 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, that sounds reasonable enough to me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7f83477045
move button and entry widgets from gtk3drawing to WidgetCache, r=andrew@comminos

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d7f83477045

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: combo box patch

There's a combo-box only patch, Thanks.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cccbc35b2882

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8776540 [details] [diff] [review]
combo box patch

Andrew, would you mind to look at it please? Would be great to have it fixed for Bug 1158076 and Bug 1303310.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

Review commit: https://reviewboard.mozilla.org/r/79452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79452/

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Attachment: combobox patch for latest trunk

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review77988

This is mostly good, thanks.  There are a few style and minor things, so worth another review I think.

::: widget/gtk/WidgetStyleCache.cpp:213
(Diff revision 1)
> +  GtkWidget* widget = gtk_combo_box_new();
> +  AddToWindowContainer(widget);
> +  return widget;
> +}
> +
> +typedef struct 

New trailing whitespace here and a few other places.

::: widget/gtk/WidgetStyleCache.cpp:222
(Diff revision 1)
> +} GtkInnerWidgetInfo;
> +
> +static void
> +GetInnerWidget(GtkWidget *widget, gpointer client_data)
> +{
> +  GtkInnerWidgetInfo *info = (GtkInnerWidgetInfo *)client_data; 

auto info = static_cast<GtkInnerWidgetInfo*>(client_data);

C-style casts (GtkInnerWidgetInfo *) should be avoided, as the C++ casts are either more explicit or provide some level of type checking.

::: widget/gtk/WidgetStyleCache.cpp:234
(Diff revision 1)
> +  GtkWidget *comboBox = GetWidget(MOZ_GTK_COMBOBOX);
> +  GtkWidget *comboBoxButton = nullptr;

* by the type.  Similarly elsewhere.

::: widget/gtk/WidgetStyleCache.cpp:240
(Diff revision 1)
> +  GtkWidget *comboBoxButton = nullptr;
> +
> +  /* Get its inner Button */
> +  GtkInnerWidgetInfo info = { GTK_TYPE_TOGGLE_BUTTON, &comboBoxButton};
> +  gtk_container_forall(GTK_CONTAINER(comboBox),
> +                       GetInnerWidget, (gpointer) &info);

The gpointer cast should not be required as casting to void* is an up-cast.

Casts can hide type errors, so better not to use them if not necessary.

Similarly elsewhere.

::: widget/gtk/WidgetStyleCache.cpp:282
(Diff revision 1)
> +     * needs to place a cell renderer, a separator, and an arrow in
> +     * the button when appears-as-list is FALSE. */
> +    GtkInnerWidgetInfo info = { GTK_TYPE_ARROW, &comboBoxArrow};
> +    gtk_container_forall(GTK_CONTAINER(buttonChild),
> +                         GetInnerWidget, (gpointer) &info);
> +  } else if(GTK_IS_ARROW(buttonChild)) {

space after "if"

::: widget/gtk/WidgetStyleCache.cpp:331
(Diff revision 1)
> +    /* comboBoxSeparator may be none
> +     * when "appears-as-list" = TRUE or "cell-view" = FALSE; if it
> +     * is invalid we just won't paint it. */

s/none/NULL/

s/if it is invalid/if there is no separator, then/

::: widget/gtk/WidgetStyleCache.cpp:407
(Diff revision 1)
> +     * needs to place a cell renderer, a separator, and an arrow in
> +     * the button when appears-as-list is FALSE. */
> +    GtkInnerWidgetInfo info = { GTK_TYPE_ARROW, &comboBoxArrow};
> +    gtk_container_forall(GTK_CONTAINER(buttonChild),
> +                         GetInnerWidget, (gpointer) &info);
> +  } else if(GTK_IS_ARROW(buttonChild)) {

space after "if"

::: widget/gtk/WidgetStyleCache.cpp:553
(Diff revision 1)
>    GtkWidgetPath* path = aParentStyle ?
>      gtk_widget_path_copy(gtk_style_context_get_path(aParentStyle)) :
>      gtk_widget_path_new();
>  
>    // Work around https://bugzilla.gnome.org/show_bug.cgi?id=767312
> -  // which exists in GTK+ 3.20.
> +  // which exists in GTK 3.20.

GTK+ is the name of the toolkit so please don't remove the +.

::: widget/gtk/WidgetStyleCache.cpp:689
(Diff revision 1)
> +      if (!widget)
> +        return nullptr;

ClaimStyleContext assumes that this returns non-null, and so this new code does not provide any benefit.

::: widget/gtk/WidgetStyleCache.cpp:758
(Diff revision 1)
> -      MOZ_ASSERT(widget);
> +      if (!widget)
> +        return nullptr;

Similarly.

::: widget/gtk/gtk3drawing.cpp:254
(Diff revision 1)
>  gint
> -moz_gtk_get_focus_outline_size(gint* focus_h_width, gint* focus_v_width)
> +moz_gtk_get_focus_outline_size(GtkStyleContext *style, 

static

::: widget/gtk/gtk3drawing.cpp:254
(Diff revision 1)
>  gint
> -moz_gtk_get_focus_outline_size(gint* focus_h_width, gint* focus_v_width)
> +moz_gtk_get_focus_outline_size(GtkStyleContext *style, 



::: widget/gtk/gtk3drawing.cpp:1124
(Diff revision 1)
>      moz_gtk_button_paint(cr, rect, state, GTK_RELIEF_NORMAL,
> -                         gComboBoxButtonWidget, direction);
> +                         GetWidget(MOZ_GTK_COMBOBOX_BUTTON), direction);
>  
> -    calculate_button_inner_rect(gComboBoxButtonWidget,
> +    calculate_button_inner_rect(GetWidget(MOZ_GTK_COMBOBOX_BUTTON),

Call GetWidget(MOZ_GTK_COMBOBOX_BUTTON) once and keep the result in a temporary to reuse.

::: widget/gtk/gtk3drawing.cpp:1150
(Diff revision 1)
>  
>      /* If there is no separator in the theme, there's nothing left to do. */
> -    if (!gComboBoxSeparatorWidget)
> +    GtkWidget *widget = GetWidget(MOZ_GTK_COMBOBOX_SEPARATOR);
> +    if (!widget)
>          return MOZ_GTK_SUCCESS;
> -    style = gtk_widget_get_style_context(gComboBoxSeparatorWidget);
> +    style = ClaimStyleContext(MOZ_GTK_COMBOBOX_SEPARATOR);

Probably a little better to keep the gtk_widget_get_style_context() on widget rather than using ClaimStyleContext() to find the widget again.

Similarly in _get_widget_border.

::: widget/gtk/gtk3drawing.cpp:1233
(Diff revision 1)
> -                         gComboBoxEntryButtonWidget, direction);
> +                         GetWidget(MOZ_GTK_COMBOBOX_ENTRY_BUTTON), direction);
>  
> -    calculate_button_inner_rect(gComboBoxEntryButtonWidget,
> +    calculate_button_inner_rect(GetWidget(MOZ_GTK_COMBOBOX_ENTRY_BUTTON),

Call GetWidget(MOZ_GTK_COMBOBOX_ENTRY_BUTTON) once.

::: widget/gtk/gtk3drawing.cpp:2187
(Diff revision 1)
>                      separator_width = border.left;
>                  }
> +                ReleaseStyleContext(style);
>              }
>  
> -            gtk_widget_get_preferred_size(gComboBoxArrowWidget, NULL, &arrow_req);
> +            gtk_widget_get_preferred_size(GetWidget(MOZ_GTK_COMBOBOX_ARROW), NULL, &arrow_req);

Wrap to stay within 80 columns.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Attachment: combo box v.2

Thanks, this should address that.

> :: widget/gtk/WidgetStyleCache.cpp:689
> +      if (!widget)
> +        return nullptr;
>
> ClaimStyleContext assumes that this returns non-null, 
> and so this new code does not provide any benefit.

It was here because of MOZ_GTK_COMBOBOX_SEPARATOR widget can be null so ClaimStyleContext(MOZ_GTK_COMBOBOX_SEPARATOR) will crash then. But I added a local check to handle that.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review83668

Revision 2 on mozreview is a mechanical rebase of revision 1, to help mozreview distinguish between changes in m-c and changes to the patch.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79452/diff/1-2/

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79452/diff/2-3/

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 8797606 [details] [diff] [review]
combo box v.2

Moved to mozreview revision 3.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review77988

> * by the type.  Similarly elsewhere.

Gecko style is GtkWidget* comboBoxButton = nullptr;

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review83940

::: widget/gtk/WidgetStyleCache.cpp:224
(Diff revisions 2 - 3)
>  }
>  
> -typedef struct 
> +typedef struct
>  {
>    GType       type;
> -  GtkWidget** widget;
> +  void**      widget;

AFAICS GtkWidget** was fine here and there is no need for the G_TYPE_CHECK_INSTANCE_CASTs to specific widget types.

GtkWidget** also avoids the need for reinterpret_cast<void **>.

::: widget/gtk/WidgetStyleCache.cpp:915
(Diff revisions 2 - 3)
> +    case MOZ_GTK_COMBOBOX_SEPARATOR:
> +    {
> +      // TODO - create style from style path
> +      // We need to check returned widget because
> +      // MOZ_GTK_COMBOBOX_SEPARATOR may not be present
> +      // on actual style
> +      GtkWidget* widget = GetWidget(aNodeType);
> +      return (widget) ? gtk_widget_get_style_context(widget) : nullptr;
> +    }

(In reply to Martin Stránský from comment #15)
> > ClaimStyleContext assumes that this returns non-null, 
> > and so this new code does not provide any benefit.
> 
> It was here because of MOZ_GTK_COMBOBOX_SEPARATOR widget can be null so
> ClaimStyleContext(MOZ_GTK_COMBOBOX_SEPARATOR) will crash then. But I added a
> local check to handle that.

The special case for MOZ_GTK_COMBOBOX_SEPARATOR is probably preferable (because it is a special case), but this still provides no benefit because the caller of GetCssNodeStyleInternal() assumes that it returns non-null.

But this is all unnecessary as ClaimStyleContext(MOZ_GTK_COMBOBOX_SEPARATOR) is never called now.  Even previously, there was a check on GetWidget(MOZ_GTK_COMBOBOX_SEPARATOR) before fetching the style.

::: widget/gtk/WidgetStyleCache.cpp:1046
(Diff revisions 2 - 3)
> +    case MOZ_GTK_COMBOBOX_SEPARATOR:
> +    {
> +      // We need to check returned widget because
> +      // MOZ_GTK_COMBOBOX_SEPARATOR may not be present
> +      // on actual style
> +      GtkWidget* widget = GetWidget(aNodeType);
> +      return (widget) ? gtk_widget_get_style_context(widget) : nullptr;

Similarly.

::: widget/gtk/gtk3drawing.cpp:1960
(Diff revisions 2 - 3)
> -            *left = *top = *right = *bottom = 
> -                gtk_container_get_border_width(GTK_CONTAINER(GetWidget(MOZ_GTK_COMBOBOX_BUTTON)));
> +            gtk_container_get_border_width(GTK_CONTAINER(
> +                GetWidget(MOZ_GTK_COMBOBOX_BUTTON)));

Was this change accidental?

::: widget/gtk/gtk3drawing.cpp:505
(Diff revision 3)
> -  
> +
>      gtk_render_arrow(style, cr, arrow_angle,
>                       arrow_rect.x,
> -                     arrow_rect.y, 
> +                     arrow_rect.y,
>                       arrow_size);
> -  
> +

Please no whitespace changes on lines otherwise untouched by this patch.  They add noise to the patch and history, and unnecessary merge conflicts for pending patches.  Similarly elsewhere.

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review83940

> AFAICS GtkWidget** was fine here and there is no need for the G_TYPE_CHECK_INSTANCE_CASTs to specific widget types.
> 
> GtkWidget** also avoids the need for reinterpret_cast<void **>.

I don't undrestand here. For instance I can't type GtkToogleButton* to GtkWidget* by GTK_WIDGET macro to get the GtkWidget** pointer. IMHO we need to use GtkWidget* as a pointer type instead - do you mean that?

> (In reply to Martin Stránský from comment #15)
> > > ClaimStyleContext assumes that this returns non-null, 
> > > and so this new code does not provide any benefit.
> > 
> > It was here because of MOZ_GTK_COMBOBOX_SEPARATOR widget can be null so
> > ClaimStyleContext(MOZ_GTK_COMBOBOX_SEPARATOR) will crash then. But I added a
> > local check to handle that.
> 
> The special case for MOZ_GTK_COMBOBOX_SEPARATOR is probably preferable (because it is a special case), but this still provides no benefit because the caller of GetCssNodeStyleInternal() assumes that it returns non-null.
> 
> But this is all unnecessary as ClaimStyleContext(MOZ_GTK_COMBOBOX_SEPARATOR) is never called now.  Even previously, there was a check on GetWidget(MOZ_GTK_COMBOBOX_SEPARATOR) before fetching the style.

Well, it's a kind of easter egg but okay.

> Similarly.

ok

> Was this change accidental?

oh, yes, thanks.

> Please no whitespace changes on lines otherwise untouched by this patch.  They add noise to the patch and history, and unnecessary merge conflicts for pending patches.  Similarly elsewhere.

Okay, that comes from atom editor I tried last time.

Comment 23

[Martin Stránský [:stransky] (ni? me)]

Attachment: combo box v.3

Thanks, there's a new one.

(In reply to Martin Stránský from comment #22)
> Comment on attachment 8792378 [details]
> Bug 1282753 - implement combo boxes by WidgetCache,
> 
> https://reviewboard.mozilla.org/r/79452/#review83940
> 
> > AFAICS GtkWidget** was fine here and there is no need for the G_TYPE_CHECK_INSTANCE_CASTs to specific widget types.
> > 
> > GtkWidget** also avoids the need for reinterpret_cast<void **>.
> 
> I don't undrestand here. For instance I can't type GtkToogleButton* to
> GtkWidget* by GTK_WIDGET macro to get the GtkWidget** pointer. IMHO we need
> to use GtkWidget* as a pointer type instead - do you mean that?

The attached patch use the GtkWidget* for widgets obtained via GtkInnerWidgetInfo struct - we may need a temporary GtkWidget* otherwise to get GtkWidget** IMHO.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79452/diff/3-4/

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8801690 [details] [diff] [review]
combo box v.3

Moved to mozreview revision 4.

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review83940

> I don't undrestand here. For instance I can't type GtkToogleButton* to GtkWidget* by GTK_WIDGET macro to get the GtkWidget** pointer. IMHO we need to use GtkWidget* as a pointer type instead - do you mean that?

Yes, that's exactly what I mean, thanks.

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review85422

::: widget/gtk/WidgetStyleCache.cpp:278
(Diff revisions 2 - 4)
> -  GtkWidget *comboBoxButton = GetWidget(MOZ_GTK_COMBOBOX_BUTTON);
> +  GtkToggleButton *comboBoxButton =
> +      GTK_TOGGLE_BUTTON(GetWidget(MOZ_GTK_COMBOBOX_BUTTON));

I wonder whether we've had a misunderstanding somewhere.  GtkWidget* was fine.
comboxBoxButton can stay GtkWidget*, and then the GTK_TOGGLE_BUTTON() is not necessary.

::: widget/gtk/WidgetStyleCache.cpp:297
(Diff revisions 2 - 4)
> -                         GetInnerWidget, (gpointer) &info);
> -  } else if(GTK_IS_ARROW(buttonChild)) {
> +                         GetInnerWidget, &info);
> +  } else if (GTK_IS_ARROW(buttonChild)) {
>      /* appears-as-list = TRUE, or cell-view = FALSE;
>       * the button only contains an arrow */
>      comboBoxArrow = buttonChild;
> -    gtk_widget_realize(comboBoxArrow);
> +    gtk_widget_realize(GTK_WIDGET(comboBoxArrow));

comboBoxArrow is already GtkWidget now, so no need for GTK_WIDGET().

::: widget/gtk/gtk3drawing.cpp:2471
(Diff revision 4)
>      case MOZ_GTK_TREE_HEADER_CELL:
>          return moz_gtk_tree_header_cell_paint(cr, rect, state,
>                                                flags, direction);
>          break;
>      case MOZ_GTK_TREE_HEADER_SORTARROW:
> -        return moz_gtk_tree_header_sort_arrow_paint(cr, rect, 
> +        return moz_gtk_tree_header_sort_arrow_paint(cr, rect,

Just one remaining whitespace change.

Comment 28

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review77988

> Gecko style is GtkWidget* comboBoxButton = nullptr;

Much of the code being replaced placed the star by the type, which is Gecko style. e.g. ensure_combo_box_widgets() had "GtkWidget* buttonChild".
This patch is placing the star by the variable name. e.g. CreateComboBoxArrowWidget() declares "GtkWidget *buttonChild".

The change to declaration at initialization is good thanks, but the * should be beside the type for consistency with Gecko style.

Comment 29

[Martin Stránský [:stransky] (ni? me)]

Attachment: combo v.4 patch

Thanks, I hope this one is okay now.

Comment 30

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79452/diff/4-5/

Comment 31

[Karl Tomlinson (:karlt)]

Comment on attachment 8802036 [details] [diff] [review]
combo v.4 patch

Moved to mozreview revision 5.

Comment 32

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

https://reviewboard.mozilla.org/r/79452/#review85744

::: widget/gtk/gtk3drawing.cpp:985
(Diff revision 5)
>                       real_arrow_rect.x, real_arrow_rect.y,
>                       real_arrow_rect.width);
> +    ReleaseStyleContext(style);
>  
>      /* If there is no separator in the theme, there's nothing left to do. */
> -    if (!gComboBoxSeparatorWidget)
> +    GtkWidget *widget = GetWidget(MOZ_GTK_COMBOBOX_SEPARATOR);

I'll move the star here.

::: widget/gtk/gtk3drawing.cpp:1069
(Diff revision 5)
>      gint x_displacement, y_displacement;
>      GdkRectangle arrow_rect, real_arrow_rect;
>      GtkStateFlags state_flags = GetStateFlagsFromGtkWidgetState(state);
>      GtkStyleContext* style;
>  
> -    ensure_combo_box_entry_widgets();
> +    GtkWidget *comboBoxEntry = GetWidget(MOZ_GTK_COMBOBOX_ENTRY_BUTTON);

and here.

Comment 33

[Karl Tomlinson (:karlt)]

Comment on attachment 8792378 [details]
Bug 1282753 - implement combo boxes by WidgetCache, r=?karlt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79452/diff/5-6/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d6136e0a045
implement combo boxes by WidgetCache, r=?karlt

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d6136e0a045

Comment 36

[Martin Stránský [:stransky] (ni? me)]

Great, Thanks! I think we can close this now.