Bug 16882 – [GTK] ChromeClientGtk is incompete

• Component: WebKit API
• Status: RESOLVED FIXED

[Christian Dywan]

ChromeClientGtk is only partly implemented.

[Christian Dywan]

Created attachment 18467 [details]
Implement stubs, no api changes

This patch implements a few stubs that don't involve public api.

[Christian Dywan]

Created attachment 18482 [details]
Implement stubs, no api changes

Sorry, uploaded wrong patch.

This is the right one.

[Alp Toker]

Comment on attachment 18482 [details]
Implement stubs, no api changes

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 <[e-mail]>
>+ * Copyright (C) 2007-2008 Christian Dywan <[e-mail]>

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.

[Christian Dywan]

Created attachment 18501 [details]
Implement stubs, no api changes, updated

> > void ChromeClient::takeFocus(FocusDirection)
> > {
> >-    notImplemented();
> >+    unfocus();
> > }
>
> Shouldn't this be focus()? (I didn't check the other port implementations;
> might be worth doing that.

I did check other implementations. For instance Qt's implementations of ::unfocus() and ::takeFocus() contain the exact same code.

[Alp Toker]

Comment on attachment 18501 [details]
Implement stubs, no api changes, updated

r=me

Please write a ChangeLog entry next time!

[Alp Toker]

Landed in r29669.