[webkit-reviews] review canceled: [Bug 78491] [GTK] Implement WebUIClient's runOpenPanel in WebKit2GTK+ : [Attachment 127815] Patch Proposal + Unit Tests + Docs

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has canceled Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 78491: [GTK] Implement WebUIClient's runOpenPanel in WebKit2GTK+
https://bugs.webkit.org/show_bug.cgi?id=78491

Attachment 127815: Patch Proposal + Unit Tests + Docs
https://bugs.webkit.org/attachment.cgi?id=127815&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(In reply to comment #38)
> [...]
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:200
> > + * Returns: #TRUE if the file chooser should allow selecting multiple
files or #FALSE otherwise.
> 
> Use %TRUE %FALSE instead of #TRUE #FALSE.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:219
> > + * Returns: (array zero-terminated=1) (transfer none): a
> > + * NULL-terminated array of strings if a list of accepted MIME types
> > + * is defined or NULL otherwise. This array and its contents are owned
> > + * by WebKitGTK+ and should not be modified or freed.
> 
> %NULL

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:251
> > + * MIME types is defined or NULL otherwise. The returned object is
> 
> %NULL

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:265
> > +	 request->priv->filter = gtk_file_filter_new();
> 
> adoptGRef(gtk_file_filter_new());

Not possible. Since GtkFileFilter derives from GInitiallyUnowned,
gtk_file_filter_new() returns a floating object, so we need to get
g_object_ref_sink() not to fail when calling the derefPtr method (right after
finishing the assignment). Calling adoptGRef avoids calling g_object_ref_sink
and so it's not possible in this case.

I put a comment explaining the situation, right before that line.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:313
> > + * associated with the request or NULL otherwise. This array and its
> 
> %NULL

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:323
> > +	 Vector<String> selectedFileNames =
toImpl(request->priv->wkParameters.get())->selectedFileNames();
> 
> We are not going to modify the Vector, would it be possible to get a const
reference?

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:179
> > +	     GOwnPtr<GSList>
filesList(gtk_file_chooser_get_filenames(GTK_FILE_CHOOSER(dialog)));
> > +	     guint numOfFiles = g_slist_length(filesList.get());
> > +
> > +	     const gchar** filesArray = g_new0(const gchar*, numOfFiles + 1);
> > +	     GSList* file = filesList.get();
> > +	     for (guint i = 0; i < numOfFiles; i++) {
> > +		 filesArray[i] = static_cast<const gchar*>(file->data);
> > +		 file = g_slist_next(file);
> > +	     }
> > +
> > +	     webkit_file_chooser_request_choose_files(adoptedRequest.get(),
const_cast<const gchar* const*>(filesArray));
> > +	     g_free(filesArray);
> 
> I think this would be simpler using a GPtrArray
> 
> GRefPtr<GPtrArray> filesArray = adoptGRef(g_ptr_array_new());
> for (GSList* l = filesList.get(); l; l = g_slist_next(l))
>     g_ptr_array_add(filesArray.get(), l->data);
> webkit_file_chooser_request_choose_files(adoptedRequest.get(),
reinterpret_cast<gchar**>(filesArray->pdata));

You lack a g_ptr_array_add(filesArray.get(), 0); right after the loop
(webkit_file_chooser_request_choose_files expects a NULL-terminated array of
strings), but otherwise I agree with you.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:200
> > +	 GRefPtr<GtkFileFilter>
filter(webkit_file_chooser_request_get_mime_types_filter(request));
> 
> You don't need a GRefPtr in this case, since get_mime_types_filter returns a
transfer none, you know the filter will be valid while the request is alive.

You're right. Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:206
> > +	 if (selectedFiles && selectedFiles[0] && *selectedFiles[0])
> 
> I think we should ignore empty files when adding items to the selected files
array, so that the user doesn't need to check this.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:858
> > +void webkitWebViewMakeFileChooserRequest(WebKitWebView* webView,
WebKitFileChooserRequest* request)
> 
> webkitWebViewRunFileChooser()

I renamed it to webkitWebViewRunFileChooserRequest instead, since
webkitWebViewRunFileChooser was a function name already in use for the default
handler.