Bug 16881

Summary: [GTK] PlatformScreenGtk is unimplemented
Product: WebKit Reporter: Christian Dywan <christian>
Component: WebKitGTKAssignee: Marco Barisione <marco.barisione>
Status: RESOLVED FIXED    
Severity: Normal CC: alp, pierre-luc.beaudoin
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 19143    
Attachments:
Description Flags
Implement stubs
none
Implement stubs, updated
darin: review-
Updated patch based on both Christian's and Pierre-Luc's patches
none
Updated patch that implements screenAvailableRect()
none
Implemente stubs
none
Implement stubs
alp: review-
Implement stubs
none
Implement stubs zecke: review+

Description Christian Dywan 2008-01-15 07:00:56 PST
PlatformScreenGtk is mostly unimplemented.
Comment 1 Christian Dywan 2008-01-16 02:44:29 PST
Created attachment 18466 [details]
Implement stubs
Comment 2 Alp Toker 2008-01-16 05:53:15 PST
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 mike.emmel@gmail.com 
>+ * Copyright (C) 2006 Michael Emmel mike.emmel@gmail.com
>+ * Copyright (C) 2008 Christian Dywan <christian@imendio.com>
>  * 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.
Comment 3 Christian Dywan 2008-01-18 03:09:13 PST
Created attachment 18525 [details]
Implement stubs, updated
Comment 4 Darin Adler 2008-01-26 08:43:06 PST
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?
Comment 5 Pierre-Luc Beaudoin 2008-03-13 05:30:08 PDT
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.
Comment 6 Marco Barisione 2008-03-19 10:55:32 PDT
Created attachment 19885 [details]
Updated patch based on both Christian's and Pierre-Luc's patches

Tomorrow I will try to fix screenAvailableRect().
Comment 7 Marco Barisione 2008-03-20 12:01:25 PDT
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?
Comment 8 Marco Barisione 2008-03-21 04:06:22 PDT
If xinerama is used qt just uses the entire screen rect, ignoring panels.
Comment 9 Marco Barisione 2008-05-22 10:18:12 PDT
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
Comment 10 Marco Barisione 2008-05-22 10:41:58 PDT
Created attachment 21296 [details]
Implement stubs

Sorry, the previous patch contained some debug code.
Comment 11 Alp Toker 2008-05-23 07:46:41 PDT
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.
Comment 12 Marco Barisione 2008-05-27 12:19:23 PDT
(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?

Comment 13 Marco Barisione 2008-05-28 03:53:42 PDT
Created attachment 21385 [details]
Implement stubs
Comment 14 Alp Toker 2008-06-08 15:46:45 PDT
(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.
Comment 15 Holger Freyther 2008-06-23 09:40:38 PDT
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).

Comment 16 Marco Barisione 2008-06-24 05:40:15 PDT
(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?
Comment 17 Christian Dywan 2008-06-29 11:02:37 PDT
Comment on attachment 21385 [details]
Implement stubs

Clearing the review flag until recent comments are addressed.
Comment 18 Marco Barisione 2008-08-05 12:40:10 PDT
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.
Comment 19 Holger Freyther 2008-08-12 13:21:33 PDT
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.
Comment 20 Jan Alonzo 2008-08-13 12:57:05 PDT
landed in r35725