Bug 16882 - [GTK] ChromeClientGtk is incompete
Summary: [GTK] ChromeClientGtk is incompete
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Christian Dywan
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2008-01-15 07:05 PST by Christian Dywan
Modified: 2008-01-19 06:00 PST (History)
0 users

See Also:


Attachments
Implement stubs, no api changes (2.20 KB, patch)
2008-01-16 02:48 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Implement stubs, no api changes (2.88 KB, patch)
2008-01-16 12:44 PST, Christian Dywan
alp: review-
Details | Formatted Diff | Diff
Implement stubs, no api changes, updated (2.89 KB, patch)
2008-01-17 08:17 PST, Christian Dywan
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2008-01-15 07:05:50 PST
ChromeClientGtk is only partly implemented.
Comment 1 Christian Dywan 2008-01-16 02:48:44 PST
Created attachment 18467 [details]
Implement stubs, no api changes

This patch implements a few stubs that don't involve public api.
Comment 2 Christian Dywan 2008-01-16 12:44:07 PST
Created attachment 18482 [details]
Implement stubs, no api changes

Sorry, uploaded wrong patch.

This is the right one.
Comment 3 Alp Toker 2008-01-17 00:43:02 PST
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 <christian@twotoasts.de>
>+ * Copyright (C) 2007-2008 Christian Dywan <christian@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.
Comment 4 Christian Dywan 2008-01-17 08:17:59 PST
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.
Comment 5 Alp Toker 2008-01-19 05:57:48 PST
Comment on attachment 18501 [details]
Implement stubs, no api changes, updated

r=me

Please write a ChangeLog entry next time!
Comment 6 Alp Toker 2008-01-19 06:00:19 PST
Landed in r29669.