[webkit-reviews] review denied: [Bug 212322] [GTK4] Implement file chooser : [Attachment 400733] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Santosh Mahto
<santosh.mahto at collabora.com>'s request for review:
Bug 212322: [GTK4] Implement file chooser
https://bugs.webkit.org/show_bug.cgi?id=212322

Attachment 400733: Patch

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




--- Comment #9 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 400733
  --> https://bugs.webkit.org/attachment.cgi?id=400733
Patch

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

> Source/WebCore/ChangeLog:9
> +
> +	   Added two function in GtkVersioning.h required
> +	   to make FileChooser working with gtk4.
> +
> +	   Covered by existing tests.

You should keep the Reviewed by nobody line in the changelog, it will be filled
automatically once the patch is accepted.

> Source/WebKit/ChangeLog:7
> +
> +	   Enable FileChooser launch with gtk4.
> +

Ditto.

> Source/WebCore/platform/gtk/GtkVersioning.h:23
> +#include <wtf/glib/GRefPtr.h>

We have avoided other dependencies to this file, keeping it gtk only. Maybe wtf
could be the only acceptable dep since all other layers depend on wtf.

> Source/WebCore/platform/gtk/GtkVersioning.h:166
> +    if (filename) {

gtk_file_chooser_select_filename in gtk3 doesn't allow to pass a null filename,
so we shouldn't here either. Add g_return macros to check parameters instead.

> Source/WebCore/platform/gtk/GtkVersioning.h:168
> +	   return gtk_file_chooser_select_file(chooser, file.get(), NULL);

This is also present in gtk3, so I think it's better to use
gtk_file_chooser_select_file instead of adding gtk_file_chooser_select_filename
definition.

> Source/WebCore/platform/gtk/GtkVersioning.h:178
> +    files = gtk_file_chooser_get_files(chooser);

Same here. We can just use gtk_file_chooser_get_files() for both gtk3 and gtk4.