[webkit-reviews] review granted: [Bug 80271] [GTK] Use a single signal for script dialogs in WebKit2 GTK+ API : [Attachment 130102] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 80271: [GTK] Use a single signal for script dialogs in WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=80271

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

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


Great patch! The large change I would make would be to give WebKitScriptDialog
its own file to align with the unwritten rule of WebKit style of "one class per
file." Do you mind doing that before landing?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:149
> +    return new WebKitScriptDialog(dialog);
> +}
> +
> +static void webkitScriptDialogFree(WebKitScriptDialog* dialog)
> +{
> +    delete dialog;

Perhaps it makes sense to use the slab allocator and placement new syntax here?


> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:195
> +	   scriptDialog->confirmed = gtk_dialog_run(GTK_DIALOG(dialog)) ==
GTK_RESPONSE_OK;

You might have to use webkit_script_dialog_confirm_set_confirmed now.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:205
> +	       scriptDialog->text = gtk_entry_get_text(GTK_ENTRY(entry));

Same sort of change probably needed here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1554
> + * signal sets %TRUE when OK button is clicked and %FALSE otherwise.

Nit: the Ok button