GNOME Bugzilla – Bug 666921
x11: Fix the _NET_SUPPORTING_WM_CHECK window fetch to be spec compliant
Last modified: 2012-01-16 15:49:42 UTC
"The child window MUST also have the _NET_SUPPORTING_WM_CHECK property set to the ID of the child window. […] If the _NET_SUPPORTING_WM_CHECK window on the client window is missing or not properly set, clients SHOULD assume that no conforming Window Manager is present." This commit implements that, which allows us to not have to do a XGetWindowProperty() every N seconds when running under a compliant WM. This is also a more correct fix for the bug handled in commit daf29bffeddc02a7ac55905a79d040c299a37eee.
Created attachment 204268 [details] [review] x11: Fix the _NET_SUPPORTING_WM_CHECK window fetch to be spec compliant
Review of attachment 204268 [details] [review]: ::: gdk/x11/gdkscreen-x11.c @@ +1332,3 @@ + + if (x11_screen->wmspec_check_window != None) + return; /* already have it */ I don't think this early exit can work - remember that window manager can change, and we are not monitoring the root window for property changes.
(In reply to comment #2) > ::: gdk/x11/gdkscreen-x11.c > @@ +1332,3 @@ > + > + if (x11_screen->wmspec_check_window != None) > + return; /* already have it */ > > I don't think this early exit can work - remember that window manager can > change, and we are not monitoring the root window for property changes. But we are monitoring this window a bit further down: /* Find out if this WM goes away, so we can reset everything. */ XSelectInput (x11_screen->xdisplay, *xwindow, StructureNotifyMask); And we then handle it going away in gdk_x11_display_translate_event(). Which root window properties do you have in mind?
The property is _NET_SUPPORTING_WM_CHECK. Oh, but I guess you are right the destroy notify should let us notice the wm going away. Have you tested this with a number of wm changes, say fvwm <> twm <> metacity ?
Created attachment 204349 [details] [review] x11: Fix the _NET_SUPPORTING_WM_CHECK window fetch to be spec compliant -- (In reply to comment #4) > Have you tested this with a number of wm changes, say fvwm <> twm <> metacity ? I have now and it did show up an X error. This new patch should be resilient to those.
Bug 607117 has some discussion related to this, btw.
Created attachment 204881 [details] [review] x11: Fix the _NET_SUPPORTING_WM_CHECK window fetch to be spec compliant -- Fixed a theoretical race condition and factored the code a bit.
Created attachment 204923 [details] [review] x11: Fix the _NET_SUPPORTING_WM_CHECK window fetch to be spec compliant -- Bail out early if the property doesn't exist on the root window.
Review of attachment 204923 [details] [review]: Looks safe now. I wonder if we still need to maintain last_wmspec_check_time at all ?
(In reply to comment #9) > Looks safe now. I wonder if we still need to maintain last_wmspec_check_time at > all ? I'd say it doesn't hurt. This comment was there originally after all: /* This function is very slow on every call if you are not running a * spec-supporting WM. For now not optimized, because it isn't in * any critical code paths, but if you used it somewhere that had to * be fast you want to avoid "GTK is slow with old WMs" complaints. * Probably at that point the function should be changed to query * _NET_SUPPORTING_WM_CHECK only once every 10 seconds or something. */
Review of attachment 204923 [details] [review]: Ok, lets leave it in for now, then
Attachment 204923 [details] pushed as f0a80fa - x11: Fix the _NET_SUPPORTING_WM_CHECK window fetch to be spec compliant