[webkit-reviews] review granted: [Bug 210745] [GTK4] Adapt to GtkIconTheme API changes : [Attachment 396974] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 210745: [GTK4] Adapt to GtkIconTheme API changes
https://bugs.webkit.org/show_bug.cgi?id=210745

Attachment 396974: Patch

https://bugs.webkit.org/attachment.cgi?id=396974&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 396974
  --> https://bugs.webkit.org/attachment.cgi?id=396974
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.