[Webkit-unassigned] [Bug 80271] [GTK] Use a single signal for script dialogs in WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-03-05 09:16:07 PST ---
(In reply to comment #3)
> (From update of attachment 130102 [details])
> 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?

Sure, no problem, even though WebKitScriptDialog is not a class, but a struct.

> > 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?

It doesn't really matter, this will never be used, but we need to give a copy and free func for boxed types. The struct is stack allocated before emitting the signal and the signal parameter is  G_SIGNAL_TYPE_STATIC_SCOPE.

> > 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.

Right, if we move the struct to its own file. 

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:205
> > +            scriptDialog->text = gtk_entry_get_text(GTK_ENTRY(entry));
> 
> Same sort of change probably needed here.

Yes.

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

Ok.

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