Bug 210745

Created attachment 396974 [details] Patch

Comment on attachment 396974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396974&action=review Not a GTK expert, but expert enough to review this I think. > Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:69 > + GRefPtr<GtkIconPaintable> iconPaintable = adoptGRef(gtk_icon_theme_lookup_icon(gtk_icon_theme_get_for_display(gdk_display_get_default()), "image-missing", nullptr, 16, scale, gtk_widget_get_default_direction(), static_cast<GtkIconLookupFlags>(0)); > + if (iconPaintable) { I suggest using auto and putting local variable definition inside the if > Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:71 > + GFile* file = gtk_icon_paintable_get_file(iconPaintable.get()); > + if (file) { Ditto. > Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:73 > + auto buffer = loadResourceSharedBuffer(g_file_peek_path(file)); > + icon->setData(WTFMove(buffer), true); Given there’s no failure checking here, I would have done it as a one-liner. > Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:80 > GRefPtr<GtkIconInfo> iconInfo = adoptGRef(gtk_icon_theme_lookup_icon(gtk_icon_theme_get_default(), "image-missing", iconSize, GTK_ICON_LOOKUP_NO_SVG)); > if (iconInfo) { Ditto. > Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:82 > auto buffer = loadResourceSharedBuffer(gtk_icon_info_get_filename(iconInfo.get())); > icon->setData(WTFMove(buffer), true); Ditto.

Comment on attachment 396974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396974&action=review > Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:70 > + GFile* file = gtk_icon_paintable_get_file(iconPaintable.get()); We need a GRefPtr<GFile> here to avoid leaking the object. The _get_file() accessor is “transfer full”. You could even do: if (auto file = adoptGRef(gtk_icon_paintable_get_file(iconPaintable.get()))) { // Load data from file. } =)

Very happy to see the adoptGRef recommendations.

Comment on attachment 396974 [details] Patch I would just remove this code for both GTK3 and GTK4 and always use the missingImage we already compile as a GResource.

Created attachment 397065 [details] Patch

Comment on attachment 397065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397065&action=review > Source/WebCore/platform/graphics/gtk/ImageGtk.cpp:53 > + return loadImageFromGResource(name); Oh, much nicer! Thanks Carlos for remembering that we had an image for this in the resource bundle already =)

Committed r260425: <https://trac.webkit.org/changeset/260425> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397065 [details].