[webkit-reviews] review denied: [Bug 16882] [GTK] ChromeClientGtk is incompete : [Attachment 18482] Implement stubs, no api changes

[bugzilla-daemon at webkit.org]

Alp Toker <alp at atoker.com> has denied Christian Dywan <christian at imendio.com>'s
request for review:
Bug 16882: [GTK] ChromeClientGtk is incompete
http://bugs.webkit.org/show_bug.cgi?id=16882

Attachment 18482: Implement stubs, no api changes
http://bugs.webkit.org/attachment.cgi?id=18482&action=edit

------- Additional Comments from Alp Toker <alp at atoker.com>
ChangeLog please.

>Index: WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp	(Revision 29531)
>+++ WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp	(Arbeitskopie)
>@@ -1,6 +1,6 @@
> /*
>  * Copyright (C) 2007 Holger Hans Peter Freyther
>- * Copyright (C) 2007 Christian Dywan <christian at twotoasts.de>
>+ * Copyright (C) 2007-2008 Christian Dywan <christian at imendio.com>

Copyright year should be extended with ', '

>  *
>  *  This library is free software; you can redistribute it and/or
>  *  modify it under the terms of the GNU Lesser General Public
>@@ -46,7 +46,16 @@ void ChromeClient::chromeDestroyed()
> 
> FloatRect ChromeClient::windowRect()
> {
>-    notImplemented();
>+    if (!m_webView)
>+	  return FloatRect();
>+    GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
>+    if (window)
>+    {
>+	  gint left, top, width, height;
>+	  gtk_window_get_position(GTK_WINDOW(window), &left, &top);
>+	  gdk_drawable_get_size(window->window, &width, &height);
>+	  return IntRect(left, top, width, height);
>+    }
>     return FloatRect();
> }

Brace should be on the same line as the if. I think there's a better way to get
the window rect of a window than gdk_drawable_get_size(), would need to look it
up though.

> 
>@@ -57,24 +66,33 @@ void ChromeClient::setWindowRect(const F
> 
> FloatRect ChromeClient::pageRect()
> {
>-    notImplemented();
>-    return FloatRect();
>+    if (!m_webView)
>+	  return FloatRect();
>+    gint width, height;
>+    gdk_drawable_get_size(GTK_WIDGET(m_webView)->window, &width, &height);
>+    return IntRect(0, 0, width, height);
> }

Why not use the widget's allocation details rather than the GdkDrawable? This
may not work if WebView gets windowless support in future.

Are x and y always meant to be 0?

> 
> float ChromeClient::scaleFactor()
> {
>-    notImplemented();
>+    // Not implementable
>     return 1.0;
> }
> 
> void ChromeClient::focus()
> {
>-    notImplemented();
>+    if (!m_webView)
>+	  return;
>+    gtk_widget_grab_focus(GTK_WIDGET(m_webView));
> }
> 
> void ChromeClient::unfocus()
> {
>-    notImplemented();
>+    if (!m_webView)
>+	  return;
>+    GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView));
>+    if (window)
>+	  gtk_window_set_focus(GTK_WINDOW(window), NULL);
> }
> 
> Page* ChromeClient::createWindow(Frame*, const FrameLoadRequest&, const
WindowFeatures& features)
>@@ -164,25 +182,24 @@ void ChromeClient::closeWindowSoon()
> 
> bool ChromeClient::canTakeFocus(FocusDirection)
> {
>-    notImplemented();
>-    return true;
>+    if (!m_webView)
>+	  return false;
>+    return GTK_WIDGET_CAN_FOCUS(m_webView);
> }
> 
> void ChromeClient::takeFocus(FocusDirection)
> {
>-    notImplemented();
>+    unfocus();
> }

Shouldn't this be focus()? (I didn't check the other port implementations;
might be worth doing that.