Do not use a shared pointer for the Xcb::Wrapper

It's not really needed, the required functionality can be achieved in a
more implicit way. The reply pointer is managed by the Wrapper class as
long as the method take() is not invoked. This method follows the
semantics of QScopedPointer::take(). That is the pointer is set to null
and the responsibility to free the pointer is passed to the callee.

By this change we do not have the overhead of creating a QSharedPointer.

In addition the Wrapper provides a copy ctor and assignment operator also
using the semantics of take().

effects.cpp

@@ -1145,12 +1145,14 @@ void EffectsHandlerImpl::checkInputWindowStacking()
     if (input_windows.count() == 0)
         return;
     xcb_window_t* wins = new xcb_window_t[input_windows.count()];
-    QList<Xcb::WindowAttributes> attributes;
+    QVector<Xcb::WindowAttributes> attributes(input_windows.count());
-    foreach (const InputWindowPair &it, input_windows) {
-        attributes << Xcb::WindowAttributes(it.second);
-    }
     int pos = 0;
-    for (QList<Xcb::WindowAttributes>::iterator it = attributes.begin(); it != attributes.end(); ++it) {
+    foreach (const InputWindowPair &it, input_windows) {
+        attributes[pos] = Xcb::WindowAttributes(it.second);
+        pos++;
+    }
+    pos = 0;
+    for (QVector<Xcb::WindowAttributes>::iterator it = attributes.begin(); it != attributes.end(); ++it) {
         if (*it && (*it)->map_state != XCB_MAP_STATE_UNMAPPED) {
             wins[pos++] = (*it).window();
         }

tests/CMakeLists.txt

@@ -50,3 +50,19 @@ target_link_libraries( testClientMachine
                        ${XCB_XCB_LIBRARIES}
                        ${X11_XCB_LIBRARIES}
 )
+
+########################################################
+# Test XcbWrapper
+########################################################
+set( testXcbWrapper_SRCS
+     test_xcb_wrapper.cpp
+)
+kde4_add_unit_test( testXcbWrapper TESTNAME kwin-TestXcbWrapper ${testXcbWrapper_SRCS} )
+
+target_link_libraries( testXcbWrapper
+                       ${QT_QTTEST_LIBRARY}
+                       ${QT_QTCORE_LIBRARY}
+                       ${QT_QTGUI_LIBRARY}
+                       ${XCB_XCB_LIBRARIES}
+                       ${X11_XCB_LIBRARIES}
+)

tests/test_client_machine.cpp

@@ -17,6 +17,7 @@ GNU General Public License for more details.
 You should have received a copy of the GNU General Public License
 along with this program.  If not, see <http://www.gnu.org/licenses/>.
 *********************************************************************/
+#include "testutils.h"
 // KWin
 #include "../client_machine.h"
 #include "../utils.h"
@@ -66,24 +67,12 @@ private slots:
     void emptyHostName();
 
 private:
-    xcb_window_t createWindow();
     void setClientMachineProperty(xcb_window_t window, const QByteArray &hostname);
     xcb_window_t m_testWindow;
     QByteArray m_hostName;
     QByteArray m_fqdn;
 };
 
-xcb_window_t TestClientMachine::createWindow()
-{
-    xcb_window_t w = xcb_generate_id(connection());
-    const uint32_t values[] = { true };
-    xcb_create_window(connection(), 0, w, rootWindow(),
-                      0, 0, 10, 10,
-                      0, XCB_WINDOW_CLASS_INPUT_ONLY, XCB_COPY_FROM_PARENT,
-                      XCB_CW_OVERRIDE_REDIRECT, values);
-    return w;
-}
-
 void TestClientMachine::setClientMachineProperty(xcb_window_t window, const QByteArray &hostname)
 {
     xcb_change_property(connection(), XCB_PROP_MODE_REPLACE, window,
@@ -188,10 +177,5 @@ void TestClientMachine::emptyHostName()
     QCOMPARE(spy.isEmpty(), false);
 }
 
-// need to implement main manually as we need a QApplication for X11 interaction
+KWIN_TEST_MAIN(TestClientMachine)
-int main(int argc, char *argv[]) {
-    QApplication app(argc, argv);
-    TestClientMachine tc;
-    return QTest::qExec(&tc, argc, argv);
-}
 #include "test_client_machine.moc"

tests/test_xcb_wrapper.cpp

@@ -0,0 +1,214 @@
+/********************************************************************
+KWin - the KDE window manager
+This file is part of the KDE project.
+
+Copyright (C) 2013 Martin Gräßlin <mgraesslin@kde.org>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*********************************************************************/
+#include "testutils.h"
+// KWin
+#include "../xcbutils.h"
+// Qt
+#include <QApplication>
+#include <QtTest/QtTest>
+// xcb
+#include <xcb/xcb.h>
+
+using namespace KWin;
+using namespace KWin::Xcb;
+
+class TestXcbWrapper : public QObject
+{
+    Q_OBJECT
+private slots:
+    void init();
+    void cleanup();
+    void defaultCtor();
+    void normalCtor();
+    void copyCtorEmpty();
+    void copyCtorBeforeRetrieve();
+    void copyCtorAfterRetrieve();
+    void assignementEmpty();
+    void assignmentBeforeRetrieve();
+    void assignmentAfterRetrieve();
+    void discard();
+private:
+    void testEmpty(WindowGeometry &geometry);
+    void testGeometry(WindowGeometry &geometry, const QRect &rect);
+    xcb_window_t m_testWindow;
+};
+
+void TestXcbWrapper::init()
+{
+    m_testWindow = XCB_WINDOW_NONE;
+}
+
+void TestXcbWrapper::cleanup()
+{
+    if (m_testWindow != XCB_WINDOW_NONE) {
+        xcb_destroy_window(connection(), m_testWindow);
+    }
+}
+
+void TestXcbWrapper::testEmpty(WindowGeometry &geometry)
+{
+    QCOMPARE(geometry.window(), noneWindow());
+    QVERIFY(!geometry.data());
+    QCOMPARE(geometry.isNull(), true);
+    QCOMPARE(geometry.rect(), QRect());
+    QVERIFY(!geometry);
+}
+
+void TestXcbWrapper::testGeometry(WindowGeometry &geometry, const QRect &rect)
+{
+    QCOMPARE(geometry.window(), m_testWindow);
+    // now lets retrieve some data
+    QCOMPARE(geometry.rect(), rect);
+    QVERIFY(geometry.isRetrieved());
+    QCOMPARE(geometry.isNull(), false);
+    QVERIFY(geometry);
+    QVERIFY(geometry.data());
+    QCOMPARE(geometry.data()->x, int16_t(rect.x()));
+    QCOMPARE(geometry.data()->y, int16_t(rect.y()));
+    QCOMPARE(geometry.data()->width, uint16_t(rect.width()));
+    QCOMPARE(geometry.data()->height, uint16_t(rect.height()));
+}
+
+void TestXcbWrapper::defaultCtor()
+{
+    WindowGeometry geometry;
+    testEmpty(geometry);
+    QVERIFY(!geometry.isRetrieved());
+}
+
+void TestXcbWrapper::normalCtor()
+{
+    m_testWindow = createWindow();
+    QVERIFY(m_testWindow != noneWindow());
+
+    WindowGeometry geometry(m_testWindow);
+    QVERIFY(!geometry.isRetrieved());
+    testGeometry(geometry, QRect(0, 0, 10, 10));
+}
+
+void TestXcbWrapper::copyCtorEmpty()
+{
+    WindowGeometry geometry;
+    WindowGeometry other(geometry);
+    testEmpty(geometry);
+    QVERIFY(geometry.isRetrieved());
+    testEmpty(other);
+    QVERIFY(!other.isRetrieved());
+}
+
+void TestXcbWrapper::copyCtorBeforeRetrieve()
+{
+    m_testWindow = createWindow();
+    QVERIFY(m_testWindow != noneWindow());
+    WindowGeometry geometry(m_testWindow);
+    QVERIFY(!geometry.isRetrieved());
+    WindowGeometry other(geometry);
+    testEmpty(geometry);
+    QVERIFY(geometry.isRetrieved());
+
+    QVERIFY(!other.isRetrieved());
+    testGeometry(other, QRect(0, 0, 10, 10));
+}
+
+void TestXcbWrapper::copyCtorAfterRetrieve()
+{
+    m_testWindow = createWindow();
+    QVERIFY(m_testWindow != noneWindow());
+    WindowGeometry geometry(m_testWindow);
+    QVERIFY(geometry);
+    QVERIFY(geometry.isRetrieved());
+    QCOMPARE(geometry.rect(), QRect(0, 0, 10, 10));
+    WindowGeometry other(geometry);
+    testEmpty(geometry);
+    QVERIFY(geometry.isRetrieved());
+
+    QVERIFY(other.isRetrieved());
+    testGeometry(other, QRect(0, 0, 10, 10));
+}
+
+void TestXcbWrapper::assignementEmpty()
+{
+    WindowGeometry geometry;
+    WindowGeometry other;
+    testEmpty(geometry);
+    testEmpty(other);
+
+    other = geometry;
+    QVERIFY(geometry.isRetrieved());
+    testEmpty(geometry);
+    testEmpty(other);
+    QVERIFY(!other.isRetrieved());
+}
+
+void TestXcbWrapper::assignmentBeforeRetrieve()
+{
+    m_testWindow = createWindow();
+    QVERIFY(m_testWindow != noneWindow());
+    WindowGeometry geometry(m_testWindow);
+    WindowGeometry other = geometry;
+    QVERIFY(geometry.isRetrieved());
+    testEmpty(geometry);
+
+    QVERIFY(!other.isRetrieved());
+    testGeometry(other, QRect(0, 0, 10, 10));
+
+    other = WindowGeometry(m_testWindow);
+    QVERIFY(!other.isRetrieved());
+    QCOMPARE(other.window(), m_testWindow);
+    other = WindowGeometry();
+    testEmpty(geometry);
+}
+
+void TestXcbWrapper::assignmentAfterRetrieve()
+{
+    m_testWindow = createWindow();
+    QVERIFY(m_testWindow != noneWindow());
+    WindowGeometry geometry(m_testWindow);
+    QVERIFY(geometry);
+    QVERIFY(geometry.isRetrieved());
+    WindowGeometry other = geometry;
+    testEmpty(geometry);
+
+    QVERIFY(other.isRetrieved());
+    testGeometry(other, QRect(0, 0, 10, 10));
+
+    other = WindowGeometry();
+    testEmpty(other);
+}
+
+void TestXcbWrapper::discard()
+{
+    // discard of reply cannot be tested properly as we cannot check whether the reply has been discarded
+    // therefore it's more or less just a test to ensure that it doesn't crash and the code paths
+    // are taken.
+    WindowGeometry *geometry = new WindowGeometry();
+    delete geometry;
+
+    m_testWindow = createWindow();
+    geometry = new WindowGeometry(m_testWindow);
+    delete geometry;
+
+    geometry = new WindowGeometry(m_testWindow);
+    QVERIFY(geometry->data());
+    delete geometry;
+}
+
+KWIN_TEST_MAIN(TestXcbWrapper)
+#include "test_xcb_wrapper.moc"

tests/testutils.h

@@ -0,0 +1,61 @@
+/********************************************************************
+KWin - the KDE window manager
+This file is part of the KDE project.
+
+Copyright (C) 2013 Martin Gräßlin <mgraesslin@kde.org>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*********************************************************************/
+#ifndef TESTUTILS_H
+#define TESTUTILS_H
+// KWin
+#include <kwinglobals.h>
+// XCB
+#include <xcb/xcb.h>
+
+// like QTEST_MAIN, just using a QApplication for X11 interaction
+#define KWIN_TEST_MAIN(TestObject) \
+int main(int argc, char *argv[]) { \
+    QApplication app(argc, argv); \
+    TestObject tc; \
+    return QTest::qExec(&tc, argc, argv); \
+}
+
+namespace KWin {
+
+/**
+ * Wrapper to create an 0,0x10,10 input only window for testing purposes
+ **/
+static xcb_window_t createWindow()
+{
+    xcb_window_t w = xcb_generate_id(connection());
+    const uint32_t values[] = { true };
+    xcb_create_window(connection(), 0, w, rootWindow(),
+                      0, 0, 10, 10,
+                      0, XCB_WINDOW_CLASS_INPUT_ONLY, XCB_COPY_FROM_PARENT,
+                      XCB_CW_OVERRIDE_REDIRECT, values);
+    return w;
+}
+
+/**
+ * casts XCB_WINDOW_NONE to uint32_t. Needed to make QCOMPARE working.
+ **/
+static uint32_t noneWindow()
+{
+    return XCB_WINDOW_NONE;
+}
+
+} // namespace
+
+#endif

xcbutils.h

@@ -40,48 +40,112 @@ template <typename Reply,
 class Wrapper
 {
 public:
+    Wrapper()
+        : m_retrieved(false)
+        , m_window(XCB_WINDOW_NONE)
+        , m_reply(NULL)
+        {
+            m_cookie.sequence = 0;
+        }
     explicit Wrapper(WindowId window)
         : m_retrieved(false)
         , m_cookie(requestFunc(connection(), window))
         , m_window(window)
+        , m_reply(NULL)
     {
     }
+    explicit Wrapper(const Wrapper &other)
+        : m_retrieved(other.m_retrieved)
+        , m_cookie(other.m_cookie)
+        , m_window(other.m_window)
+        , m_reply(NULL)
+    {
+        takeFromOther(const_cast<Wrapper&>(other));
+    }
     virtual ~Wrapper() {
-        if (!m_retrieved) {
+        cleanup();
-            xcb_discard_reply(connection(), m_cookie.sequence);
+    }
+    inline Wrapper &operator=(const Wrapper &other) {
+        if (this != &other) {
+            // if we had managed a reply, free it
+            cleanup();
+            // copy members
+            m_retrieved = other.m_retrieved;
+            m_cookie = other.m_cookie;
+            m_window = other.m_window;
+            m_reply = other.m_reply;
+            // take over the responsibility for the reply pointer
+            takeFromOther(const_cast<Wrapper&>(other));
         }
+        return *this;
     }
 
     inline const Reply *operator->() {
         getReply();
-        return m_reply.data();
+        return m_reply;
+    }
+    inline bool isNull() {
+        getReply();
+        return m_reply == NULL;
     }
     inline operator bool() {
-        getReply();
+        return !isNull();
-        return !m_reply.isNull();
     }
-    inline const QSharedPointer<Reply> &data() {
+    inline const Reply *data() {
         getReply();
         return m_reply;
     }
     inline WindowId window() const {
         return m_window;
     }
+    inline bool isRetrieved() const {
+        return m_retrieved;
+    }
+    /**
+     * Returns the value of the reply pointer referenced by this object. The reply pointer of
+     * this object will be reset to null. Calling any method which requires the reply to be valid
+     * will crash.
+     *
+     * Callers of this function take ownership of the pointer.
+     **/
+    inline Reply *take() {
+        getReply();
+        Reply *ret = m_reply;
+        m_reply = NULL;
+        m_window = XCB_WINDOW_NONE;
+        return ret;
+    }
 
 protected:
     void getReply() {
-        if (m_retrieved) {
+        if (m_retrieved || !m_cookie.sequence) {
             return;
         }
-        m_reply = QSharedPointer<Reply>(replyFunc(connection(), m_cookie, NULL), &free);
+        m_reply = replyFunc(connection(), m_cookie, NULL);
         m_retrieved = true;
     }
 
 private:
+    inline void cleanup() {
+        if (!m_retrieved && m_cookie.sequence) {
+            xcb_discard_reply(connection(), m_cookie.sequence);
+        } else if (m_reply) {
+            free(m_reply);
+        }
+    }
+    inline void takeFromOther(Wrapper &other) {
+        if (m_retrieved) {
+            m_reply = other.take();
+        } else {
+            //ensure that other object doesn't try to get the reply or discards it in the dtor
+            other.m_retrieved = true;
+            other.m_window = XCB_WINDOW_NONE;
+        }
+    }
     bool m_retrieved;
     Cookie m_cookie;
     WindowId m_window;
-    QSharedPointer<Reply> m_reply;
+    Reply *m_reply;
 };
 
 typedef Wrapper<xcb_get_window_attributes_reply_t, xcb_get_window_attributes_cookie_t, &xcb_get_window_attributes_reply, &xcb_get_window_attributes_unchecked> WindowAttributes;
@@ -90,11 +154,12 @@ typedef Wrapper<xcb_get_window_attributes_reply_t, xcb_get_window_attributes_coo
 class WindowGeometry : public Wrapper<xcb_get_geometry_reply_t, xcb_get_geometry_cookie_t, &xcb_get_geometry_reply, &xcb_get_geometry_unchecked>
 {
 public:
+    WindowGeometry() : Wrapper<xcb_get_geometry_reply_t, xcb_get_geometry_cookie_t, &xcb_get_geometry_reply, &xcb_get_geometry_unchecked>() {}
     explicit WindowGeometry(xcb_window_t window) : Wrapper<xcb_get_geometry_reply_t, xcb_get_geometry_cookie_t, &xcb_get_geometry_reply, &xcb_get_geometry_unchecked>(window) {}
 
     inline QRect rect() {
-        const QSharedPointer<xcb_get_geometry_reply_t> &geometry = data();
+        const xcb_get_geometry_reply_t *geometry = data();
-        if (geometry.isNull()) {
+        if (!geometry) {
             return QRect();
         }
         return QRect(geometry->x, geometry->y, geometry->width, geometry->height);