Bug 15891 – [GTK] Javascript console and dialogs are not implemented

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

Currently the Gtk port doesn't implement Javascript console messages, alert, confirm or prompt at all.

[Christian Dywan]

Created attachment 17116 [details]
Implement signals

This is an implementation of the mentioned methods in the form of signals.

[Mark Rowe (bdash)]

Comment on attachment 17116 [details]
Implement signals

I talked with Christian about this on IRC and he's going to give this patch a revamp.

[Christian Dywan]

Created attachment 17132 [details]
Provide default implementations and other issues

This new patch provides default implementations for the signals when no client has connected a signal.

[Xan Lopez]

+typedef enum {
+    WEBKIT_JAVA_SCRIPT_DIALOG_ALERT,
+    WEBKIT_JAVA_SCRIPT_DIALOG_CONFIRM,
+    WEBKIT_JAVA_SCRIPT_DIALOG_PROMPT
+ } WEBKIT_JAVA_SCRIPT_DIALOG;

WebKitJavaScriptDialog.

+    if (g_signal_has_handler_pending(m_webPage, signal, 0, TRUE)) {
+        g_signal_emit(m_webPage, signal, NULL, message.utf8().data(), lineNumber, sourceId.utf8().data());

Mmmm, unless those UTF8 conversions are really expensive I'd say that's a bit too much, but maybe that's just me.


+    GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(page));

This is not totally guaranteed to return a window/toplevel, check the return value with if (GTK_WIDGET_TOPLEVEL (window)) { .... }

+    dialog = GTK_DIALOG(gtk_message_dialog_new(GTK_WINDOW(window)

In general you declare all widgets with GtkWidget * type to not do this kind of castings.

+    webkit_page_signals[JAVA_SCRIPT_CONSOLE_MESSAGE] = g_signal_new("java_script_console_message",
+            G_TYPE_FROM_CLASS(pageClass),
+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
+            0,
             ^^

You are telling g_signal_new you won't set any default handler for the signal, and then you *do* associate them (this is for all the new signals). Use G_STRUCT_OFFSET(WebKitPageClass, <vfunc>) here. Out of curiosity, does this even work? No warnings printed?

Also, why did you set alert and console-message as returning void? Doesn't that mean the default handler will be executed unless you stop the emission manually from a callback? For console-message I suppose it's ok, but why for alert?

[Christian Dywan]

(In reply to comment #4)
> +    GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(page));
> 
> This is not totally guaranteed to return a window/toplevel, check the return
> value with if (GTK_WIDGET_TOPLEVEL (window)) { .... }

You're right. I'll add a check to be on the safe side.

> +    dialog = GTK_DIALOG(gtk_message_dialog_new(GTK_WINDOW(window)
> 
> In general you declare all widgets with GtkWidget * type to not do this kind of
> castings.

I don't even know why I made dialog a GtkDialog* in the first place, I'll fix it.

> 
> +    webkit_page_signals[JAVA_SCRIPT_CONSOLE_MESSAGE] =
> g_signal_new("java_script_console_message",
> +            G_TYPE_FROM_CLASS(pageClass),
> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> +            0,
>              ^^
> 
> You are telling g_signal_new you won't set any default handler for the signal,
> and then you *do* associate them (this is for all the new signals). Use
> G_STRUCT_OFFSET(WebKitPageClass, <vfunc>) here. Out of curiosity, does this
> even work? No warnings printed?

Well, at first I did provide the handlers via G_STRUCT_OFFSET but this meant that they are always executed. And we want to allow clients to override the signals. So I left it blank here. It does work fine for me, tested with and without signals connected by a client.

> Also, why did you set alert and console-message as returning void? Doesn't that
> mean the default handler will be executed unless you stop the emission manually
> from a callback? For console-message I suppose it's ok, but why for alert?

The default handler is only executed if there are no client signals connected. I found it a bit nicer api wise to do the magic in the background so that the return values can actually be used.

[Christian Dywan]

Created attachment 17164 [details]
Style fixes and two issues pointed out by xan

A slightly improved version of the patch.

[Xan Lopez]

(In reply to comment #5)
> Well, at first I did provide the handlers via G_STRUCT_OFFSET but this meant
> that they are always executed. And we want to allow clients to override the
> signals. So I left it blank here. It does work fine for me, tested with and
> without signals connected by a client.

You don't do it like that. Make the signal have boolean return value, if the clients want to override the default handler they connect to the signal and return TRUE in their callback.

> 
> > Also, why did you set alert and console-message as returning void? Doesn't that
> > mean the default handler will be executed unless you stop the emission manually
> > from a callback? For console-message I suppose it's ok, but why for alert?
> 
> The default handler is only executed if there are no client signals connected.
> I found it a bit nicer api wise to do the magic in the background so that the
> return values can actually be used.

Same than above.

[Xan Lopez]

(In reply to comment #7)
> (In reply to comment #5)
> > Well, at first I did provide the handlers via G_STRUCT_OFFSET but this meant
> > that they are always executed. And we want to allow clients to override the
> > signals. So I left it blank here. It does work fine for me, tested with and
> > without signals connected by a client.
> 
> You don't do it like that. Make the signal have boolean return value, if the
> clients want to override the default handler they connect to the signal and
> return TRUE in their callback.
> 

For this you need to set a boolean accumulator in g_signal_new of course (g_signal_accumulator_true_handled as fifth parameter).

[Christian Dywan]

Created attachment 17184 [details]
Changed signal arguments

I agree with xan now that the default signals should be implemented similar to event handlers in that a boolean return value indicates wether a signal was handled and changed it accordingly.
I think there is a small bug with the prompt signal, but I decided to proceed with this anyway.

[Xan Lopez]

(In reply to comment #9)
> Created an attachment (id=17184) [edit]
> Changed signal arguments
> 
> I agree with xan now that the default signals should be implemented similar to
> event handlers in that a boolean return value indicates wether a signal was
> handled and changed it accordingly.
> I think there is a small bug with the prompt signal, but I decided to proceed
> with this anyway.
> 

Looks very good! Would there be any point in overriding the behavior for the console-message signal? That is, do we always want to do the LOG? If we do, then maybe it makes sense to make the signal RUN_FIRST without return value.

[Alp Toker]

+    *didConfirm = webkit_page_java_script_dialog(page, frame, message
+     , WEBKIT_JAVA_SCRIPT_DIALOG_CONFIRM, 0, 0);

I think having a linebreak in these cases is overkill given the length of other lines in this file.

Also, in ordinary writing, it should be "JavaScript", not "Java Script", though JAVA_SCRIPT is probably OK as when the name is manifested in code.

[Christian Dywan]

Created attachment 17223 [details]
Style fixes according to Alp's suggestions

I corrected the occurrences of "Java Script" and removed dubious linebreaks.

The javascript-console-message is left unchanged because I think it makes sense to override it if a client has its own means of displaying error messages.

[Alp Toker]

I think this patch is basically there. I have one last thought before we land this.

There are a bunch of people looking to support languages other than JavaScript in WebKit, and the new scripting languages will probably still support alert/confirm/prompt, so they will use the same signals.

Maybe we should refactor the naming to "script_prompt" or even just "prompt" etc.? This is a slight deviation from what WebCore does but it seems to make sense.

I got the inspiration from this Mozilla code:

    val = g_signal_connect(G_OBJECT(engine), "alert",
                           G_CALLBACK(alert_cb), self);
    g_mozilla_engine_array_insert_signal(self, val);

    val = g_signal_connect(G_OBJECT(engine), "prompt",
                           G_CALLBACK(prompt_cb), self);
    g_mozilla_engine_array_insert_signal(self, val);

    val = g_signal_connect(G_OBJECT(engine), "confirm",
                           G_CALLBACK(confirm_cb), self);
    g_mozilla_engine_array_insert_signal(self, val);

    val = g_signal_connect(G_OBJECT(engine), "confirm_ex",
                           G_CALLBACK(confirm_ex_cb), self);
    g_mozilla_engine_array_insert_signal(self, val);

http://garage.maemo.org/plugins/scmsvn/viewcvs.php/mozilla/trunk/microb-eal/src/gmozillaengine.c?root=browser&view=markup

[Christian Dywan]

Created attachment 17235 [details]
Better signal names.

As discussed in the IRC.

[Christian Dywan]

Created attachment 17236 [details]
Better signal names. (fixed comment)

Sry, quickly fixed a comment.

[Alp Toker]

Comment on attachment 17236 [details]
Better signal names. (fixed comment)

>Index: WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp	(Revision 27755)
>+++ WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp	(Arbeitskopie)
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2007 Holger Hans Peter Freyther
>+ * Copyright (C) 2007 Christian Dywan <[e-mail]>
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -195,40 +196,35 @@
> 
> void ChromeClient::addMessageToConsole(const WebCore::String& message, unsigned int lineNumber, const WebCore::String& sourceId)
> {
>-    CString messageString = message.utf8();
>-    CString sourceIdString = sourceId.utf8();
>-
>-    WEBKIT_PAGE_GET_CLASS(m_webPage)->java_script_console_message(m_webPage, messageString.data(), lineNumber, sourceIdString.data());
>+    gboolean retval;
>+    g_signal_emit_by_name(m_webPage, "console-message", message.utf8().data(), lineNumber, sourceId.utf8().data(), &retval);
> }
> 
> void ChromeClient::runJavaScriptAlert(Frame* frame, const String& message)
> {
>-    CString messageString = message.utf8();
>-    WEBKIT_PAGE_GET_CLASS(m_webPage)->java_script_alert(m_webPage, kit(frame), messageString.data());
>+    gboolean retval;
>+    g_signal_emit_by_name(m_webPage, "script-alert", kit(frame), message.utf8().data(), &retval);
> }
> 
> bool ChromeClient::runJavaScriptConfirm(Frame* frame, const String& message)
> {
>-    CString messageString = message.utf8();
>-    return WEBKIT_PAGE_GET_CLASS(m_webPage)->java_script_confirm(m_webPage, kit(frame), messageString.data());
>+    gboolean retval;
>+    gboolean didConfirm;
>+    g_signal_emit_by_name(m_webPage, "script-confirm", kit(frame), message.utf8().data(), &didConfirm, &retval);
>+    return didConfirm == TRUE;
> }
> 
> bool ChromeClient::runJavaScriptPrompt(Frame* frame, const String& message, const String& defaultValue, String& result)
> {
>-    CString messageString = message.utf8();
>-    CString defaultValueString = defaultValue.utf8(); 
>-
>-    gchar* cresult = WEBKIT_PAGE_GET_CLASS(m_webPage)->java_script_prompt(m_webPage,
>-                                                                              kit(frame),
>-                                                                              messageString.data(),
>-                                                                              defaultValueString.data());
>-    if (!cresult)
>-        return false;
>-    else {
>-        result = String::fromUTF8(cresult);
>-        g_free(cresult);
>+    gboolean retval;
>+    gchar* value = 0;
>+    g_signal_emit_by_name(m_webPage, "script-prompt", kit(frame), message.utf8().data(), defaultValue.utf8().data(), &value, &retval);
>+    if (value) {
>+        result = String::fromUTF8(value);
>+        g_free(value);
>         return true;
>     }
>+    return false;
> }
> 
> void ChromeClient::setStatusbarText(const String& string)
>Index: WebKit/gtk/ChangeLog
>===================================================================
>--- WebKit/gtk/ChangeLog	(Revision 27758)
>+++ WebKit/gtk/ChangeLog	(Arbeitskopie)
>@@ -1,3 +1,18 @@
>[phone]  Christian Dywan  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Implement signals for script dialogs and console messages.
>+
>+        * Api/webkitgtk-marshal.list:
>+        * Api/webkitgtkpage.cpp:
>+        * Api/webkitgtkpage.h:
>+        * WebCoreSupport/ChromeClientGtk.cpp:
>+        (WebKit::ChromeClient::addMessageToConsole):
>+        (WebKit::ChromeClient::runJavaScriptAlert):
>+        (WebKit::ChromeClient::runJavaScriptConfirm):
>+        (WebKit::ChromeClient::runJavaScriptPrompt):
>+
> 2007-11-11  Alp Toker  <[e-mail]>
> 
>         Reviewed by Anders.
>Index: WebKit/gtk/Api/webkitgtk-marshal.list
>===================================================================
>--- WebKit/gtk/Api/webkitgtk-marshal.list	(Revision 27755)
>+++ WebKit/gtk/Api/webkitgtk-marshal.list	(Arbeitskopie)
>@@ -1,3 +1,7 @@
> VOID:STRING,STRING
> VOID:OBJECT,BOOLEAN
> VOID:OBJECT,OBJECT
>+BOOLEAN:STRING,INT,STRING
>+BOOLEAN:OBJECT,STRING
>+BOOLEAN:OBJECT,STRING,BOOLEAN
>+BOOLEAN:OBJECT,STRING,STRING,STRING
>Index: WebKit/gtk/Api/webkitgtkpage.cpp
>===================================================================
>--- WebKit/gtk/Api/webkitgtkpage.cpp	(Revision 27755)
>+++ WebKit/gtk/Api/webkitgtkpage.cpp	(Arbeitskopie)
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2007 Holger Hans Peter Freyther
>+ * Copyright (C) 2007 Christian Dywan <[e-mail]>
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -64,6 +65,10 @@
>     STATUS_BAR_TEXT_CHANGED,
>     ICOND_LOADED,
>     SELECTION_CHANGED,
>+    CONSOLE_MESSAGE,
>+    SCRIPT_ALERT,
>+    SCRIPT_CONFIRM,
>+    SCRIPT_PROMPT,
>     LAST_SIGNAL
> };
> 
>@@ -215,33 +220,105 @@
>     return g_strdup(old_name);
> }
> 
>-static void webkit_page_real_java_script_alert(WebKitPage*, WebKitFrame*, const gchar*)
>+typedef enum {
>+    WEBKIT_SCRIPT_DIALOG_ALERT,
>+    WEBKIT_SCRIPT_DIALOG_CONFIRM,
>+    WEBKIT_SCRIPT_DIALOG_PROMPT
>+ } WebKitJavaScriptDialogType;

Please rename this enumeration to WebKitScriptDialogType.

>+
>+static gboolean webkit_page_script_dialog(WebKitPage* page, WebKitFrame* frame, const gchar* message, WebKitJavaScriptDialogType type, const gchar* defaultValue, gchar** value)
> {
>-    notImplemented();
>+    GtkMessageType messageType;
>+    GtkButtonsType buttons;
>+    gint defaultResponse;
>+    GtkWidget* window;
>+    GtkWidget* dialog;
>+    GtkWidget* entry;
>+    gboolean didConfirm;
>+
>+    switch (type) {
>+
>+    case WEBKIT_SCRIPT_DIALOG_ALERT:
>+        messageType = GTK_MESSAGE_WARNING;
>+        buttons = GTK_BUTTONS_CLOSE;
>+        defaultResponse = GTK_RESPONSE_CLOSE;
>+        break;
>+
>+    case WEBKIT_SCRIPT_DIALOG_CONFIRM:
>+        messageType = GTK_MESSAGE_QUESTION;
>+        buttons = GTK_BUTTONS_YES_NO;
>+        defaultResponse = GTK_RESPONSE_YES;
>+        break;
>+
>+    case WEBKIT_SCRIPT_DIALOG_PROMPT:
>+        messageType = GTK_MESSAGE_QUESTION;
>+        buttons = GTK_BUTTONS_OK_CANCEL;
>+        defaultResponse = GTK_RESPONSE_OK;
>+    }
>+
>+    window = gtk_widget_get_toplevel(GTK_WIDGET(page));
>+    dialog = gtk_message_dialog_new(
>+     GTK_WIDGET_TOPLEVEL(window) ? GTK_WINDOW(window) : 0
>+     , GTK_DIALOG_DESTROY_WITH_PARENT, messageType, buttons, message);
>+    gchar* title = g_strconcat("JavaScript - ", webkit_frame_get_location(frame), 0);
>+    gtk_window_set_title(GTK_WINDOW(dialog), title);
>+    g_free(title);
>+
>+    if (type == WEBKIT_SCRIPT_DIALOG_PROMPT) {
>+        entry = gtk_entry_new();
>+        gtk_entry_set_text(GTK_ENTRY(entry), defaultValue);
>+        gtk_container_add(GTK_CONTAINER(GTK_DIALOG(dialog)->vbox), entry);
>+        gtk_entry_set_activates_default(GTK_ENTRY(entry), TRUE);
>+        gtk_widget_show(entry);
>+    }
>+
>+    gtk_dialog_set_default_response(GTK_DIALOG(dialog), defaultResponse);
>+    gint response = gtk_dialog_run(GTK_DIALOG(dialog));
>+
>+    switch (response) {
>+
>+    case GTK_RESPONSE_YES:
>+        didConfirm = TRUE;
>+        break;
>+
>+    case GTK_RESPONSE_NO:
>+    case GTK_RESPONSE_CANCEL:
>+        didConfirm = FALSE;
>+        break;
>+
>+    case GTK_RESPONSE_OK:
>+        didConfirm = TRUE;
>+        *value = g_strdup(gtk_entry_get_text(GTK_ENTRY(entry)));
>+    }
>+    gtk_widget_destroy(GTK_WIDGET(dialog));
>+    return didConfirm;
> }
> 
>-static gboolean webkit_page_real_java_script_confirm(WebKitPage*, WebKitFrame*, const gchar*)
>+static gboolean webkit_page_real_script_alert(WebKitPage* page, WebKitFrame* frame, const gchar* message)
> {
>-    notImplemented();
>-    return FALSE;
>+    webkit_page_script_dialog(page, frame, message, WEBKIT_SCRIPT_DIALOG_ALERT, 0, 0);
>+    return TRUE;
> }
> 
>-/**
>- * WebKitPage::java_script_prompt
>- *
>- * @return: NULL to cancel the prompt
>- */
>-static gchar* webkit_page_real_java_script_prompt(WebKitPage*, WebKitFrame*, const gchar*, const gchar* defaultValue)
>+static gboolean webkit_page_real_script_confirm(WebKitPage* page, WebKitFrame* frame, const gchar* message, gboolean* didConfirm)
> {
>-    notImplemented();
>-    return g_strdup(defaultValue);
>+    *didConfirm = webkit_page_script_dialog(page, frame, message, WEBKIT_SCRIPT_DIALOG_CONFIRM, 0, 0);
>+    return TRUE;
> }
> 
>-static void webkit_page_real_java_script_console_message(WebKitPage*, const gchar*, unsigned int, const gchar*)
>+static gboolean webkit_page_real_script_prompt(WebKitPage* page, WebKitFrame* frame, const gchar* message, const gchar* defaultValue, gchar** value)
> {
>-    notImplemented();
>+    if (!webkit_page_script_dialog(page, frame, message, WEBKIT_SCRIPT_DIALOG_PROMPT, defaultValue, value))
>+        *value = g_strdup(defaultValue);
>+    return TRUE;
> }
> 
>+static gboolean webkit_page_real_console_message(WebKitPage* page, const gchar* message, unsigned int line, const gchar* sourceId)
>+{
>+    LOG("console-message: %s@%d: %s\n", sourceId, line, message);
>+    return TRUE;
>+}
>+
> static void webkit_page_finalize(GObject* object)
> {
>     webkit_page_stop_loading(WEBKIT_PAGE(object));
>@@ -354,17 +431,97 @@
>             g_cclosure_marshal_VOID__VOID,
>             G_TYPE_NONE, 0);
> 
>+    /**
>+     * WebKitPage::console-message
>+     * @page: the object on which the signal is emitted
>+     * @message: the message text
>+     * @line: the line where the error occured
>+     * @source_id: the source id
>+     * @return: TRUE to stop other handlers from being invoked for the event. FALSE to propagate the event further.
>+     *
>+     * A JavaScript console message was created.
>+     */
>+    webkit_page_signals[CONSOLE_MESSAGE] = g_signal_new("console_message",
>+            G_TYPE_FROM_CLASS(pageClass),
>+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+            G_STRUCT_OFFSET(WebKitPageClass, console_message),
>+            g_signal_accumulator_true_handled,
>+            NULL,
>+            webkit_marshal_BOOLEAN__STRING_INT_STRING,
>+            G_TYPE_BOOLEAN, 3,
>+            G_TYPE_STRING, G_TYPE_INT, G_TYPE_STRING);
> 
>+    /**
>+     * WebKitPage::script-alert
>+     * @page: the object on which the signal is emitted
>+     * @frame: the relevant frame
>+     * @message: the message text
>+     * @return: TRUE to stop other handlers from being invoked for the event. FALSE to propagate the event further.
>+     *
>+     * A JavaScript alert dialog was created.
>+     */
>+    webkit_page_signals[SCRIPT_ALERT] = g_signal_new("alert",
>+            G_TYPE_FROM_CLASS(pageClass),
>+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+            G_STRUCT_OFFSET(WebKitPageClass, script_alert),
>+            g_signal_accumulator_true_handled,
>+            NULL,
>+            webkit_marshal_BOOLEAN__OBJECT_STRING,
>+            G_TYPE_BOOLEAN, 2,
>+            G_TYPE_OBJECT, G_TYPE_STRING);

You forgot to rename "alert" here, so it doesn't work. Should be "script_alert"?

[Christian Dywan]

Created attachment 17237 [details]
Better signal names (take three)

Fixed according to Alp's comment.

[Christian Dywan]

Created attachment 17238 [details]
Better signal names (take four)

[Alp Toker]

Doesn't work with the test case at http://www.fluffycat.com/JavaScript/Prompt/

Always seems to return null.

[Christian Dywan]

Created attachment 17242 [details]
Fluffy cat compatible

Apparently Glib doesn't count gchar** as a string, type changed to pointer and the example works.

[Alp Toker]

Comment on attachment 17242 [details]
Fluffy cat compatible

Hitting "Cancel" should not return the default value, but instead null, based on the behaviour of other browsers. I believe the only purpose of the default value is as a convenience to the user, not as a default return value for the prompt().

[Alp Toker]

Comment on attachment 17242 [details]
Fluffy cat compatible

I'm landing this patch since it's basically fine except for the Cancel issue. Please resolve this issue in a further patch ASAP so we can close this bug. Thanks!

[Alp Toker]

Patch landed in r27782. Waiting on the Cancel bug fix before closing this bug for good.

[Christian Dywan]

Created attachment 17324 [details]
Correct script prompt return value and avoid warrings.

This changes the behavior of the script-prompt to return NULL when the dialog
is cancelled. This matches other browsers, tested with Firefox and Opera on a
number of websites.

Besides I changed a bit of the dialog code to avoid compiler warnings regarding
uninitialized values.

At last I modifed the console message default to prevent linkification in
terminals to include the line number in the link.

[Xan Lopez]

(In reply to comment #24)
-    GtkMessageType messageType;
-    GtkButtonsType buttons;
-    gint defaultResponse;
+    GtkMessageType messageType = GTK_MESSAGE_OTHER;
+    GtkButtonsType buttons = GTK_BUTTONS_NONE;
+    gint defaultResponse = 0;

I'd rather do this in the "default:" of the switch or, better, warn there that something weird is going on and maybe return just there.

-    dialog = gtk_message_dialog_new(
-     GTK_WIDGET_TOPLEVEL(window) ? GTK_WINDOW(window) : 0
-     , GTK_DIALOG_DESTROY_WITH_PARENT, messageType, buttons, message);
-    gchar* title = g_strconcat("JavaScript - ", webkit_frame_get_location(frame), 0);
+    dialog = gtk_message_dialog_new(GTK_WIDGET_TOPLEVEL(window) ? GTK_WINDOW(window) : 0, GTK_DIALOG_DESTROY_WITH_PARENT, messageType, buttons, message);
+    gchar* title = g_strconcat("JavaScript - ", webkit_frame_get_location(frame), (gchar*)0);

Keeping the code at ~80 columns is nicer I'd say. And can we use NULL instead of  casting 0 to gchar*?

-    case GTK_RESPONSE_NO:
-    case GTK_RESPONSE_CANCEL:
-        didConfirm = FALSE;
-        break;
-

Even if didConfirm = FALSE is default I'd let the assignment for GTK_RESPONSE_NO, it's more clear (I assume we still want it for NO).

[Christian Dywan]

Created attachment 17332 [details]
Correct script prompt return value and avoid warrings, Take Two

Updated patch after discussion with xan.

[Alp Toker]

Comment on attachment 17332 [details]
Correct script prompt return value and avoid warrings, Take Two

I think we are all OK with the NULL use here. r=me

[Alp Toker]

Fix landed in r27888. Please open a new bug if there are further issues. Thanks!

[Alp Toker]

Looks like we made the right call on "Script" instead of "JavaScript":

 *
 * Revision 6 (March 29, 2004):
 * Renamed functions implemented by user agent to NPN_*.  Removed _ from
 * type names.
 * Renamed "JavaScript" types to "Script".
 *

That's from npruntime.h. Looks like they reached the same conclusion we did.

[Christian Dywan]

Created attachment 17492 [details]
Fix call to gtk_message_dialog_new which expects a format string.

We need to prevent messages with format tokens from crashing WebKit.

[Alp Toker]

Fix landed in r28008.