Bug 19130 – [GTK] ChromeClient::createWindow and friends need to be implemented

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

To allow for javascript, for instance, creating new windows, we need some ChromeClientGtk methods implemented.

[Gustavo Noronha (kov)]

Created attachment 21244 [details]
my proposed solution

some stuff I wondered about: should create_web_view or the signal that is emited be passed a WebKitNetworkRequest for them to make a policy decision, and let one of those call the actual webkit_web_view_open()?

[Marco Barisione]

We worked the same patch in the same days :(

Your patch seems better but it's missing a couple of things and there are other things I would like to change.
Can you ping me on IRC so we can discuss this? My nick is barisione.

[Marco Barisione]

*** Bug 19143 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Comment on attachment 21244 [details]
my proposed solution

removing the review flag, since I'll be working with barisione on merging our patches (see #19143)

[Gustavo Noronha (kov)]

Created attachment 21341 [details]
patch merged with work by me and barisione

I'm adding the patch with no review flag set, for now, so that Marco Barisione is able to review it first, since it is a merge of both my work and his.

[Marco Barisione]

(In reply to comment #5)
> Created an attachment (id=21341) [edit]
> patch merged with work by me and barisione

Some comments on the patch:

- In webkit_web_view_real_create_web_view() and webkit_web_view_real_show_web_view() you can omit param names as they are not used.

- It's not clear from the documentation who is the owner of the object returned by "create-web-view". IMHO we are doing the right thing without calling g_object_ref in the callback as the reference will be owned by the window, but then we have to document this and to get the reference count right (i.e. use g_value_take object in the accumulator/unref the value returned by the emission of the signal).

- Use the exact type when registering new signals, not just G_TYPE_OBJECT.

- How about replacing locationbar with uribar? We use uri in every other place, but I don't know what I prefer. In both cases I would prefer to separate bar from the previous word but then it wouldn't be consistent with the other names.

- The default value for resizable should be TRUE.

- The default value for fullscreen should be FALSE.

- The default value for dialog should be FALSE.

- WebKitWebFeatures doesn't have functions to access directly properties but probably it's better this way than adding a lot of getters/setters.

- g_object_notify() is needed only in functions setting values not in the implementation of set_property as g_object_set_property already emits it.

- Change the _set_window_features() function to set it only if it's different from the previous one and to use g_object_notify() after setting it. I know that other properties are not doing it but there is a bug open about this (see bug #18405).

[Marco Barisione]

I forgot a couple of things:

- You don't need to use webkit_web_view_open() in the callback, WebCore will do it for you.

- When your patch is ready for review remove the code from main.c, maybe it could go in a different patch used for testing and without the review flag set.

[Gustavo Noronha (kov)]

> - It's not clear from the documentation who is the owner of the object returned
> by "create-web-view". IMHO we are doing the right thing without calling
> g_object_ref in the callback as the reference will be owned by the window, but
> then we have to document this and to get the reference count right (i.e. use
> g_value_take object in the accumulator/unref the value returned by the emission
> of the signal).

with regards to the ref count, I made some testing and it seems to me like it is correct the way it is now; GSignal is probably doing some unref magic along the way, because the ref count gets to the signal emiter as 1, even though it is at 2 after g_value_set_object; and if I replace g_value_set_object with g_value_take_object in the accumulator I end up having all kinds of g_critical messages, even without doing a g_object_unref:

(lt-GtkLauncher:11414): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer

(lt-GtkLauncher:11414): GLib-GObject-CRITICAL **: g_signal_emit_valist: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

(lt-GtkLauncher:11414): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer

(lt-GtkLauncher:11414): GLib-GObject-CRITICAL **: g_signal_handlers_destroy: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

(lt-GtkLauncher:11414): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer

(lt-GtkLauncher:11414): GLib-GObject-CRITICAL **: g_signal_handlers_destroy: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

(lt-GtkLauncher:11414): GLib-GObject-CRITICAL **: g_object_ref: assertion `G_IS_OBJECT (object)' failed

(lt-GtkLauncher:11414): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

So I think perhaps we don't need handling different than what we have now?

> - Use the exact type when registering new signals, not just G_TYPE_OBJECT.

done
 
> - How about replacing locationbar with uribar? We use uri in every other place,
> but I don't know what I prefer. In both cases I would prefer to separate bar
> from the previous word but then it wouldn't be consistent with the other names.

Not a lot of precedence set for this one. Perhaps uri_entry, since it is not really a 'bar' like the others? I'm not changing this one for now so we can discuss and reach some consensus.

> - The default value for resizable should be TRUE.

Gah. Bad copy/paste =)

> - WebKitWebFeatures doesn't have functions to access directly properties but
> probably it's better this way than adding a lot of getters/setters.

I would rather keep it that way. g_object_{set,get} are convenient enough, and we're not deviating from stuff such as WebKitWebSettings, which has properties only accessible through the GObject generic accessors.
 
> - g_object_notify() is needed only in functions setting values not in the
> implementation of set_property as g_object_set_property already emits it.
> 
> - Change the _set_window_features() function to set it only if it's different
> from the previous one and to use g_object_notify() after setting it. I know
> that other properties are not doing it but there is a bug open about this (see
> bug #18405).

ACK

[Gustavo Noronha (kov)]

Created attachment 21357 [details]
merged patch reworked after comments

Patch incorporating the comments.

[Marco Barisione]

(In reply to comment #9)
> Created an attachment (id=21357) [edit]
> merged patch reworked after comments
> 
> Patch incorporating the comments.

The documentation for webkit_web_view_set_window_features() is not very clear. Excluding this the patch seems ok to me, now you should mark it for review so a real reviewer can review it :)

[Gustavo Noronha (kov)]

Created attachment 21374 [details]
proposed patch

Whoever reviews this patch please consider the suggestion made by Marco Barisione about renaming locationbar to uribar, or uri_bar, and my own of renaming it to something such as uri_entry where relevant.

[Gustavo Noronha (kov)]

> The documentation for webkit_web_view_set_window_features() is not very clear.
> Excluding this the patch seems ok to me, now you should mark it for review so a
> real reviewer can review it :)

I tried making it a bit better =). Thanks a lot for working with me on this, I really appreciate it!

[Gustavo Noronha (kov)]

Created attachment 21464 [details]
Correct proposed patch

It looks like I fscked up the last patch upload; this is a fixed one. My comments above about uri_entry remain.

[Marco Barisione]

The return type of create-web-view and show-web-view are wrong, they should be WEBKIT_TYPE_WEB_VIEW and G_TYPE_BOOLEAN instead of G_TYPE_OBJECT and WEBKIT_TYPE_WEB_VIEW.

[Christian Dywan]

Comment on attachment 21464 [details]
Correct proposed patch

>+    GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
>+    if (window) {
>+        IntRect intrect = IntRect(rect);
>+        WebKitWebWindowFeatures* webWindowFeatures = webkit_web_view_get_window_features(m_webView);
>+
>+        g_object_set(G_OBJECT(webWindowFeatures),
>+                     "x", intrect.x(),
>+                     NULL);
>+
>+        g_object_set(G_OBJECT(webWindowFeatures),
>+                     "y", intrect.y(),
>+                     NULL);
>+
>+        g_object_set(G_OBJECT(webWindowFeatures),
>+                     "width", intrect.width(),
>+                     NULL);
>+
>+        g_object_set(G_OBJECT(webWindowFeatures),
>+                     "height", intrect.height(),
>+                     NULL);
>+    }

That should be one g_object_set instead of four.

>+void webkit_web_view_show(WebKitWebView* webView)
>+{
>+    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
>+
>+    gboolean isHandled = FALSE;
>+    g_signal_emit(webView, webkit_web_view_signals[SHOW_WEB_VIEW], 0, &isHandled);
>+    if (isHandled)
>+        return;
>+
>+    GtkWidget* toplevel = gtk_widget_get_toplevel(GTK_WIDGET(webView));
>+    gtk_widget_show_all(toplevel);
>+}

You should never gtk_widget_show_all on code you did not write. Plus you cannot know if there is a toplevel, so you need to check that first. A gtk_widget_show might be okay.

>+static void webkit_web_view_request_init(WebKitWebViewRequest* request)
>+{
>+    request->priv = WEBKIT_WEB_VIEW_REQUEST_GET_PRIVATE(request);
>+
>+    WebKitWebViewRequestPrivate* priv = request->priv;
>+    priv->name = g_strdup("");
>+    priv->webWindowFeatures = webkit_web_window_features_new();
>+}

Why is name initially "" as opposed to NULL?

>+    g_object_class_install_property(objectClass, PROP_NAME,
>+                                    g_param_spec_string("name",
>+                                                        "New window name",
>+                                                        "The name of the new window",
>+                                                        "",
>+                                                        WEBKIT_PARAM_READWRITE));

I'm not sure about name. It's the name of a "frame" or "window" in HTML terms, so it might be better to name it accordingly.

>+WEBKIT_API GType
>+webkit_web_view_request_get_type(void);
>+
>+WEBKIT_API WebKitNetworkRequest*
>+webkit_web_view_request_get_network_request(WebKitWebViewRequest* request);
>+
>+WEBKIT_API const gchar*
>+webkit_web_view_request_get_name(WebKitWebViewRequest* request);
>+
>+WEBKIT_API void
>+webkit_web_view_request_set_name(WebKitWebViewRequest* request, const gchar* name);
>+
>+WEBKIT_API WebKitWebWindowFeatures*
>+webkit_web_view_request_get_window_features(WebKitWebViewRequest* request);
>+
>+WEBKIT_API void
>+webkit_web_view_request_set_window_features(WebKitWebViewRequest* request, WebKitWebWindowFeatures* windowFeatures);

WebViewRequest might be confusing, looking at NetworkRequest. However I'm not quite clear yet as for how this class fits in the interface. It might be less confusing in a general context.

>+enum {
>+    PROP_0,
>+
>+    PROP_X,
>+    PROP_Y,
>+    PROP_WIDTH,
>+    PROP_HEIGHT,
>+    PROP_TOOLBAR_VISIBLE,
>+    PROP_STATUSBAR_VISIBLE,
>+    PROP_SCROLLBAR_VISIBLE,
>+    PROP_MENUBAR_VISIBLE,
>+    PROP_LOCATIONBAR_VISIBLE,
>+    PROP_RESIZABLE,
>+    PROP_FULLSCREEN,
>+    PROP_DIALOG

If I remember correctly one of these became meaningless at some point, in the sense that it causes no error in javascript but is never honored. I think it probably was "resizable" but I cannot find the bug number right now.

>+    g_object_class_install_property(gobject_class,
>+                                    PROP_X,
>+                                    g_param_spec_int(
>+                                    "x",
>+                                    "x",
>+                                    "The starting x position of the window on the screen.",
>+                                    -1,
>+                                    G_MAXINT,
>+                                    -1,
>+                                    flags));

Why is the lower limit -1? I don't think this is in the ECMA spec.

>+    g_object_class_install_property(gobject_class,
>+                                    PROP_FULLSCREEN,
>+                                    g_param_spec_boolean(
>+                                    "fullscreen",
>+                                    "Full-screen",
>+                                    "Controls whether window will be displayed fullscreen.",
>+                                    FALSE,
>+                                    flags));

All the properties have non-matching name and nick. The spelling must be the same, merely the case is different, for example "fullscreen" and "Fullscreen" in this case.

>+    g_object_class_install_property(gobject_class,
>+                                    PROP_DIALOG,
>+                                    g_param_spec_boolean(
>+                                    "dialog",
>+                                    "Dialog",
>+                                    "Controls whether window should be a dialog, or a top level window.",
>+                                    FALSE,
>+                                    flags));

What is the exact use case of this? Is this actually something that websites make use of/ is in a spec/ has any other relevance?

>+    g_object_set(G_OBJECT(webWindowFeatures),
>+                 "toolbar_visible", features.toolBarVisible,
>+                 "statusbar_visible", features.statusBarVisible,
>+                 "scrollbar_visible", features.scrollbarsVisible,
>+                 "menubar_visible", features.menuBarVisible,
>+                 "locationbar_visible", features.locationBarVisible,
>+                 "resizable", features.resizable,
>+                 "fullscreen", features.fullscreen,
>+                 "dialog", features.dialog,
>+                 NULL);

Please use hyphens for property and signal names, like "toolbar-visible" instead of "toolbar_visible".

[Marco Barisione]

(In reply to comment #15)
> (From update of attachment 21464 [details] [edit])
> You should never gtk_widget_show_all on code you did not write. Plus you cannot
> know if there is a toplevel, so you need to check that first. A gtk_widget_show
> might be okay.

Ok for gtk_widget_show() but in this case it should also be called on the webview too. _get_toplevel() returns the widget itself if there is not toplevel.

> >+static void webkit_web_view_request_init(WebKitWebViewRequest* request)
> >+{
> >+    request->priv = WEBKIT_WEB_VIEW_REQUEST_GET_PRIVATE(request);
> >+
> >+    WebKitWebViewRequestPrivate* priv = request->priv;
> >+    priv->name = g_strdup("");
> >+    priv->webWindowFeatures = webkit_web_window_features_new();
> >+}
> 
> Why is name initially "" as opposed to NULL?

Don't remember if there was a reason for that, probably NULL is the right thing.
 
> >+    g_object_class_install_property(objectClass, PROP_NAME,
> >+                                    g_param_spec_string("name",
> >+                                                        "New window name",
> >+                                                        "The name of the new window",
> >+                                                        "",
> >+                                                        WEBKIT_PARAM_READWRITE));
> 
> I'm not sure about name. It's the name of a "frame" or "window" in HTML terms,
> so it might be better to name it accordingly.

I would say frame-name.
 
> WebViewRequest might be confusing, looking at NetworkRequest. However I'm not
> quite clear yet as for how this class fits in the interface. It might be less
> confusing in a general context.

I'm not so sure anymore that we need it as both the win and mac port pass to the delegate just the URI and the window features.

> >+enum {
> >+    PROP_0,
> >+
> >+    PROP_X,
> >+    PROP_Y,
> >+    PROP_WIDTH,
> >+    PROP_HEIGHT,
> >+    PROP_TOOLBAR_VISIBLE,
> >+    PROP_STATUSBAR_VISIBLE,
> >+    PROP_SCROLLBAR_VISIBLE,
> >+    PROP_MENUBAR_VISIBLE,
> >+    PROP_LOCATIONBAR_VISIBLE,
> >+    PROP_RESIZABLE,
> >+    PROP_FULLSCREEN,
> >+    PROP_DIALOG
> 
> If I remember correctly one of these became meaningless at some point, in the
> sense that it causes no error in javascript but is never honored. I think it
> probably was "resizable" but I cannot find the bug number right now.

We will check/ask on #webkit.

> >+    g_object_class_install_property(gobject_class,
> >+                                    PROP_DIALOG,
> >+                                    g_param_spec_boolean(
> >+                                    "dialog",
> >+                                    "Dialog",
> >+                                    "Controls whether window should be a dialog, or a top level window.",
> >+                                    FALSE,
> >+                                    flags));
> 
> What is the exact use case of this? Is this actually something that websites
> make use of/ is in a spec/ has any other relevance?

I think it's a IE extension also supported in WebKit. After the merge of squirrel fish WebKit doesn't support anymore showModalDialog(), I asked on #webkit if this is a regression but I got no answer. I will try again and submit a bug report.

[Gustavo Noronha (kov)]

Created attachment 21902 [details]
proposed patch

I applied most of the suggestions made by Christian, following also the comments made by Marco Barisione. This patch has an alternative approach to passing information to the create-web-view signal: instead of a new class holding the information I just pass the WebWindowFeatures and the NetworkRequest objects.

I tried to address the problem with property names. I left some issues unmodified while me and Barisione work on finding the correct answers, such as the dialog property of WebWindowFeatures, and the limits for width/height.

[Christian Dywan]

Comment on attachment 21902 [details]
proposed patch

>-void ChromeClient::setWindowRect(const FloatRect& r)
>+void ChromeClient::setWindowRect(const FloatRect& rect)
> {
>-    notImplemented();
>+    if (!m_webView)
>+        return;
>+
>+    GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
>+    if (window) {
>+        IntRect intrect = IntRect(rect);
>+        WebKitWebWindowFeatures* webWindowFeatures = webkit_web_view_get_window_features(m_webView);
>+
>+        g_object_set(G_OBJECT(webWindowFeatures),
>+                     "x", intrect.x(),
>+                     "y", intrect.y(),
>+                     "width", intrect.width(),
>+                     "height", intrect.height(),
>+                     NULL);
>+    }

What is the purpose of that "if (window) {" check? I have actually missed that in the last patch. It doesn't have any effect, from what I can see.

>+    if (!webView)
>         return 0;
>-    } else {
>-        /* TODO: FrameLoadRequest is not used */
>-        WebKitWebView* webView = WEBKIT_WEB_VIEW_GET_CLASS(m_webView)->create_web_view(m_webView);
>-        if (!webView)
>-            return 0;
>-
>-        WebKitWebViewPrivate* privateData = WEBKIT_WEB_VIEW_GET_PRIVATE(webView);
>-        return privateData->corePage;
>-    }
>+
>+    WebKitWebViewPrivate* privateData = WEBKIT_WEB_VIEW_GET_PRIVATE(webView);
>+    return privateData->corePage;

Make that "return core(webView);" please.

>@@ -67,6 +69,7 @@ extern "C" {
>     struct _WebKitWebViewPrivate {
>         WebCore::Page* corePage;
>         WebKitWebSettings* webSettings;
>+        WebKitWebWindowFeatures* webWindowFeatures;
> 
>         WebKitWebFrame* mainFrame;
>         WebCore::String applicationNameForUserAgent;
>@@ -110,6 +113,9 @@ extern "C" {
>     WebKitWebHistoryItem*
>     webkit_web_history_item_new_with_core_item(WebCore::HistoryItem*);
> 
>+    void
>+    webkit_web_view_show (WebKitWebView* webView);
>+
>     // FIXME: Move these to webkitwebframe.h once their API has been discussed.
> 
>     WEBKIT_API GSList*

What is the idea here? Please decide if you want to keep this private for now, by moving it below the FIXME comment, or move it to webkitwebview.h. :)

>@@ -123,6 +129,12 @@ extern "C" {
> 
>     WEBKIT_API gchar*
>     webkit_web_view_get_selected_text (WebKitWebView* web_view);
>+
>+    WEBKIT_API WebKitWebWindowFeatures*
>+    webkit_web_window_features_new_from_core_features (const WebCore::WindowFeatures& features);

This is also misplaced. We will *never* expose WebCore interfaces. So please move this above the comment and remove the WEBKIT_API macro.

>+     * WebKitWebView::create-web-view:
>+     * @web_view: the object on which the signal is emitted
>+     * @frame: the #WebKitWebFrame
>+     * @window_features: a #WebKitWebWindowFeaturest
>+     * @network_request: a #WebKitNetworkRequest
>+     * @return: a newly allocated #WebKitWebView or %NULL
>+     *
>+     * Emitted when the creation of a new window is requested.
>+     * If this signal is handled the signal handler should return the
>+     * newly created #WebKitWebView. The new #WebKitWebView should not be
>+     * displayed to the user until the "show-web-view" signal is emitted.
>+     *
>+     * The signal handlers should not try to deal with the reference
>+     * count for the new #WebKitWebView. The widget to which the
>+     * widget is added will handle that.
>+     *
>+     * @web_view_request contains information about the new #WebKitWebView,
>+     * but note that you can freely ignore any particular request like the
>+     * size of the new window or what bars should be displayed in the new
>+     * window.
>+     */
>+    webkit_web_view_signals[CREATE_WEB_VIEW] = g_signal_new("create-web-view",
>+            G_TYPE_FROM_CLASS(webViewClass),
>+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+            G_STRUCT_OFFSET (WebKitWebViewClass, create_web_view),
>+            webkit_create_web_view_request_handled,
>+            NULL,
>+            webkit_marshal_OBJECT__OBJECT_OBJECT_OBJECT,
>+            G_TYPE_OBJECT, 3,
>+            WEBKIT_TYPE_WEB_FRAME,
>+            WEBKIT_TYPE_WEB_WINDOW_FEATURES,
>+            WEBKIT_TYPE_NETWORK_REQUEST);

According to the documentation the return type needs to be a WEBKIT_TYPE_WEB_VIEW (or a subclass). So that type should also be specified in code.

>+     * WebKitWebView::show-web-view:
>+     * @web_view: the object on which the signal is emitted
>+     * @return: %TRUE to stop other handlers from being invoked for the event, %FALSE to propagate the event further
>+     *
>+     * Emitted after "create-web-view" when the new #WebKitWebView should
>+     * be displayed to the user.
>+     *
>+     * If there isn't any handler for this signal or if they return %FALSE,
>+     * then the default action is to call gtk_widget_show_all() on the
>+     * toplevel window containing @web_view.

The documentation needs to follow the implementation. ;)

>+void webkit_web_view_show(WebKitWebView* webView)
>+{
>+    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
>+
>+    gboolean isHandled = FALSE;
>+    g_signal_emit(webView, webkit_web_view_signals[SHOW_WEB_VIEW], 0, &isHandled);
>+    if (isHandled)
>+        return;
>+
>+    GtkWidget* toplevel = gtk_widget_get_toplevel(GTK_WIDGET(webView));
>+    if (toplevel)
>+        gtk_widget_show(toplevel);
>+    gtk_widget_show(GTK_WIDGET(webView));

I'm not sure how useful that behaviour is but I could go with it if someone likes it.

Note that "if (toplevel)" is wrong and must be "GTK_WIDGET_TOPLEVEL(toplevel)" (see the documentation of gtk_widget_get_toplevel in the Gtk+ API reference)

>+    g_object_class_install_property(gobject_class,
>+                                    PROP_TOOLBAR_VISIBLE,
>+                                    g_param_spec_boolean(
>+                                    "toolbar-visible",
>+                                    "Toolbar Visible",
>+                                    "Controls whether the tool bar should be visible for the window.",
>+                                    TRUE,
>+                                    flags));
>+ [snip]

Thanks for fixing the property names. Now this is might be nit picking but I would really like to see the one word style in blurbs, too, for the sake of consistency and avoiding confusion about API.

I like this version much better than the previous one. I will try to find some time to try this out in midori.

[Christian Dywan]

> I like this version much better than the previous one. I will try to find some
> time to try this out in midori.

So I found time and also two issues:

> WebKitWebView  * (* create_web_view) (WebKitWebView* web_view,
>            WebKitWebWindowFeatures* window_features,
>            WebKitNetworkRequest* request);
The signature is wrong, the first argument after the web view is a web frame.

Also after implementing create-web-view, new windows did start to show up as tabs in midori. However the following critical warning was printed:

GLib-GObject-CRITICAL **: g_value_set_boolean: assertion `G_VALUE_HOLDS_BOOLEAN (value)' failed

I'm not sure where this is coming from, I can't see the problem in the code.

[Marco Barisione]

(In reply to comment #17)
> Created an attachment (id=21902) [edit]
> proposed patch

The return value of show-web-view should be a boolean but when creating the new signal WEBKIT_TYPE_WEB_VIEW is used. This also explain Christian's problem.

[Christian Dywan]

(In reply to comment #20)
> (In reply to comment #17)
> > Created an attachment (id=21902) [edit]
> > proposed patch
> 
> The return value of show-web-view should be a boolean but when creating the new
> signal WEBKIT_TYPE_WEB_VIEW is used. This also explain Christian's problem.

Good catch. You mentioned return types earlier but I didn't double check that on the last version of the patch.

Another question came to my mind: when testing create-web-view I did not use show-web-view as suggested by the documentation. For instance to add a tab to a GtkNotebook the widget needs to be visible due to a quirk in the implementation. So the widget is visible immediately. This is probably different if the new web view is contained in a newly created window. Further more, there is no indication of the expected time frame between creating and showing the new web view - I can't leave the new widget invisible for a possibly long time, otherwise the application looks broken. This needs to be clearly documented, otherwise it might better to discard that second step altogether.

[Gustavo Noronha (kov)]

Created attachment 21935 [details]
new proposed patch

I haven't removed/changed show-web-view yet, though it looks like we might:

<kov>
 shiira implements the webViewShow delegate and does this:
     document = [[NSDocumentController sharedDocumentController] 
             documentForWindow:[webView window]];
     [document showWindows];
 kalikiana: aha: http://developer.apple.com/documentation/Cocoa/Conceptual/DisplayWebContent/Tasks/MultipleWindows.html#//apple_ref/doc/uid/20002026-117573
 kalikiana: that may answer our doubt about the delay; according to that doc, show-web-view would be invoked directly after create-web-view
 what I find weird about the way this is documented is that their createWebViewWithRequest needs to ask the frame to load the request, while, as barisione showed me, webcore already loads the request on the newly created webview

<kalikiana>
kov: Hm.... makes me wonder how useful that signal is. I mean, if web-view-show is emitted immediately afterwards anyway, why bother to use that?

[Marco Barisione]

At this point maybe it's better to just remove the show-web-view signal.

I tested the patch a bit more, I found that clicking on open in new window in the context menu opens a new windows but doesn't navigate there. I didn't investigate the reason yet, I wonder if this goes through yet another code path.

Note that we are passing a network request to new-web-view but other ports just pass the URL.

Any suggestion on the locationbar/uribar/uri_entry naming?

[Christian Dywan]

(In reply to comment #23)
> At this point maybe it's better to just remove the show-web-view signal.

I think I agree here. Right now the use case is rather unclear. Unless... see above:

> I tested the patch a bit more, I found that clicking on open in new window in
> the context menu opens a new windows but doesn't navigate there. I didn't
> investigate the reason yet, I wonder if this goes through yet another code
> path.

This might be the only reason why show-web-view exists, if in this case it was meant to activate the new window/ tab? Or it might as well be nonsense, I did not investigate that idea yet. Marco, did you by chance try to hook show-web-view up to see if it makes a difference?

> Note that we are passing a network request to new-web-view but other
> ports just pass the URL.

I wonder if there was a reason besides simplicity of api. Imho it's quite a lot more flexible with a network request, including possible future enhancements.

> Any suggestion on the locationbar/uribar/uri_entry naming?

Traditionally locationbar was actually a separate toolbar, unlike today where most navigators include a location entry in the main toolbar. So I think we should keep that name.

[Gustavo Noronha (kov)]

(In reply to comment #23) 
> I tested the patch a bit more, I found that clicking on open in new window in
> the context menu opens a new windows but doesn't navigate there. I didn't
> investigate the reason yet, I wonder if this goes through yet another code
> path.

It did work on my test with midori. Also, I put some printf's in WebCore's code and found that open in new window goes this way:

WebCore/page/ContextMenuController.cpp:openNewWindow

which calls

WebCore/page/Chrome.cpp:Chrome::createWindow

which then calls

WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:ChromeClient::createWindow

javascript's window.open calls ChromeClient's createWindow through another code path, and target="_blank" links opens new windows in Midori with Christian's patch, but does not go through that code path at all; I'll try to find what path it takes

[Gustavo Noronha (kov)]

Created attachment 22564 [details]
improved proposed patch

I did some enhancement in this patch, adding documentation that was missing, improving some of the already existing documentation (mainly adding Since tags).

[Christian Dywan]

(In reply to comment #26)
> Created an attachment (id=22564) [edit]
> improved proposed patch
> 
> I did some enhancement in this patch, adding documentation that was missing,
> improving some of the already existing documentation (mainly adding Since
> tags).

The patch still includes the "resizable" and "dialog" window features properties, which I am not sure we want to have here.

For one, resizable isn't useful anymore, is it? For instance, Gecko ignores it these days as far as I know. I propose that we leave it unless someone comes up with a good reason to have it.

The "dialog" feature doesn't seem to have a use case. I'm having a hard time trying to find a person or website that actually makes use of this. So again, let's omit that now. If somebody wants it, we can always add it in a follow up patch.

[Gustavo Noronha (kov)]

Created attachment 22605 [details]
proposed patch

okidoki! patch with those properties removed =)

[Marco Barisione]

(In reply to comment #28)
> Created an attachment (id=22605) [edit]
> proposed patch

Oepning a new window from the context menu doesn't work here: the new page/tab is opened but the navigation doesn't occur. How did you test it? Are you manually navigating to the web page manually from the create-web-view handler?


This is the behaviour in the three different cases we can have:

target=_"blank":
- FrameLoaderClient::dispatchCreatePage()
- We emit create-web-view:
  - ResourceRequest not available -> WebKitNetworkRequest with a NULL uri
  - WindowFeatures not available -> WebKitWebWindowFeatures with default settings
- FrameLoaderClient::dispatchShow()
- The URL is loaded

Contextual menu:
- ChromeClient::createWindow()
- We emit create-web-view:
  - ResourceRequest available
  - WindowFeatures available
- ChromeClient::setFoo()
- ChromeClient::show()
- We emit show-web-view
- The URL is loaded

window.open() via Javascript:
- ChromeClient::createWindow()
- We emit create-web-view:
  - ResourceRequest not available -> NULL WebKitNetworkRequest
  - WindowFeatures available
- ChromeClient::setFoo()
- ChromeClient::show()
- We emit show-web-view
- The URL is loaded

Unfortunately the ResourceRequest is available only when the link is opened from the contextual menu, so that parameter seems quite useless atm.
In the case of target="_blank" we have the URL in WebCore so we could just modify it to pass the URL to FrameLoaderClient::dispatchCreatePage().
In the case of window.open() the URL is not available and the reason is explained in JSDOMWindowBase.cpp:

// FIXME: It's much better for client API if a new window starts with a URL, here where we
// know what URL we are going to open. Unfortunately, this code passes the empty string
// for the URL, but there's a reason for that. Before loading we have to set up the opener,
// openedByDOM, and dialogArguments values. Also, to decide whether to use the URL we currently
// do an allowsAccessFrom call using the window we create, which can't be done before creating it.
// We'd have to resolve all those issues to pass the URL instead of "".

Another problem is the ChromeClient::setFoo() is called for every window feature, it seems that you are not supposed to set the features from the window features but when the ChromeClient::setFoo() method is called. This is why there is a separate ChromeClient::show() method, so you can avoid flickering.

A possible solution is to make the WindowFeatures object just a struct with public fields and have an empty implementation of the various methods to set window features in the ChromeClient. In this case we are not losing anything as those methods are called only before showing the window and are not accessible in other ways, like js. If we do this we can also kill the show-web-view signal.

[Eric Seidel (no email)]

Comment on attachment 22605 [details]
proposed patch

Once again, assigning to Alp for review or assignment to some other Gtk reviewer.  The rest of us don't know anything about gtk :(

[Gustavo Noronha (kov)]

(In reply to comment #29)
> (In reply to comment #28)
> > Created an attachment (id=22605) [edit]
> > proposed patch
> 
> Oepning a new window from the context menu doesn't work here: the new page/tab
> is opened but the navigation doesn't occur. How did you test it? Are you
> manually navigating to the web page manually from the create-web-view handler?

You're right! I tried it in midori and it worked; I'm pretty sure midori is probably doing the open itself, as it did not work with the simple implementation I wrote for GtkLauncher.

> Unfortunately the ResourceRequest is available only when the link is opened
> from the contextual menu, so that parameter seems quite useless atm.

OK, let's think from a use-case point of view here... that parameter would be useful for the client application to choose whether the new window should be created by examining the request that would be loaded, and returning the new instance or NULL as a means of having the decision implemented. The client application should have other ways of handling this use-case by monitoring the signals for WebKitWebView/WebKitWebFrame, right?

I'm almost convinced that we should not even be passing the URI if we maintain the create-web-view signal.

> Another problem is the ChromeClient::setFoo() is called for every window
> feature, it seems that you are not supposed to set the features from the window
> features but when the ChromeClient::setFoo() method is called. This is why
> there is a separate ChromeClient::show() method, so you can avoid flickering.
> 
> A possible solution is to make the WindowFeatures object just a struct with
> public fields and have an empty implementation of the various methods to set
> window features in the ChromeClient. In this case we are not losing anything as
> those methods are called only before showing the window and are not accessible
> in other ways, like js. If we do this we can also kill the show-web-view
> signal.

I think javascript can request changing the size/position of windows after the page has been loaded, so perhaps we should keep WindowFeatures as a GObject to with properties which an interested application can monitor.

How about we try to handle everything in ChromeClient? We create the new WebView ourselves, without depending on the application to do it, we capture all the changes done by setFoo methods into the webview's internal WindowFeatures object, then when show-web-view is being emitted today we emit a 'new-web-view' signal passing the already composed WebView, and expect the user to add it to a window. We can simply destroy it, in case the signal is not handled (no handlers or all of them returning FALSE).

This may cause some unneeded memory/cpu usage, but makes things very simple. We can even try to handle loading the URL ourselves if we are able to detect the case in which it won't be loaded. What do you think?

[Marco Barisione]

(In reply to comment #31)
> OK, let's think from a use-case point of view here... that parameter would be
> useful for the client application to choose whether the new window should be
> created by examining the request that would be loaded, and returning the new
> instance or NULL as a means of having the decision implemented. The client
> application should have other ways of handling this use-case by monitoring the
> signals for WebKitWebView/WebKitWebFrame, right?

It should but this mean that you should keep the window hidden until you now the destination of the navigation.

> I'm almost convinced that we should not even be passing the URI if we maintain
> the create-web-view signal.

Probably as it's useless.
 
> I think javascript can request changing the size/position of windows after the
> page has been loaded, so perhaps we should keep WindowFeatures as a GObject to
> with properties which an interested application can monitor.

Does JS can do that? I didn't find code for that in WebCore and I'm not a JS expert.

> How about we try to handle everything in ChromeClient? We create the new
> WebView ourselves, without depending on the application to do it, we capture
> all the changes done by setFoo methods into the webview's internal
> WindowFeatures object, then when show-web-view is being emitted today we emit a
> 'new-web-view' signal passing the already composed WebView, and expect the user
> to add it to a window. We can simply destroy it, in case the signal is not
> handled (no handlers or all of them returning FALSE).

No, because most non-trivial apps will want to create an instance of a class derived from a WebView.

[Gustavo Noronha (kov)]

(In reply to comment #32)
> > I think javascript can request changing the size/position of windows after the
> > page has been loaded, so perhaps we should keep WindowFeatures as a GObject to
> > with properties which an interested application can monitor.
> 
> Does JS can do that? I didn't find code for that in WebCore and I'm not a JS
> expert.

I know it is able to; I checked and it seems to not be part of the W3C DOM specification (yet?), although it recognizes that some implementations do have ways of changing size/position. The window object for gecko, for instance, has the resizeTo method.

> No, because most non-trivial apps will want to create an instance of a class
> derived from a WebView.

Right.... 

[Gustavo Noronha (kov)]

Created attachment 23180 [details]
proposed patch

this patch opens the URL in all cases listed by barisione, and documents the create-web-view/show-web-view stuff better; it also handles the window features in a more clean way; should be good to land now

[Gustavo Noronha (kov)]

Created attachment 23181 [details]
a test page

a test page I've been using to test the patch on

[Gustavo Noronha (kov)]

Created attachment 23182 [details]
the second test page

[Gustavo Noronha (kov)]

Created attachment 23183 [details]
GtkLauncher test patch

code to test the patch with the GtkLauncher

[Gustavo Noronha (kov)]

Created attachment 23184 [details]
proposed patch

[copying from the bad patch entry]
this patch opens the URL in all cases listed by barisione, and documents the
create-web-view/show-web-view stuff better; it also handles the window features
in a more clean way; should be good to land now

[Alp Toker]

Comment on attachment 23184 [details]
proposed patch

I'm not sure about webkit_web_view_show()

I missed the discussion about the importance of show() and what it actually does in other bug reports.

Since it'll conflict with gtk_widget_show() in many language bindings we probably want to call it something like webkit_web_view_present() instead.

Could you describe what this function is here for and what it does so we can figure out what to do with it?

Thanks

[Gustavo Noronha (kov)]

(In reply to comment #39)
> (From update of attachment 23184 [details] [edit])
> I'm not sure about webkit_web_view_show()
[...]
> Could you describe what this function is here for and what it does so we can
> figure out what to do with it?

I'm pretty sure this came from barisione's patch, and I never got around to changing it. Looking at it now I believe I should probably make it private, as the only good it does is it helps both ChromeClient and FrameLoaderClient, does that seem OK to you?

As for the other show, the signal, The webkit "workflow" we have to work with is create-web-view is emitted, then some other delegates are called that set window features, and then show is emitted. This is so that the application can hand whatever WebView subclass they have, and still only show the window/webview after all the window features are known, to avoid flickering and other unwanted effects related to size/position. Of course it only matters if you are not going to ignore the window features.

[Gustavo Noronha (kov)]

Created attachment 23324 [details]
proposed patch

reworked the patch to make webkit_web_view_show private

[Marco Barisione]

(In reply to comment #40)
> I'm pretty sure this came from barisione's patch, and I never got around to
> changing it. Looking at it now I believe I should probably make it private, as
> the only good it does is it helps both ChromeClient and FrameLoaderClient, does
> that seem OK to you?

In the first version of the patch it was private, I don't know when it was made public but probably it  was just a mistake by me or Gustavo.


In some cases it could be useful to keep the popup hidden, so we should fix the ScrollView::update problem. For instance I implemented a popup blocker, and the window must be kept hidden until we receive navigation-requested.

[Gustavo Noronha (kov)]

Created attachment 23501 [details]
update proposed patch

I updated the patch so that it takes advantage of the bug fix done by barisione on the scroll update problem, thus removing the need for the work-around.

[Gustavo Noronha (kov)]

Created attachment 24607 [details]
patch applying cleanly as of now

I am updating the patch so that it applies cleanly as of current TOT.

[Gustavo Noronha (kov)]

Created attachment 24620 [details]
patch after tonight's review session

I'm attaching a new patch with fixes for problems found and consensus we reached in the #webkit-gtk review session tonight. Had input from barisione, kalikiana, xan, pierlux (to a lesser extent, since he had to go to his class), janm, and jonner. I am asking the people who are OK with the patch landing to pop in and add a comment here to that effect.

[Jan Alonzo]

+     * If there isn't any handler for this signal or if they return                                                                                                                                                
+     * %FALSE, then the default action is to call gtk_widget_show() on                                                                                                                                             
+     * the toplevel window containing @web_view, and on @web_view.   

^^ Is that still true?

[Gustavo Noronha (kov)]

(In reply to comment #46)
> +     * If there isn't any handler for this signal or if they return            
> +     * %FALSE, then the default action is to call gtk_widget_show() on         
> +     * the toplevel window containing @web_view, and on @web_view.   
> 
> ^^ Is that still true?
> 

No, removed from the patch.

[Gustavo Noronha (kov)]

Created attachment 24626 [details]
proposed patch with comments from tonight's review session

remove bad paragraph from comment/documentation

[Jan Alonzo]

(In reply to comment #48)
> Created an attachment (id=24626) [edit]
> proposed patch with comments from tonight's review session
> 
> remove bad paragraph from comment/documentation
> 

The API's been discussed and agreed upon by the attendees during today's meeting (minutes at http://live.gnome.org/Epiphany/Meetings/20081023).

Gtk reviewers: Is there anything more that needs to be done for this patch to be r+'d and landed?

[Christian Dywan]

(In reply to comment #48)
> Created an attachment (id=24626) [edit]
> proposed patch with comments from tonight's review session
> 
> remove bad paragraph from comment/documentation

Hmpf, it seems something ate my comment on the way before it reached bugzilla. Anyhow.

I went over the patch yet another time and actually have a tiny nitpick: a the bottom there is a "GNU Library General License" header. That one is called "GNU Lesser Genereal License" these days ;)

Incidentally I wonder when we are bumping the version, so that code can actually use the new functionality without build system work arounds.

For what I want, that patch should be ready, as was agreed on by the already mentioned attendées.

[Gustavo Noronha (kov)]

(In reply to comment #50)
> I went over the patch yet another time and actually have a tiny nitpick: a the
> bottom there is a "GNU Library General License" header. That one is called "GNU
> Lesser Genereal License" these days ;)

Bad copy/paste. I agree with the change; I won't upload a new patch because this is easily sed'able in the patch, and I don't want to clutter the bug report even more, but if whoever will commit it prefers I can do it.

Thanks, kalikiana!

[Kalle Vahlman]

I was testing this and had a situation where I didn't handle the web-view-ready signal. The result was a crash in PlatformScreenGtk since the view wasn't realized and PlatformScreenGtk assumed it is. While PlatformScreenGtk obviously needs fixing (alp is on it), what would probably make sense also is to call gtk_widget_realize() from the default handler to ensure correct values are gained for the PlatformScreenGtk methods and to prevent crashing.

I'm not sure if the crash is due to the page loading even though the view isn't visible, but if this is the case it might actually make more sense to cancel the load from the default handler (if possible).

[Holger Freyther]

Comment on attachment 24626 [details]
proposed patch with comments from tonight's review session


> +        Added the new WebKitGtk files to be built:
> +        WebKit/gtk/webkit/webkitwebwindowfeatures.{cpp,h}
> +

Did you use ./WebKitTools/Scripts/prepare-ChangeLog to generate this? It does not look it. In the future please use this script.



>          Build fix for older GTK+ versions where GTK_TYPE_TARGET_LIST isn't
> diff --git a/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp b/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
> index 256a3a3..c0ace16 100644
> --- a/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
> +++ b/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
> @@ -2,6 +2,7 @@
>   * Copyright (C) 2007 Holger Hans Peter Freyther
>   * Copyright (C) 2007, 2008 Christian Dywan <[e-mail]>
>   * Copyright (C) 2008 Nuanti Ltd.
> + * Copyright (C) 2008 Gustavo Noronha Silva <[e-mail]>
>   *
>   *  This library is free software; you can redistribute it and/or
>   *  modify it under the terms of the GNU Lesser General Public
> @@ -21,6 +22,7 @@
>  #include "config.h"
>  #include "ChromeClientGtk.h"
>  
> +#include "FrameLoadRequest.h"
>  #include "FloatRect.h"
>  #include "IntRect.h"
>  #include "PlatformString.h"
> @@ -28,6 +30,7 @@
>  #include "HitTestResult.h"
>  #include "KURL.h"
>  #include "webkitwebview.h"
> +#include "webkitnetworkrequest.h"
>  #include "webkitprivate.h"
>  #include "NotImplemented.h"
>  #include "WindowFeatures.h"
> @@ -53,7 +56,7 @@ FloatRect ChromeClient::windowRect()
>      if (!m_webView)
>          return FloatRect();
>      GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
> -    if (window) {
> +    if (GTK_WIDGET_TOPLEVEL(window)) {
>          gint left, top, width, height;
>          gtk_window_get_position(GTK_WINDOW(window), &left, &top);
>          gtk_window_get_size(GTK_WINDOW(window), &width, &height);
> @@ -62,9 +65,20 @@ FloatRect ChromeClient::windowRect()
>      return FloatRect();
>  }
>  
> -void ChromeClient::setWindowRect(const FloatRect& r)
> +void ChromeClient::setWindowRect(const FloatRect& rect)
>  {
> -    notImplemented();
> +    if (!m_webView)
> +        return;

okay. This is not needed. but other code is doing it as well... a WebKitWebView has a WebCore::Page and a WebCore::Page has a WebCore::ChromeClient associated... During destructing of a WebKitWebView no JS or such should run... I will clean this afterwards.



> +    g_object_set(G_OBJECT(webWindowFeatures),
> +                 "x", intrect.x(),
> +                 "y", intrect.y(),
> +                 "width", intrect.width(),
> +                 "height", intrect.height(),
> +                 NULL

no NULL in C++ code please, use 0, which gets turned into a null pointer...


>      GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
> -    if (window)
> +    if (GTK_WIDGET_TOPLEVEL(window))
>          gtk_window_set_focus(GTK_WINDOW(window), NULL);

so GTK_WIDGET_TOPLEVEL(0) will function correctly?



> +    WebKitWebWindowFeatures* webWindowFeatures = webkit_web_view_get_window_features(m_webView);
> +
> +    g_object_set(G_OBJECT(webWindowFeatures), "toolbar-visible", visible, NULL);

NULL again


> +    WebKitWebWindowFeatures* webWindowFeatures = webkit_web_view_get_window_features(m_webView);
> +    gboolean visible;

initialize it? or is there alaways a toolbar-visible property defined?



> +static gboolean webkit_create_web_view_request_handled(GSignalInvocationHint* ihint, GValue* returnAccu, const GValue* handlerReturn, gpointer dummy)
> +{
> +  gboolean continueEmission = TRUE;
> +  gpointer newWebView = g_value_get_object(handlerReturn);
> +  g_value_set_object(returnAccu, newWebView);
> +
> +  if (newWebView)
> +      continueEmission = FALSE;
> +
> +  return continueEmission;
> +}

return !newWebView is doing the same job


> +
>  static gboolean webkit_navigation_request_handled(GSignalInvocationHint* ihint, GValue* returnAccu, const GValue* handlerReturn, gpointer dummy)
>  {
>    gboolean continueEmission = TRUE;
> @@ -814,6 +841,63 @@ static void webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
>       * Signals
>       */

> +     * The new #WebKitWebView should not be displayed to the user
> +     * until the ::web-view-ready signal is emitted.

wrong gtk-doc sequence.. you need to fully qualify the name.

> +     * The signal handlers should not try to deal with the reference
> +     * count for the new #WebKitWebView. The widget to which the
> +     * widget is added will handle that.
> +     *
> +     * Since 1.0.2

Needs bumping to 1.0.3



> +     * Emitted after ::create-web-view when the new #WebKitWebView

gtk-doc...





> +
> +/**
> + * webkit_web_view_get_window_features
> + * @webView: a #WebKitWebView

likely to be web_view in the header file.


> +/**
>   * webkit_web_view_set_maintains_back_forward_list:
>   * @webView: a #WebKitWebView

again...







> +static void webkit_web_window_features_finalize(GObject* object)
> +{
> +    G_OBJECT_CLASS(webkit_web_window_features_parent_class)->finalize(object);

test if a finalize is set? or may we assume that for GObject?



> +    if(features.heightSet)
> +        g_object_set(G_OBJECT(webWindowFeatures), "height", static_cast<int>(features.height), NULL);

default x,y,width,height is 0 then? How does this mix with the properties allowing -1 as well?


> +gboolean webkit_web_window_features_equal(WebKitWebWindowFeatures* features1, WebKitWebWindowFeatures* features2)
> +{
> +    WebKitWebWindowFeaturesPrivate* priv1 = features1->priv;
> +    WebKitWebWindowFeaturesPrivate* priv2 = features2->priv;
> +    
> +    if((priv1->x == priv2->x) &&
> +       (priv1->y == priv2->y) &&
> +       (priv1->width == priv2->width) &&
> +       (priv1->height == priv2->height) &&

how to differentiate x,y,width,height set or not set?



> +
> +#ifndef __WEBKIT_WEB_WINDOW_FEATURES_H__
> +#define __WEBKIT_WEB_WINDOW_FEATURES_H__

__ is reserved to the system (AFAIK).


       /*< private *>/
> +    WebKitWebWindowFeaturesPrivate* priv;
> +};
> +
> +struct _WebKitWebWindowFeaturesClass {
> +    GObjectClass parent_class;
> +

       /*< private >*/
> +    /* Padding for future expansion */
> +    void (*_webkit_reserved1) (void);
> +    void (*_webkit_reserved2) (void);
> +    void (*_webkit_reserved3) (void);
> +    void (*_webkit_reserved4) (void);
> +};



>

[Holger Freyther]

I can fix:
  - The documentation, the NULLs, style... when landing

Before saying yes:
  - I want to know if -1 should be used for not set x,y,width, height or if we do not intend to be able to differentiate.

[Holger Freyther]

(In reply to comment #54)

> Before saying yes:
>   - I want to know if -1 should be used for not set x,y,width, height or if we
> do not intend to be able to differentiate.

From IRC: G_PARAM_CONSTRUCT is set on the flags which means the default value should be set object initialization.

[Holger Freyther]

Created attachment 25577 [details]
Changes on top of the patch done before landing

For reference my diff against the version of the patch (1.0.2 got renamed to 1.0.3 earlier).

[Holger Freyther]

Comment on attachment 24626 [details]
proposed patch with comments from tonight's review session

Okay, with comments. They will be fixed after landing the patch.

[Holger Freyther]

Landed in r38834.