Merge lp:~sil2100/compiz/quickfix_1141079_trunk into lp:compiz/0.9.10

[Sam Spilsbury]

Qt is stupid since it uses the stupid X11 protocol - let's not set all the attributes (especially override_redirect) every time, only in cases when it's actually needed (LP: #1141079)

- Problem:

The fix introduced in revision 3606 triggers a regression with Qt menus. It seems that passing on the override_redirect state from xcreatewindow results in this regression happening. Pop-up menus in Qt apps sometimes are detected as a separate window.

- Fix:

We try not to invalidly unnecessarily pass the additional attributes to the event when there is no need. This way the override_redirect attribute is not corrupted and all is cool.

- Tests:

Attached related gtest tests.

(once this branch gets approved, please approve the raring one as well:
https://code.launchpad.net/~sil2100/compiz/quickfix_1141079/+merge/153427 )

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3646
http://jenkins.qa.ubuntu.com/job/compiz-ci/120/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/157/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/509/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/120/rebuild

review: Needs Fixing (continuous-integration)

[Sam Spilsbury (smspillaz)]

I am resubmitting this for now to get a green CI result - there is a race condition which can cause multiple builds' tests to fail due to the way xorg-gtest works.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3646
http://jenkins.qa.ubuntu.com/job/compiz-ci/121/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/158/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/510/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/121/rebuild

review: Needs Fixing (continuous-integration)

[Łukasz Zemczak (sil2100)]

I'm triggering a rebuild again.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3646
http://jenkins.qa.ubuntu.com/job/compiz-ci/124/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/161
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/513

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/124/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/event.cpp'
--- src/event.cpp	2013-02-10 05:01:50 +0000
+++ src/event.cpp	2013-04-04 11:55:39 +0000
@@ -1285,19 +1285,20 @@
 	 * the window to the window list as we might get configure requests
 	 * which require us to stack other windows relative to it. Setting
 	 * some default values if this is the case. */
-	if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
+	if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa)) {
 	    privateScreen.setDefaultWindowAttributes (&wa);
 
-	/* That being said, we should store as much information as possible
-	 * about it. There may be requests relative to this window that could
-	 * use the data in the XCreateWindowEvent structure, especially the
-	 * override redirect state */
-	wa.x = event->xcreatewindow.x;
-	wa.y = event->xcreatewindow.y;
-	wa.width = event->xcreatewindow.width;
-	wa.height = event->xcreatewindow.height;
-	wa.border_width = event->xcreatewindow.border_width;
-	wa.override_redirect = event->xcreatewindow.override_redirect;
+	    /* That being said, we should store as much information as possible
+	     * about it. There may be requests relative to this window that could
+	     * use the data in the XCreateWindowEvent structure, especially the
+	     * override redirect state */
+	    wa.x = event->xcreatewindow.x;
+	    wa.y = event->xcreatewindow.y;
+	    wa.width = event->xcreatewindow.width;
+	    wa.height = event->xcreatewindow.height;
+	    wa.border_width = event->xcreatewindow.border_width;
+	    wa.override_redirect = event->xcreatewindow.override_redirect;
+	}
 
 	foreach (CompWindow *w, screen->windows ())
 	{
@@ -1475,16 +1476,17 @@
 		 * the window to the window list as we might get configure requests
 		 * which require us to stack other windows relative to it. Setting
 		 * some default values if this is the case. */
-		if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
+		if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa)) {
 		    privateScreen.setDefaultWindowAttributes (&wa);
 
-		/* That being said, we should store as much information as possible
-		 * about it. There may be requests relative to this window that could
-		 * use the data in the XCreateWindowEvent structure, especially the
-		 * override redirect state */
-		wa.x = event->xreparent.x;
-		wa.y = event->xreparent.y;
-		wa.override_redirect = event->xreparent.override_redirect;
+		    /* That being said, we should store as much information as possible
+		     * about it. There may be requests relative to this window that could
+		     * use the data in the XCreateWindowEvent structure, especially the
+		     * override redirect state */
+		    wa.x = event->xreparent.x;
+		    wa.y = event->xreparent.y;
+		    wa.override_redirect = event->xreparent.override_redirect;
+		}
 
 		PrivateWindow::createCompWindow (getTopWindow ()->id (), getTopServerWindow ()->id (), wa, event->xreparent.window);
 		break;

=== modified file 'tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp'
--- tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp	2013-03-11 06:38:17 +0000
+++ tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp	2013-04-04 11:55:39 +0000
@@ -212,6 +212,14 @@
 	bool VerifySetFrameExtentsResponse (Window w, int left, int right, int top, int bottom);
 	bool VerifyWindowSize (Window w, int x, int y, int width, int height);
 
+	/* Helper functions for the Create*WindowOverrideRedirect* tests */
+	Window GrabAndCreateWindowWithAttrs (::Display *dpy,
+					     XSetWindowAttributes &attr);
+	void UngrabSyncAndTestOverride (::Display *dpy,
+					Window w,
+					bool overrideAssert);
+
+
     protected:
 
 	ReparentedWindow CreateWindow (::Display *);
@@ -720,6 +728,7 @@
 				currentHeight));
 
     SendSetFrameExtentsRequest (client, left, right, top, bottom);
+    
     ASSERT_TRUE (VerifySetFrameExtentsResponse (client, left, right, top, bottom));
 
     /* Map the window */
@@ -750,3 +759,93 @@
 				   currentWidth,
 				   currentHeight));
 }
+
+
+Window
+CompizXorgSystemConfigureWindowTest::GrabAndCreateWindowWithAttrs (::Display *dpy,
+								   XSetWindowAttributes &attr)
+{
+    /* NOTE: We need to ungrab the server after this function */
+    XGrabServer (dpy);
+
+    /* Create a window with our custom attributes */
+    return XCreateWindow (dpy, DefaultRootWindow (dpy),
+			  0, 0, 100, 100, 0, CopyFromParent,
+			  InputOutput, CopyFromParent, CWOverrideRedirect,
+			  &attr);
+}
+
+void
+CompizXorgSystemConfigureWindowTest::UngrabSyncAndTestOverride (::Display *dpy,
+								Window w,
+								bool overrideAssert)
+{
+    XSync (dpy, false);
+
+    XUngrabServer (dpy);
+
+    XSync (dpy, false);
+
+    /* Check if the created window had the attributes passed correctly */
+    std::vector <long> data = WaitForWindowCreation (w);
+    EXPECT_EQ (overrideAssert, IsOverrideRedirect (data));
+}
+
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateDestroyWindowOverrideRedirectTrue)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 1;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    XDestroyWindow (dpy, w);
+
+    UngrabSyncAndTestOverride (dpy, w, true);
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateDestroyWindowOverrideRedirectFalse)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 0;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    XDestroyWindow (dpy, w);
+
+    UngrabSyncAndTestOverride (dpy, w, false);
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateChangeWindowOverrideRedirectTrue)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 0;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    attr.override_redirect = 1;
+    XChangeWindowAttributes (dpy, w, CWOverrideRedirect, &attr);
+
+    UngrabSyncAndTestOverride (dpy, w, true);
+
+    XDestroyWindow (dpy, w);
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, CreateChangeWindowOverrideRedirectFalse)
+{
+    ::Display *dpy = Display ();
+    XSetWindowAttributes attr;
+    attr.override_redirect = 1;
+
+    Window w = GrabAndCreateWindowWithAttrs (dpy, attr);
+
+    attr.override_redirect = 0;
+    XChangeWindowAttributes (dpy, w, CWOverrideRedirect, &attr);
+
+    UngrabSyncAndTestOverride (dpy, w, false);
+
+    XDestroyWindow (dpy, w);
+}

=== modified file 'tests/xorg-gtest/include/compiz-xorg-gtest.h'
--- tests/xorg-gtest/include/compiz-xorg-gtest.h	2013-02-23 10:47:18 +0000
+++ tests/xorg-gtest/include/compiz-xorg-gtest.h	2013-04-04 11:55:39 +0000
@@ -214,7 +214,8 @@
 	    protected:
 
 		Atom FetchAtom (const char *);
-		void WaitForWindowCreation (Window w);
+		std::vector <long> WaitForWindowCreation (Window w);
+		bool IsOverrideRedirect (std::vector <long> &data);
 
 		virtual int  GetEventMask ();
 

=== modified file 'tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp'
--- tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp	2013-02-14 12:12:53 +0000
+++ tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp	2013-04-04 11:55:39 +0000
@@ -192,6 +192,7 @@
 
     std::vector <long> data;
     data.push_back (static_cast <long> (window->id ()));
+    data.push_back (static_cast <long> (window->overrideRedirect ()));
     ct::SendClientMessage (screen->dpy (),
 			   ts->fetchAtom (ctm::TEST_HELPER_WINDOW_READY),
 			   screen->root (),

=== modified file 'tests/xorg-gtest/src/compiz-xorg-gtest.cpp'
--- tests/xorg-gtest/src/compiz-xorg-gtest.cpp	2013-02-23 08:23:05 +0000
+++ tests/xorg-gtest/src/compiz-xorg-gtest.cpp	2013-04-04 11:55:39 +0000
@@ -738,7 +738,7 @@
 	std::auto_ptr <ct::MessageAtoms> mMessages;
 };
 
-void
+std::vector <long>
 ct::AutostartCompizXorgSystemTestWithTestHelper::WaitForWindowCreation (Window w)
 {
     ::Display *dpy = Display ();
@@ -758,7 +758,19 @@
 
     }
 
-    ASSERT_TRUE (requestAcknowledged);
+    EXPECT_TRUE (requestAcknowledged);
+
+    std::vector <long> data;
+    for (int i = 0; i < 5; ++i)
+	data.push_back(static_cast <long> (event.xclient.data.l[i]));
+
+    return data;
+}
+
+bool
+ct::AutostartCompizXorgSystemTestWithTestHelper::IsOverrideRedirect (std::vector <long> &data)
+{
+    return (data[1] > 0) ? true : false;
 }
 
 Atom