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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied 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 144635: Patch
https://bugs.webkit.org/attachment.cgi?id=144635&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144635&action=review


Thanks for writing this patch. It actually looks very much like it's ready to
go. There are a few stylistic things that I think should be changed before that
though. I always feel a bit bad giving a good patch an r- for mostly stylistic
reasons, but it's important that we follow the rules that the community agreed
on (otherwise chaos would ensue, undoubtedly). 

> Source/WebKit/gtk/tests/testwebview.c:509
> +static gboolean check_mime_type_for_filter(GtkFileFilter* filter, const
gchar* mime_type)

Please use WebKit coding style when writing C files. See the style rules here:
http://www.webkit.org/coding/coding-style.html Many of the existing tests were
written before we followed these rules as well as we do now. We try to ensure
that all new code follows the style rules though.

Note that C files have the bizarre requirement of putting the asterisk next to
the variable name, unlike C++ files. Variable names should be like mimeType
instead of mime_type.

> Source/WebKit/gtk/tests/testwebview.c:517
> +static gboolean run_file_chooser_cb_1(WebKitWebView* webview,
WebKitFileChooserRequest* request, gpointer data)

This names run_file_chooser_cb_1 and run_file_chooser_cb_2 don't really say
much about what these callbacks actually do. Consider using names that better
describe the what each callback does.

> Source/WebKit/gtk/tests/testwebview.c:522
> +    const gchar* const* mime_types;
> +    const gchar* const* selected_files;
> +    GtkFileFilter* filter;
> +

We don't typically declare variable before we use them like this in WebKit.
Please make this change throughout your patch.

> Source/WebKit/gtk/tests/testwebview.c:670
> +    webkit_web_view_load_string(WEBKIT_WEB_VIEW(web_view), html_format,
NULL, NULL, NULL);
> +    g_free(html_format);
> +
> +    g_timeout_add(100, (GSourceFunc)
click_mouse_button_and_wait_for_file_chooser_request,
WEBKIT_WEB_VIEW(web_view));

Instead of using a timeout here it would be better to run the main loop and
wait until loading it complete.

> Source/WebKit/gtk/tests/testwebview.c:694
> +    // Multiple selections not allowed, only accept images, audio and video
files..

Nit: I think you have an extra period here.

> Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:45
> + * some cases, which could prefer to use their own file chooser

"cases , which could prefer" sounds a bit odd here. Perhaps cases, such as when
an embedding applications prefers to use its own"

> Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:144
> +							
WEBKIT_PARAM_READABLE));    /**

Your comment seems to joined to the previous line here. Are you a vim user? :)

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1318
> +static gboolean webkit_web_view_real_run_file_chooser(WebKitWebView*
webView, WebKitFileChooserRequest* request)

You should use camelCase for this method name since it isn't exposed in the
public API.