Bug 150082 – [GTK] Use GUniquePtr for GtkIconInfo

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

[ChangSeok Oh]

SSIA

[ChangSeok Oh]

Created attachment 262977 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 262977 [details]
Patch

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

Thanks for the patch. This is not enough, though. With this patch the GtkIconInfo will be freed with g_free, not with gtk_icon_info_free(), You need to also include GUniquePtrGtk.h.

> Source/WebCore/rendering/RenderThemeGtk.cpp:242
>      GdkPixbuf* icon = 0;

Please change this to nullptr.

> Source/WebCore/rendering/RenderThemeGtk.cpp:244
> +        icon = gtk_icon_info_load_symbolic_for_context(info.get(), context, 0, 0);

nullptr, nullptr

[ChangSeok Oh]

(In reply to comment #2)
> With this patch the GtkIconInfo will be freed with g_free, not with gtk_icon_info_free(), You need to also include GUniquePtrGtk.h.
Thanks for your review. =) I am not sure of this comment. We already have the gtk_icon_info_free in GUniquePtrGtk.h So the smart pointer has been used in ImageGtk.cpp

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > With this patch the GtkIconInfo will be freed with g_free, not with gtk_icon_info_free(), You need to also include GUniquePtrGtk.h.
> Thanks for your review. =) I am not sure of this comment. We already have
> the gtk_icon_info_free in GUniquePtrGtk.h So the smart pointer has been used
> in ImageGtk.cpp

ImageGtk.cpp includes GUniquePtrGtk.h, but RenderThemeGtk.cpp doesn't.

[ChangSeok Oh]

Created attachment 262981 [details]
Patch

[ChangSeok Oh]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > With this patch the GtkIconInfo will be freed with g_free, not with gtk_icon_info_free(), You need to also include GUniquePtrGtk.h.
> > Thanks for your review. =) I am not sure of this comment. We already have
> > the gtk_icon_info_free in GUniquePtrGtk.h So the smart pointer has been used
> > in ImageGtk.cpp
> 
> ImageGtk.cpp includes GUniquePtrGtk.h, but RenderThemeGtk.cpp doesn't.

Oh yeah. You are right.

[WebKit Commit Bot]

Comment on attachment 262981 [details]
Patch

Clearing flags on attachment: 262981

Committed r190970: <http://trac.webkit.org/changeset/190970>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.