[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





--- Comment #9 from Mario Sanchez Prada <msanchez at igalia.com>  2012-05-29 03:12:49 PST ---
(From update of attachment 144053)
View in context: https://bugs.webkit.org/attachment.cgi?id=144053&action=review

Informally reviewing the patch, which overall looks good to me. I've mostly commented on he need of keeping the webkit_file_chooser_request_cancel() function in the API and some coding style issues. Hope this helps.

See my comments below...

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:365
> +     */

I'm not sure whether it's useful to keep this webkit_file_chooser_request_cancel() function in WebKit1 if we don't need it after all. For WebKit2 is needed in order to tell the Web process that the request have been cancelled so it does not keep waiting forever, but that's not an issue in WK1 (and the reason, I think, why is not needed).

So, I think I would remove this function from the API in WK1, even if that's not consistent with WK2 API. After all, both APIs are similar but not identical anyway.

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitfilechooserrequest.h:70
> +webkit_file_chooser_request_cancel                (WebKitFileChooserRequest *request);

Same here. I would remove this one, although I'd appreciate others' opinion on this topic, too.

Carlos, what do you think?

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitwebview.cpp:1315
> +        webkit_file_chooser_request_cancel(adoptedRequest.get());

Make sure to remove this else branch if we finally agree on removing webkit_file_chooser_request_cancel() from the API

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitwebview.cpp:1338
> +    gtk_file_chooser_set_select_multiple(GTK_FILE_CHOOSER(dialog), allowsMultipleSelection);

This line seems to be duplicated.

Also, I think this is a good place to ask the WebKitFileChooserRequest object whether it already has a non-empty list of files selected that we can use to pre-select them in the file chooser dialog when opening (actually we just can pre-select one file). This is what we do in WK2:

    if (const gchar* const* selectedFiles = webkit_file_chooser_request_get_selected_files(request))
        gtk_file_chooser_select_filename(GTK_FILE_CHOOSER(dialog), selectedFiles[0]);

Hence, I would do the same here.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:509
> +static gboolean checkMimeTypeForFilter(GtkFileFilter* filter, const gchar* mimeType)

Coding style issue: in the diff for this file you seem to use sometimes thisKindOfNamingStyle for functions, local variables and parameters, and some times this_other_naming_style, and that is wrong as you should be consistent.

In theory the former (camel case) is the commonly agreed coding style for newly introduced code in WK *but*, as (1) unit tests are an exception in some ways to coding style, as (2) 99% of this file seems to be already written using this_other_naming_style and as (3) we don't expect much more changes in this file from now on... I'd say that these are good reasons to stick to this_other_naming_style here.

Which is a long way of saying that you should be consistent with the code introduced in this patch and that you should rename this function name and parameters to something like this:

   static gboolean check_mime_type_for_filter(GtkFileFilter* filter, const gchar* mime_type)

Others might disagree (and I would also if we were thinking of adding many more changes in the future to this file), but I think that it's pretty reasonable to do it this time according to reasons (1), (2) and (3).

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:511
> +    GtkFileFilterInfo filterInfo;

Don't use camel case style here: filterInfo -> filter_info

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:520
> +    const gchar* const* selectedFiles;

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:521
> +    GtkFileFilter *filter;

Coding style issue: Misplaced '*'. It should be 'GtkFileFilter* filter;'

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:531
> +    webkit_file_chooser_request_cancel(request);

Same than before: remove this line if we agree on removing the _cancel() function from the API

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:540
> +    const gchar* const* selectedFiles;

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:541
> +    GtkFileFilter *filter;

Misplaced '*'.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:570
> +    const gchar* const* selectedFiles;

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:578
> +    webkit_file_chooser_request_cancel(request);

Remove this line if agreed

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:588
> +    GtkFileFilter *filter;

Misplaced '*'.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:607
> +    webkit_file_chooser_request_cancel(request);

Put my "usual comment" here :)

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:613
> +void doMouseButtonEvent(GtkWidget *widget, GdkEventType event_type, int x, int y, unsigned int button, unsigned int modifiers)

Don't use camel case style here: doMouseButtonEvent -> do_mouse_button_event

Also, misplaced '*' for widget.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:619
> +    GdkEvent *event = gdk_event_new(event_type);

Misplaced '*'.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:638
> +static void clickMouseButton(GtkWidget *widget, int x, int y, unsigned int button, unsigned int modifiers)

Don't use camel case style here. Misplaced '*' for widget

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:644
> +static gboolean clickMouseButtonAndWaitForFileChooserRequest(WebKitWebView* webView)

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:655
> +    gchar *html_format;

Misplaced '*'.in these two variables: html_format_base and html_format.

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