[webkit-reviews] review denied: [Bug 19130] [GTK] ChromeClient::createWindow and friends need to be implemented : [Attachment 21464] Correct proposed patch

[bugzilla-daemon at webkit.org]

Christian Dywan <christian at imendio.com> has denied Gustavo Noronha Silva
<gns at gnome.org>'s request for review:
Bug 19130: [GTK] ChromeClient::createWindow and friends need to be implemented
https://bugs.webkit.org/show_bug.cgi?id=19130

Attachment 21464: Correct proposed patch
https://bugs.webkit.org/attachment.cgi?id=21464&action=edit

------- Additional Comments from Christian Dywan <christian at imendio.com>
>+    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".