[webkit-reviews] review granted: [Bug 87283] [GTK] run-file-chooser signal for WebKit1 : [Attachment 147465] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Daniel Drake
<dsd at laptop.org>'s request for review:
Bug 87283: [GTK] run-file-chooser signal for WebKit1
https://bugs.webkit.org/show_bug.cgi?id=87283

Attachment 147465: Patch
https://bugs.webkit.org/attachment.cgi?id=147465&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147465&action=review


Thanks for your work! Looks good to me, but have a few comments, probably needs
a new patch uploaded since you're not a committer, I guess?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2605
> +	* This signal is emitted when the user interacts with a <input
> +	* type='file' /> HTML element, requesting from WebKit to show
> +	* a dialog to select one or more files to be uploaded. To let the
> +	* application know the details of the file chooser, as well as to
> +	* allow the client application to either cancel the request or
> +	* perform an actual selection of files, the signal will pass an
> +	* instance of the #WebKitFileChooserRequest in the @request
> +	* argument.

I miss a comment about the lifecycle of the FileChooserRequest object and how
to handle this async. If I want to handle this signal asynchronously, do I ref
the request, return TRUE and then call its select_files() method when I have
the chosen files? If that's the case (and it seems to be from my quick
inspection of the default handler), then I think the API looks good, but having
this spelled out here would help =).

> Source/WebKit/gtk/webkit/webkitwebview.h:170
> +    gboolean 		  (* run_file_chooser)	     (WebKitWebView    
       *web_view,
> +							     
WebKitFileChooserRequest *request);

Should go at the bottom and consume the padding.

> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:140
> +void webkit_web_view_run_file_chooser_request(WebKitWebView*,
WebKitFileChooserRequest*);

This function should use camelCase like the other internal non-public ones.