Bug 212322 – [GTK4] Implement file chooser

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

[Carlos Garcia Campos]

It just needs to be ported

[Santosh Mahto]

Created attachment 400723 [details]
Patch

[Santosh Mahto]

Would be nice if anyone could review it, Thanks

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Adrian Perez]

Comment on attachment 400723 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        to port FileChooser seemlessly.

Typo: seemlessly → seamlessly

> Source/WebCore/platform/gtk/GtkVersioning.h:166
> +        return gtk_file_chooser_select_file(chooser, g_file_new_for_path(filename), NULL);

This leaks the GFile returned by g_file_new_for_path(), please store its return
value in a GRefPtr<GFile> before passing it to gtk_file_chooser_select_file() to
ensure that it will be unref'd properly.

> Source/WebCore/platform/gtk/GtkVersioning.h:178
> +        gchar* uri = g_file_get_path(static_cast<GFile*>(node->data));

The documentation for gtk_file_chooser_get_filenames() says:

   “If files in the current folder cannot be represented as
    local filenames they will be ignored.“

Therefore we need to check here whether the GFile is local or
not, and skip adding the non-local ones to the result list. I
think that using g_file_is_native() would fit well.

[Santosh Mahto]

(In reply to Adrian Perez from comment #4)
> Comment on attachment 400723 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400723&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        to port FileChooser seemlessly.
> 
> Typo: seemlessly → seamlessly
> 

 thanks, will update.

> > Source/WebCore/platform/gtk/GtkVersioning.h:166
> > +        return gtk_file_chooser_select_file(chooser, g_file_new_for_path(filename), NULL);
> 
> This leaks the GFile returned by g_file_new_for_path(), please store its
> return
> value in a GRefPtr<GFile> before passing it to
> gtk_file_chooser_select_file() to
> ensure that it will be unref'd properly.
> 

Thanks for pointing this.

> > Source/WebCore/platform/gtk/GtkVersioning.h:178
> > +        gchar* uri = g_file_get_path(static_cast<GFile*>(node->data));
> 
> The documentation for gtk_file_chooser_get_filenames() says:
> 
>    “If files in the current folder cannot be represented as
>     local filenames they will be ignored.“
> 
> Therefore we need to check here whether the GFile is local or
> not, and skip adding the non-local ones to the result list. I
> think that using g_file_is_native() would fit well.

As per doc g_file_get_path seems to only return localname
"Gets the local pathname for GFile, if one exists. If non-NULL, this is guaranteed to be an absolute, canonical path. It might contain symlinks.".

Even gtk3 implementation of gtk_file_chooser_get_filenames seems to be doing the same (filtering  local filename by call to g_file_get_path) :
https://gitlab.gnome.org/GNOME/gtk/-/blob/3.24.20/gtk/gtkfilechooser.c#L867

[Adrian Perez]

(In reply to Santosh Mahto from comment #5)
> (In reply to Adrian Perez from comment #4)
> >
> > [...]
> > 
> > > Source/WebCore/platform/gtk/GtkVersioning.h:178
> > > +        gchar* uri = g_file_get_path(static_cast<GFile*>(node->data));
> > 
> > The documentation for gtk_file_chooser_get_filenames() says:
> > 
> >    “If files in the current folder cannot be represented as
> >     local filenames they will be ignored.“
> > 
> > Therefore we need to check here whether the GFile is local or
> > not, and skip adding the non-local ones to the result list. I
> > think that using g_file_is_native() would fit well.
> 
> As per doc g_file_get_path seems to only return localname
> "Gets the local pathname for GFile, if one exists. If non-NULL, this is
> guaranteed to be an absolute, canonical path. It might contain symlinks.".

Good point. I should have read the documentation for g_file_get_path()
before assuming it would return non-NULL for non-local files. No need to
change this, then.

[Santosh Mahto]

Created attachment 400733 [details]
Patch

[Santosh Mahto]

Thanks Adrian Perez.
Updated the patch again with some cleanups, so request you to look again.

btw, gtk-ews fails to compile this patch, but it looks like its building wrongly, seems like USE_GTK4 is enabled but GTK3 based sdk are used. Do we have EWS enabled 
for GTK4 ?

[Carlos Garcia Campos]

Comment on attachment 400733 [details]
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.

[Carlos Garcia Campos]

(In reply to Santosh Mahto from comment #8)
> Thanks Adrian Perez.
> Updated the patch again with some cleanups, so request you to look again.
> 
> btw, gtk-ews fails to compile this patch, but it looks like its building
> wrongly, seems like USE_GTK4 is enabled but GTK3 based sdk are used. Do we
> have EWS enabled 
> for GTK4 ?

I think you need to update WebKit and rebase the patch, there's now and #else in GtkVersioning.h, it seems the new functions are added after the #else, so for GTK3. We don't EWS for GTK4 yet.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #10)
> (In reply to Santosh Mahto from comment #8)
> > Thanks Adrian Perez.
> > Updated the patch again with some cleanups, so request you to look again.
> > 
> > btw, gtk-ews fails to compile this patch, but it looks like its building
> > wrongly, seems like USE_GTK4 is enabled but GTK3 based sdk are used. Do we
> > have EWS enabled 
> > for GTK4 ?
> 
> I think you need to update WebKit and rebase the patch, there's now and
> #else in GtkVersioning.h, it seems the new functions are added after the
> #else, so for GTK3. We don't EWS for GTK4 yet.

Yes, that is exactly the problem. I tried applying the patch locally and
the new functions get added after the #else.

Santosh, could you please make sure to pull the latest changes from the
repository and update the patch? Thanks!

[Santosh Mahto]

(In reply to Carlos Garcia Campos from comment #9)
> Comment on attachment 400733 [details]
> 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.


Good idea, this way we don't need to add new functions in GtkVersioning.h, thanks for hinting that.

[Santosh Mahto]

Created attachment 400811 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 400811 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:57
>          GRefPtr<GPtrArray> filesArray = adoptGRef(g_ptr_array_new());

This is an existing issue, but I think we were leaking the filenames. I think we should use g_ptr_array_new_with_free_func(g_free)

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:60
> +            gchar* filename = g_file_get_path(static_cast<GFile*>(file->data));
> +            if (filename)

if (gchar* filename = g_file_get_path(G_FILE(file->data))

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:63
>          g_ptr_array_add(filesArray.get(), 0);

Now that we are changing this, use nullptr here instead of 0, please.

[Santosh Mahto]

(In reply to Carlos Garcia Campos from comment #14)
> Comment on attachment 400811 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400811&action=review
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:57
> >          GRefPtr<GPtrArray> filesArray = adoptGRef(g_ptr_array_new());
> 
> This is an existing issue, but I think we were leaking the filenames. I
> think we should use g_ptr_array_new_with_free_func(g_free)

Looks like you are right, we were leaking filename here, I will update patch  to use g_ptr_array_new_with_free_func.

> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:60
> > +            gchar* filename = g_file_get_path(static_cast<GFile*>(file->data));
> > +            if (filename)
> 
> if (gchar* filename = g_file_get_path(G_FILE(file->data))
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:63
> >          g_ptr_array_add(filesArray.get(), 0);
> 
> Now that we are changing this, use nullptr here instead of 0, please.

Sure will do.

[Santosh Mahto]

Created attachment 400822 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 400822 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:60
> +            gchar* filename = g_file_get_path(static_cast<GFile*>(file->data));
> +            if (filename)

if (gchar* filename = g_file_get_path(G_FILE(file->data))

[Santosh Mahto]

Created attachment 400908 [details]
Patch

[EWS]

Committed r262483: <https://trac.webkit.org/changeset/262483>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400908 [details].