[Webkit-unassigned] [Bug 87283] [GTK] run-file-chooser signal for WebKit1

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=87283


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #144635|review?                     |review-
               Flag|                            |




--- Comment #19 from Martin Robinson <mrobinson at webkit.org>  2012-06-08 15:51:10 PST ---
(From update of attachment 144635)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.