Bug 80271

Summary: [GTK] Use a single signal for script dialogs in WebKit2 GTK+ API
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, pnormand, webkit.review.bot
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch mrobinson: review+

Carlos Garcia Campos
Reported 2012-03-05 05:37:19 PST
As discussed in bug #17171, current signals are not bindings friendly. A single signal would simplify the API and make bindings users happier.
Attachments
Patch (29.08 KB, patch)
2012-03-05 05:44 PST, Carlos Garcia Campos
mrobinson: review+
Carlos Garcia Campos
Comment 1 2012-03-05 05:44:18 PST
WebKit Review Bot
Comment 2 2012-03-05 05:46:38 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Martin Robinson
Comment 3 2012-03-05 08:51:25 PST
Comment on attachment 130102 [details] Patch 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
Carlos Garcia Campos
Comment 4 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.
Martin Robinson
Comment 5 2012-03-05 09:18:52 PST
Comment on attachment 130102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130102&action=review >>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:149 >>> + 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. Perhaps it would make sense to use ASSERT_NOT_REACHED here then? >>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1554 >>> + * signal sets %TRUE when OK button is clicked and %FALSE otherwise. >> >> Nit: the Ok button > > Ok. Sorry, I meant "the OK button" here.
Carlos Garcia Campos
Comment 6 2012-03-05 23:52:56 PST
(In reply to comment #5) > (From update of attachment 130102 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130102&action=review > > >>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:149 > >>> + 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. > > Perhaps it would make sense to use ASSERT_NOT_REACHED here then? in this case I prefer to do the right thing in case we reach here for whatever reason. I'll change to use slab allocator before landing. > >>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1554 > >>> + * signal sets %TRUE when OK button is clicked and %FALSE otherwise. > >> > >> Nit: the Ok button > > > > Ok. > > Sorry, I meant "the OK button" here. OK :-)
Carlos Garcia Campos
Comment 7 2012-03-06 01:26:36 PST
Note You need to log in before you can comment on or make changes to this bug.