Bug 212322 - [GTK4] Implement file chooser
Summary: [GTK4] Implement file chooser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2020-05-24 05:50 PDT by Carlos Garcia Campos
Modified: 2020-06-03 03:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.19 KB, patch)
2020-06-01 00:34 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2020-06-01 07:44 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (3.68 KB, patch)
2020-06-02 06:00 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2020-06-02 08:25 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (3.79 KB, patch)
2020-06-03 02:17 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-05-24 05:50:01 PDT
It just needs to be ported
Comment 1 Santosh Mahto 2020-06-01 00:34:38 PDT
Created attachment 400723 [details]
Patch
Comment 2 Santosh Mahto 2020-06-01 00:35:17 PDT
Would be nice if anyone could review it, Thanks
Comment 3 EWS Watchlist 2020-06-01 00:35:39 PDT
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
Comment 4 Adrian Perez 2020-06-01 02:10:15 PDT
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.
Comment 5 Santosh Mahto 2020-06-01 03:49:28 PDT
(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
Comment 6 Adrian Perez 2020-06-01 06:56:19 PDT
(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.
Comment 7 Santosh Mahto 2020-06-01 07:44:38 PDT
Created attachment 400733 [details]
Patch
Comment 8 Santosh Mahto 2020-06-01 07:53:35 PDT
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 ?
Comment 9 Carlos Garcia Campos 2020-06-02 01:13:50 PDT
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.
Comment 10 Carlos Garcia Campos 2020-06-02 01:15:29 PDT
(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.
Comment 11 Adrian Perez 2020-06-02 02:24:02 PDT
(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!
Comment 12 Santosh Mahto 2020-06-02 05:11:12 PDT
(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.
Comment 13 Santosh Mahto 2020-06-02 06:00:24 PDT
Created attachment 400811 [details]
Patch
Comment 14 Carlos Garcia Campos 2020-06-02 06:15:12 PDT
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.
Comment 15 Santosh Mahto 2020-06-02 07:07:26 PDT
(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.
Comment 16 Santosh Mahto 2020-06-02 08:25:49 PDT
Created attachment 400822 [details]
Patch
Comment 17 Carlos Garcia Campos 2020-06-03 02:00:57 PDT
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))
Comment 18 Santosh Mahto 2020-06-03 02:17:16 PDT
Created attachment 400908 [details]
Patch
Comment 19 EWS 2020-06-03 03:08:25 PDT
Committed r262483: <https://trac.webkit.org/changeset/262483>

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