Merge lp://staging/~vanvugt/compiz/fix-1019337.2 into lp://staging/compiz/0.9.8

PASSED: Continuous integration, rev:3285
http://jenkins.qa.ubuntu.com/job/compiz-ci/14/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./distribution=quantal,flavor=amd64/14/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./distribution=quantal,flavor=i386/14/console

Also good,

23 if (result == Success && error == Success && data)

I don't think there's a reason to check for result and error being Success here - my understanding of the protocol is that XGetWindowProperty is a synchronous call and returns its error code (in this case being Success, BadValue, BadAtom or BadWindow) (http://www.x.org/docs/XProtocol/proto.pdf)). However, the error traps are necessary because we don't want the process to abort.

We should find other unguarded X Protocol calls which may also generate such errors too and guard them as well.

If it turns out that the actual XErrorEvent trapped may return something different to XGetWindowProperty then it's an Approve from me, otherwise remove the other redundant check and consider it Approved.

(No tests required in gtk-window-decorator now, there is no testing framework for it)

I'm not going to comment on which result is more important, I just notice that gdk_error_trap_pop is defined to return either X error code or 0 on success.

We are comparing the X function return (an int) against Success, and the g_ func return (a gint) against Success. I suggest we change the error check to be (error == 0). Even if Success is defined to be zero somewhere, I think this makes the API clearer.

In X error codes, "Success" is what you're meant to use:

% grep "#define Success" /usr/include/X11/X.h
#define Success 0 /* everything's okay */

Yes, it is possible result != error. Because all the docs I found said that result is either Success or undefined. However 'error' tells you what the error was.

However it's a moot point since all we're testing for is Success. So there's no need to check 'error == Success' too. It's redundant but harmless.

Looks safe. Didier also pointed that it can't possibly do any harm, so I'm approving.