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].