Summary: | [GTK] PlatformScreenGtk is unimplemented | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Christian Dywan <christian> | ||||||||||||||||||
Component: | WebKitGTK | Assignee: | 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
Christian Dywan
2008-01-15 07:00:56 PST
Created attachment 18466 [details]
Implement stubs
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. Created attachment 18525 [details]
Implement stubs, updated
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?
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. Created attachment 19885 [details]
Updated patch based on both Christian's and Pierre-Luc's patches
Tomorrow I will try to fix screenAvailableRect().
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?
If xinerama is used qt just uses the entire screen rect, ignoring panels. 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 Created attachment 21296 [details]
Implement stubs
Sorry, the previous patch contained some debug code.
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.
(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? Created attachment 21385 [details]
Implement stubs
(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. 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). (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 on attachment 21385 [details]
Implement stubs
Clearing the review flag until recent comments are addressed.
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 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.
landed in r35725 |