Bug 212322

Created attachment 400723 [details] Patch

Would be nice if anyone could review it, Thanks

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

(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

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

Created attachment 400733 [details] Patch

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

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

(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!

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

Created attachment 400811 [details] Patch

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.

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

Created attachment 400822 [details] Patch

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))

Created attachment 400908 [details] Patch

Committed r262483: <https://trac.webkit.org/changeset/262483> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400908 [details].