Bug 16881 – [GTK] PlatformScreenGtk is unimplemented

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

PlatformScreenGtk is mostly unimplemented.

[Christian Dywan]

Created attachment 18466 [details]
Implement stubs

[Alp Toker]

Comment on attachment 18466 [details]
Implement stubs

>Index: WebCore/platform/gtk/PlatformScreenGtk.cpp
>===================================================================
>--- WebCore/platform/gtk/PlatformScreenGtk.cpp	(Revision 29512)
>+++ WebCore/platform/gtk/PlatformScreenGtk.cpp	(Arbeitskopie)
>@@ -1,6 +1,7 @@
> /*
>  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
>- * Copyright (C) 2006 Michael Emmel [e-mail] 
>+ * Copyright (C) 2006 Michael Emmel [e-mail]
>+ * Copyright (C) 2008 Christian Dywan <[e-mail]>
>  * All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>@@ -35,37 +36,34 @@
> 
> namespace WebCore {
> 
>-int screenDepth(Widget* widget) 
>+int screenDepth(Widget* widget)
> {
>-    ASSERT(widget->containingWindow() && GTK_WIDGET(widget->containingWindow())->window);
>-
>-    gint dummy, depth;
>-    gdk_window_get_geometry(GTK_WIDGET(widget->containingWindow())->window, &dummy, &dummy, &dummy, &dummy, &depth);
>-    return depth;
>+    GdkVisual* visual = gtk_widget_get_visual(GTK_WIDGET(widget->containingWindow()));
>+    return visual->depth;
> }
> 
>-int screenDepthPerComponent(Widget*)
>+int screenDepthPerComponent(Widget* widget)
> {
>-    notImplemented();
>-    return 8;
>+    GdkVisual* visual = gtk_widget_get_visual(GTK_WIDGET(widget->containingWindow()));
>+    return visual->bits_per_rgb;
> }
> 
>-bool screenIsMonochrome(Widget*) 
>-{ 
>-    notImplemented(); 
>-    return false; 
>+bool screenIsMonochrome(Widget* widget)
>+{
>+    return screenDepth(widget) > 1;
> }

This doesn't look right.

I'm curious, did you find a way to test these functions?

It might be worth doing some null checking for screens, widgets, displays etc.

[Christian Dywan]

Created attachment 18525 [details]
Implement stubs, updated

[Darin Adler]

Comment on attachment 18525 [details]
Implement stubs, updated

 73     return IntRect(0, 0, gdk_screen_get_width(screen), gdk_screen_get_height(screen));

This doesn't look right for when multiple screens are involved. This is supposed to return the proper rectangle considering a single large coordinate space -- it can't have an origin of 0,0 for every screen. Did you test this?

[Pierre-Luc Beaudoin]

This was partly solved when fixing Bug 17681 but this patch has a better screenDepth and has an implementation for screenDepthPerComponent.  I guess having a non-zero rect returned for screenAvailableRect() is also better.

[Marco Barisione]

Created attachment 19885 [details]
Updated patch based on both Christian's and Pierre-Luc's patches

Tomorrow I will try to fix screenAvailableRect().

[Marco Barisione]

Created attachment 19901 [details]
Updated patch that implements screenAvailableRect()

The code uses _NET_WORKAREA to know the available space on the desktop, as done by qt and nautilus.

I'm not sure on what we should do when using Xinerama. I tried:
for i in `seq 1 4`; do Xnest :$i -ac -geometry 400x300+1+1 & done
Xdmx :10 -ac -configfile xdmx.conf +xinerama
export DISPLAY=:10
metacity
gnome-panel

where xdmx.conf is:
virtual example {
    wall localhost:1 localhost:2
         localhost:3 localhost:4;
}

In this case gnome-panel goes completely in the first XNest window, so what the available screen rect should be in this case?

Do you know when screenAvailableRect() is called in WebKit?

[Marco Barisione]

If xinerama is used qt just uses the entire screen rect, ignoring panels.

[Marco Barisione]

Created attachment 21295 [details]
Implemente stubs

Same patch as before but with ChangeLog entry.

screenAvailableRect() ignores panels placed not on the borders of virtual screens but this is because of the EWMH specification, see also http://bugzilla.gnome.org/show_bug.cgi?id=339692

[Marco Barisione]

Created attachment 21296 [details]
Implement stubs

Sorry, the previous patch contained some debug code.

[Alp Toker]

Comment on attachment 21296 [details]
Implement stubs

This will break non-X11 builds. Please wrap with an #if defined(...) and provide a fallback using standard GTK+ API.

You should also try to ensure that this code won't crash when no windowing system is active as we do elsewhere. This is so we can support commandline tools like web thumbnailers in future.

[Marco Barisione]

(In reply to comment #11)
> You should also try to ensure that this code won't crash when no windowing
> system is active as we do elsewhere. This is so we can support commandline
> tools like web thumbnailers in future.

Isn't checking widget->containingWindow() enough?

[Marco Barisione]

Created attachment 21385 [details]
Implement stubs

[Alp Toker]

(In reply to comment #13)
> Created an attachment (id=21385) [edit]
> Implement stubs
> 

Will try to review this soon unless someone beats me to it.

In the meantime, I noticed

+    return screenRect(widget)

^ breaks non-X11 builds due to missing semicolon.

 int screenDepth(Widget* widget)
 {
-    ASSERT(widget->containingWindow() && GTK_WIDGET(widget->containingWindow())->window);
+    GtkWidget* container = GTK_WIDGET(widget->containingWindow());
+    if (!container)
+        return 0;

^ Wonder if it's worth returning a 'sane' depth here instead of 0?

-int screenDepthPerComponent(Widget*)
+int screenDepthPerComponent(Widget* widget)
 {
-    notImplemented();
-    return 8;
+    GtkWidget* container = GTK_WIDGET(widget->containingWindow());
+    if (!container)
+        return 0;

^ Ditto.

It's worth studying what Firefox returns for these and doing the same thing if it makes sense. Was the direct X11 use done to achieve compatibility with existing browsers?

I'm not going to r- since this patch could do with more reviewer attention, but I have a feeling it's not ready to land yet.

[Holger Freyther]

Some comments (basicly r=+, if someone fixes/decides when landing):

   - screenAvailableRect will not compile for non X11. ";" is missing

   - I don't know if removing the asserts make sense but it is okay with me, did you hit the asserts?

   - Parsing NET_WORKAREA looks right, I would use a IntRect and then get this converted to FloatRect (no strong opinion on this).

[Marco Barisione]

(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=21385) [edit]
> > Implement stubs
> > 
> 
> Will try to review this soon unless someone beats me to it.

Sorry for the late reply but I didn't receive the notification for your comment (or probably it ended up on the wrong mail folder).

> In the meantime, I noticed
> 
> +    return screenRect(widget)
> 
> ^ breaks non-X11 builds due to missing semicolon.

Ok.

>  int screenDepth(Widget* widget)
>  {
> -    ASSERT(widget->containingWindow() &&
> GTK_WIDGET(widget->containingWindow())->window);
> +    GtkWidget* container = GTK_WIDGET(widget->containingWindow());
> +    if (!container)
> +        return 0;
> 
> ^ Wonder if it's worth returning a 'sane' depth here instead of 0?

Probably, but what is a sane value in this case? 24?

> -int screenDepthPerComponent(Widget*)
> +int screenDepthPerComponent(Widget* widget)
>  {
> -    notImplemented();
> -    return 8;
> +    GtkWidget* container = GTK_WIDGET(widget->containingWindow());
> +    if (!container)
> +        return 0;
> 
> ^ Ditto.

8 in this case?

> It's worth studying what Firefox returns for these and doing the same thing if
> it makes sense. Was the direct X11 use done to achieve compatibility with
> existing browsers?

I'm not sure to understand what do you mean.

(In reply to comment #14)
> (In reply to comment #13)
> > Created an attachment (id=21385) [edit]
> > Implement stubs
> > 
> 
> Will try to review this soon unless someone beats me to it.
> 
> In the meantime, I noticed
> 
> +    return screenRect(widget)
> 
> ^ breaks non-X11 builds due to missing semicolon.
> 
>  int screenDepth(Widget* widget)
>  {
> -    ASSERT(widget->containingWindow() &&
> GTK_WIDGET(widget->containingWindow())->window);
> +    GtkWidget* container = GTK_WIDGET(widget->containingWindow());
> +    if (!container)
> +        return 0;
> 
> ^ Wonder if it's worth returning a 'sane' depth here instead of 0?
> 
> -int screenDepthPerComponent(Widget*)
> +int screenDepthPerComponent(Widget* widget)
>  {
> -    notImplemented();
> -    return 8;
> +    GtkWidget* container = GTK_WIDGET(widget->containingWindow());
> +    if (!container)
> +        return 0;
> 
> ^ Ditto.
> 
> It's worth studying what Firefox returns for these and doing the same thing if
> it makes sense. Was the direct X11 use done to achieve compatibility with
> existing browsers?
> 
> I'm not going to r- since this patch could do with more reviewer attention, but
> I have a feeling it's not ready to land yet.
> 

(In reply to comment #15)
>    - screenAvailableRect will not compile for non X11. ";" is missing

I will fix it.

>    - I don't know if removing the asserts make sense but it is okay with me,
> did you hit the asserts?

containingWindow could be NULL if you are using WebKit without a X server (e.g. a thumbnailer).

>    - Parsing NET_WORKAREA looks right, I would use a IntRect and then get this
> converted to FloatRect (no strong opinion on this).

Why? For style reasons?

[Christian Dywan]

Comment on attachment 21385 [details]
Implement stubs

Clearing the review flag until recent comments are addressed.

[Marco Barisione]

Created attachment 22659 [details]
Implement stubs

- Fixed the non-X11 build issue
- Return values != 0 if the containing window is null

I didn't replace the FloatRect with an IntRect because it would makes the code longer and IMHO less readable (the conversion is needed immediately after the creation of the rect).

The Firefox behaviour seems broken for instace it seems to cache values so it returns the wrong value if you add/remove a monitor and it doesn't consider panels, so screen.availHeight/Width is always = screen.height/width.

[Holger Freyther]

Comment on attachment 22659 [details]
Implement stubs

The code is an improvement. It seems to obey the style (I ignore the NULL) and I think corner cases we might hit can be sorted out later. 

Thank you.

[Jan Alonzo]

landed in r35725