[webkit-reviews] review denied: [Bug 19130] [GTK] ChromeClient::createWindow and friends need to be implemented : [Attachment 21902] 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 21902: proposed patch
https://bugs.webkit.org/attachment.cgi?id=21902&action=edit

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