Bug 15891 - [GTK] Javascript console and dialogs are not implemented
Summary: [GTK] Javascript console and dialogs are not implemented
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-11-07 22:33 PST by Christian Dywan
Modified: 2007-11-24 17:24 PST (History)
1 user (show)

See Also:


Attachments
Implement signals (8.37 KB, patch)
2007-11-07 22:40 PST, Christian Dywan
mrowe: review-
Details | Formatted Diff | Diff
Provide default implementations and other issues (13.03 KB, patch)
2007-11-08 15:54 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Style fixes and two issues pointed out by xan (13.42 KB, patch)
2007-11-09 19:29 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Changed signal arguments (13.61 KB, patch)
2007-11-11 06:14 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Style fixes according to Alp's suggestions (13.58 KB, patch)
2007-11-13 01:58 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Better signal names. (14.89 KB, patch)
2007-11-13 12:29 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Better signal names. (fixed comment) (14.89 KB, patch)
2007-11-13 12:32 PST, Christian Dywan
alp: review-
Details | Formatted Diff | Diff
Better signal names (take three) (14.89 KB, patch)
2007-11-13 13:07 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Better signal names (take four) (14.89 KB, patch)
2007-11-13 13:10 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Fluffy cat compatible (14.89 KB, patch)
2007-11-13 14:54 PST, Christian Dywan
alp: review+
Details | Formatted Diff | Diff
Correct script prompt return value and avoid warrings. (2.95 KB, patch)
2007-11-16 19:49 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Correct script prompt return value and avoid warrings, Take Two (3.59 KB, patch)
2007-11-17 17:02 PST, Christian Dywan
alp: review+
Details | Formatted Diff | Diff
Fix call to gtk_message_dialog_new which expects a format string. (1.30 KB, patch)
2007-11-24 17:04 PST, Christian Dywan
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2007-11-07 22:33:59 PST
Currently the Gtk port doesn't implement Javascript console messages, alert, confirm or prompt at all.
Comment 1 Christian Dywan 2007-11-07 22:40:57 PST
Created attachment 17116 [details]
Implement signals

This is an implementation of the mentioned methods in the form of signals.
Comment 2 Mark Rowe (bdash) 2007-11-08 03:20:23 PST
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.
Comment 3 Christian Dywan 2007-11-08 15:54:21 PST
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.
Comment 4 Xan Lopez 2007-11-09 17:40:13 PST
+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?
Comment 5 Christian Dywan 2007-11-09 19:12:11 PST
(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.
Comment 6 Christian Dywan 2007-11-09 19:29:41 PST
Created attachment 17164 [details]
Style fixes and two issues pointed out by xan

A slightly improved version of the patch.
Comment 7 Xan Lopez 2007-11-10 01:48:33 PST
(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.


Comment 8 Xan Lopez 2007-11-10 02:12:32 PST
(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).
Comment 9 Christian Dywan 2007-11-11 06:14:37 PST
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.
Comment 10 Xan Lopez 2007-11-11 13:57:38 PST
(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.
Comment 11 Alp Toker 2007-11-12 04:48:36 PST
+    *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.
Comment 12 Christian Dywan 2007-11-13 01:58:53 PST
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.
Comment 13 Alp Toker 2007-11-13 04:15:22 PST
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
Comment 14 Christian Dywan 2007-11-13 12:29:13 PST
Created attachment 17235 [details]
Better signal names.

As discussed in the IRC.
Comment 15 Christian Dywan 2007-11-13 12:32:36 PST
Created attachment 17236 [details]
Better signal names. (fixed comment)

Sry, quickly fixed a comment.
Comment 16 Alp Toker 2007-11-13 13:01:10 PST
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 <christian@twotoasts.de>
>  *
>  * 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 @@
>+2007-11-13  Christian Dywan  <christian@twotoasts.de>
>+
>+        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  <alp@atoker.com>
> 
>         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 <christian@twotoasts.de>
>  *
>  * 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"?
Comment 17 Christian Dywan 2007-11-13 13:07:51 PST
Created attachment 17237 [details]
Better signal names (take three)

Fixed according to Alp's comment.
Comment 18 Christian Dywan 2007-11-13 13:10:16 PST
Created attachment 17238 [details]
Better signal names (take four)
Comment 19 Alp Toker 2007-11-13 13:35:21 PST
Doesn't work with the test case at http://www.fluffycat.com/JavaScript/Prompt/

Always seems to return null.
Comment 20 Christian Dywan 2007-11-13 14:54:00 PST
Created attachment 17242 [details]
Fluffy cat compatible

Apparently Glib doesn't count gchar** as a string, type changed to pointer and the example works.
Comment 21 Alp Toker 2007-11-13 15:13:41 PST
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().
Comment 22 Alp Toker 2007-11-13 21:40:15 PST
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!
Comment 23 Alp Toker 2007-11-13 21:41:38 PST
Patch landed in r27782. Waiting on the Cancel bug fix before closing this bug for good.
Comment 24 Christian Dywan 2007-11-16 19:49:23 PST
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.
Comment 25 Xan Lopez 2007-11-17 15:34:15 PST
(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).
Comment 26 Christian Dywan 2007-11-17 17:02:12 PST
Created attachment 17332 [details]
Correct script prompt return value and avoid warrings, Take Two

Updated patch after discussion with xan.
Comment 27 Alp Toker 2007-11-18 03:30:57 PST
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
Comment 28 Alp Toker 2007-11-18 03:31:36 PST
Fix landed in r27888. Please open a new bug if there are further issues. Thanks!
Comment 29 Alp Toker 2007-11-19 10:38:42 PST
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.
Comment 30 Christian Dywan 2007-11-24 17:04:27 PST
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.
Comment 31 Alp Toker 2007-11-24 17:24:08 PST
Fix landed in r28008.