Merge lp:~aacid/qtmir/make_sure_surface_not_null into lp:qtmir

[Albert Astals Cid]

Check for find() result not being null before using it

We do it in onWindowReady, onWindowMoved, onWindowFocusChanged, etc so no reason to not do it in onWindowRemoved

 * Are there any related MPs required for this MP to build/function as expected?
No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
N/A

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

[Daniel d'Andrada (dandrader)]

Ok

review: Approve

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:609
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/544/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/4281/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/4309
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4143
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/4143/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4143
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=zesty/4143/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4143/console
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/4143/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4143
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=zesty/4143/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4143
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/4143/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4143
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=zesty/4143/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/544/rebuild

review: Needs Fixing (continuous-integration)

[lp:~aacid/qtmir/make_sure_surface_not_null]

613. By Alan Griffiths on 2017-03-02

Simplify and remove dead code

614. By Alan Griffiths on 2017-03-02

Put const where "we" like it

615. By Alan Griffiths on 2017-03-03

Move const to the left

616. By Alan Griffiths on 2017-03-13

merge :parent

620. By Gerry Boland on 2017-03-02

Small SurfaceManager Test improvements

621. By Gerry Boland on 2017-03-02

Remove tests for the old delete-self behaviour of MirSurface

622. By Gerry Boland on 2017-03-02

Stop MirSurface deleting itself, ensure SurfaceManager alone manages MirSurface lifetimes

This fixes bugs where a MirSurface would call deleteLater on itself, but SurfaceManager would have no idea and keep a pointer to that MirSurface in its internal list.

Instead SurfaceManager listens for signals from MirSurface and decides when to delete it.

623. By Gerry Boland on 2017-03-02

Cleanup: no need to pass MirSurface out in isBeingDisplayedChanged signal

624. By Gerry Boland on 2017-03-03

Make self-triggered MirSurface deletion happen later, makes code more robust

625. By Gerry Boland on 2017-03-13

Remnant of bad rebase, delete SessionManager reference

626. By Albert Astals Cid on 2017-03-14

Check for find() result not being null before using it

We do it in onWindowReady, onWindowMoved, onWindowFocusChanged, etc so no reason to not do it in onWindowRemoved

627. By Albert Astals Cid on 2017-03-20

Merge

628. By Albert Astals Cid on 2017-03-20

Wrong merge

Unmerged revisions

=== modified file 'src/modules/Unity/Application/CMakeLists.txt'
--- src/modules/Unity/Application/CMakeLists.txt	2017-02-08 12:19:14 +0000
+++ src/modules/Unity/Application/CMakeLists.txt	2017-03-10 12:23:14 +0000
@@ -74,8 +74,6 @@
 # Frig for files that still rely on mirserver-dev
 string(REPLACE ";" " -I" QTMIR_ADD_MIRSERVER "-I ${MIRSERVER_INCLUDE_DIRS}")
 set_source_files_properties(mirsurface.cpp         PROPERTIES COMPILE_FLAGS "${CMAKE_CXXFLAGS} ${QTMIR_ADD_MIRSERVER}")
-set_source_files_properties(mirbuffersgtexture.cpp PROPERTIES COMPILE_FLAGS "${CMAKE_CXXFLAGS} ${QTMIR_ADD_MIRSERVER}")
-set_source_files_properties(surfacemanager.cpp     PROPERTIES COMPILE_FLAGS "${CMAKE_CXXFLAGS} ${QTMIR_ADD_MIRSERVER}")
 
 target_link_libraries(
     unityapplicationplugin

=== modified file 'src/modules/Unity/Application/mirbuffersgtexture.cpp'
--- src/modules/Unity/Application/mirbuffersgtexture.cpp	2015-12-14 09:05:06 +0000
+++ src/modules/Unity/Application/mirbuffersgtexture.cpp	2017-03-10 12:23:14 +0000
@@ -17,12 +17,9 @@
 #include "mirbuffersgtexture.h"
 
 // Mir
-#include <mir/graphics/buffer.h>
 #include <mir/geometry/size.h>
-#include <mir/renderer/gl/texture_source.h>
 
 namespace mg = mir::geometry;
-namespace mrg = mir::renderer::gl;
 
 MirBufferSGTexture::MirBufferSGTexture()
     : QSGTexture()
@@ -54,14 +51,14 @@
 void MirBufferSGTexture::setBuffer(const std::shared_ptr<mir::graphics::Buffer>& buffer)
 {
     m_mirBuffer = buffer;
-    mg::Size size = buffer->size();
+    mg::Size size = m_mirBuffer.size();
     m_height = size.height.as_int();
     m_width = size.width.as_int();
 }
 
 bool MirBufferSGTexture::hasBuffer() const
 {
-    return !!m_mirBuffer;
+    return m_mirBuffer.hasBuffer();
 }
 
 int MirBufferSGTexture::textureId() const
@@ -76,12 +73,7 @@
 
 bool MirBufferSGTexture::hasAlphaChannel() const
 {
-    if (hasBuffer()) {
-        return m_mirBuffer->pixel_format() == mir_pixel_format_abgr_8888
-            || m_mirBuffer->pixel_format() == mir_pixel_format_argb_8888;
-    } else {
-        return false;
-    }
+    return m_mirBuffer.hasAlphaChannel();
 }
 
 void MirBufferSGTexture::bind()
@@ -90,10 +82,5 @@
     glBindTexture(GL_TEXTURE_2D, m_textureId);
     updateBindOptions(true/* force */);
 
-    auto const texture_source =
-        dynamic_cast<mrg::TextureSource*>(m_mirBuffer->native_buffer_base());
-    if (!texture_source)
-        throw std::logic_error("Buffer does not support GL rendering");
-
-    texture_source->gl_bind_to_texture();
+    m_mirBuffer.glBindToTexture();
 }

=== modified file 'src/modules/Unity/Application/mirbuffersgtexture.h'
--- src/modules/Unity/Application/mirbuffersgtexture.h	2015-10-28 13:45:07 +0000
+++ src/modules/Unity/Application/mirbuffersgtexture.h	2017-03-10 12:23:14 +0000
@@ -17,7 +17,7 @@
 #ifndef MIRBUFFERSGTEXTURE_H
 #define MIRBUFFERSGTEXTURE_H
 
-#include <memory>
+#include "mirbuffer.h"
 
 #include <QSGTexture>
 
@@ -44,7 +44,7 @@
     void bind() override;
 
 private:
-    std::shared_ptr<mir::graphics::Buffer> m_mirBuffer;
+    qtmir::MirBuffer m_mirBuffer;
     int m_width;
     int m_height;
     GLuint m_textureId;

=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp	2017-02-09 11:01:37 +0000
+++ src/modules/Unity/Application/mirsurface.cpp	2017-03-10 12:23:14 +0000
@@ -169,7 +169,6 @@
     connect(m_surfaceObserver.get(), &SurfaceObserver::confinesMousePointerChanged, this, &MirSurface::confinesMousePointerChanged);
     m_surfaceObserver->setListener(this);
 
-    //connect(session, &QObject::destroyed, this, &MirSurface::onSessionDestroyed); // TODO try using Shared pointer for lifecycle
     connect(session, &SessionInterface::stateChanged, this, [this]() {
         if (clientIsRunning() && m_pendingResize.isValid()) {
             resize(m_pendingResize.width(), m_pendingResize.height());
@@ -585,9 +584,6 @@
         DEBUG_MSG << "(" << value << ")";
         m_live = value;
         Q_EMIT liveChanged(value);
-        if (m_views.isEmpty() && !m_live) {
-            deleteLater();
-        }
     }
 }
 
@@ -712,9 +708,6 @@
     DEBUG_MSG << "(" << viewId << ")" << " after=" << m_views.count() << " live=" << m_live;
     if (m_views.count() == 0) {
         Q_EMIT isBeingDisplayedChanged();
-        if (m_session.isNull() || !m_live) {
-            deleteLater();
-        }
     }
     updateExposure();
     setViewActiveFocus(viewId, false);
@@ -759,13 +752,6 @@
     return m_currentFrameNumber;
 }
 
-void MirSurface::onSessionDestroyed()
-{
-    if (m_views.isEmpty()) {
-        deleteLater();
-    }
-}
-
 void MirSurface::emitSizeChanged()
 {
     Q_EMIT sizeChanged(m_size);

=== modified file 'src/modules/Unity/Application/mirsurface.h'
--- src/modules/Unity/Application/mirsurface.h	2017-02-02 09:17:48 +0000
+++ src/modules/Unity/Application/mirsurface.h	2017-03-10 12:23:14 +0000
@@ -188,7 +188,6 @@
     void dropPendingBuffer();
     void onAttributeChanged(const MirWindowAttrib, const int);
     void onFramesPostedObserved();
-    void onSessionDestroyed();
     void emitSizeChanged();
     void setCursor(const QCursor &cursor);
     void onCloseTimedOut();

=== modified file 'src/modules/Unity/Application/sessionmanager.cpp'
--- src/modules/Unity/Application/sessionmanager.cpp	2016-11-03 20:17:46 +0000
+++ src/modules/Unity/Application/sessionmanager.cpp	2017-03-10 12:23:14 +0000
@@ -90,6 +90,10 @@
     setObjectName(QStringLiteral("qtmir::SessionManager"));
 }
 
+SessionManager::SessionManager() // for test use only
+    : m_applicationManager(nullptr)
+{}
+
 SessionManager::~SessionManager()
 {
     qCDebug(QTMIR_SESSIONS) << "SessionManager::~SessionManager - this=" << this;

=== modified file 'src/modules/Unity/Application/sessionmanager.h'
--- src/modules/Unity/Application/sessionmanager.h	2016-11-03 20:17:46 +0000
+++ src/modules/Unity/Application/sessionmanager.h	2017-03-10 12:23:14 +0000
@@ -56,11 +56,11 @@
         ApplicationManager* applicationManager,
         QObject *parent = 0
     );
-    ~SessionManager();
+    virtual ~SessionManager();
 
     static SessionManager* singleton();
 
-    SessionInterface *findSession(const mir::scene::Session* session) const;
+    virtual SessionInterface *findSession(const mir::scene::Session* session) const;
 
 Q_SIGNALS:
     void sessionStarting(SessionInterface* session);
@@ -76,6 +76,7 @@
     void onPromptProviderRemoved(const qtmir::PromptSession &promptSession, const std::shared_ptr<mir::scene::Session> &);
 
 protected:
+    SessionManager(); // for test purposes only
 
 private:
     const std::shared_ptr<PromptSessionManager> m_promptSessionManager;

=== modified file 'src/modules/Unity/Application/surfacemanager.cpp'
--- src/modules/Unity/Application/surfacemanager.cpp	2017-02-15 13:21:07 +0000
+++ src/modules/Unity/Application/surfacemanager.cpp	2017-03-10 12:23:14 +0000
@@ -27,9 +27,6 @@
 #include <debughelpers.h>
 #include <mirqtconversion.h>
 
-// Mir
-#include <mir/scene/surface.h>
-
 // Qt
 #include <QGuiApplication>
 
@@ -41,7 +38,8 @@
 using namespace qtmir;
 namespace unityapi = unity::shell::application;
 
-SurfaceManager::SurfaceManager(QObject *)
+
+SurfaceManager::SurfaceManager()
 {
     DEBUG_MSG << "()";
 
@@ -59,6 +57,16 @@
     m_sessionManager = SessionManager::singleton();
 }
 
+SurfaceManager::SurfaceManager(WindowControllerInterface *windowController,
+                               WindowModelNotifier *windowModel,
+                               SessionManager *sessionManager)
+    : m_windowController(windowController)
+    , m_sessionManager(sessionManager)
+{
+    DEBUG_MSG << "()";
+    connectToWindowModelNotifier(windowModel);
+}
+
 void SurfaceManager::connectToWindowModelNotifier(WindowModelNotifier *notifier)
 {
     connect(notifier, &WindowModelNotifier::windowAdded,          this, &SurfaceManager::onWindowAdded,           Qt::QueuedConnection);
@@ -90,27 +98,32 @@
 
 void SurfaceManager::onWindowAdded(const NewWindow &window)
 {
+    const auto &windowInfo = window.windowInfo;
     {
-        std::shared_ptr<mir::scene::Surface> surface = window.surface;
-        DEBUG_MSG << " mir::scene::Surface[type=" << mirSurfaceTypeToStr(surface->type())
-            << ",parent=" << (void*)(surface->parent().get())
-            << ",state=" << mirSurfaceStateToStr(surface->state())
-            << ",top_left=" << toQPoint(surface->top_left())
+        DEBUG_MSG << " mir::scene::Surface[type=" << mirSurfaceTypeToStr(windowInfo.type())
+            << ",parent=" << (void*)(std::shared_ptr<mir::scene::Surface>{windowInfo.parent()}.get())
+            << ",state=" << mirSurfaceStateToStr(windowInfo.state())
+            << ",top_left=" << toQPoint(windowInfo.window().top_left())
             << "]";
     }
 
-    auto mirSession = window.windowInfo.window().application();
+    auto mirSession = windowInfo.window().application();
+
     SessionInterface* session = m_sessionManager->findSession(mirSession.get());
 
-    MirSurface *parentSurface;
-    {
-        std::shared_ptr<mir::scene::Surface> surface = window.windowInfo.window();
-        parentSurface = find(surface->parent());
-    }
-
-    auto surface = new MirSurface(window, m_windowController, session, parentSurface);
+    const auto parentSurface = find(windowInfo.parent());
+    const auto surface = new MirSurface(window, m_windowController, session, parentSurface);
     rememberMirSurface(surface);
 
+    connect(surface, &MirSurface::isBeingDisplayedChanged, this, [this, surface]() {
+        if ((!surface->live() || !surface->session())
+                && !surface->isBeingDisplayed()) {
+            forgetMirSurface(static_cast<MirSurface*>(surface)->window());
+            surface->deleteLater(); // don't delete immediately, slot may be directly connected
+            tracepoint(qtmir, surfaceDestroyed);
+        }
+    });
+
     if (parentSurface) {
         static_cast<MirSurfaceListModel*>(parentSurface->childSurfaceList())->prependSurface(surface);
     }
@@ -126,8 +139,12 @@
 {
     MirSurface *surface = find(windowInfo);
     forgetMirSurface(windowInfo.window());
-    surface->setLive(false);
-    tracepoint(qtmir, surfaceDestroyed);
+    if (surface && surface->isBeingDisplayed()) {
+        surface->setLive(false);
+    } else {
+        delete surface;
+        tracepoint(qtmir, surfaceDestroyed);
+    }
 }
 
 MirSurface *SurfaceManager::find(const miral::WindowInfo &needle) const
@@ -145,16 +162,6 @@
     return nullptr;
 }
 
-MirSurface *SurfaceManager::find(const std::shared_ptr<mir::scene::Surface> &needle) const
-{
-    Q_FOREACH(const auto surface, m_allSurfaces) {
-        if (surface->window() == needle) {
-            return surface;
-        }
-    }
-    return nullptr;
-}
-
 void SurfaceManager::onWindowReady(const miral::WindowInfo &windowInfo)
 {
     if (auto mirSurface = find(windowInfo)) {

=== modified file 'src/modules/Unity/Application/surfacemanager.h'
--- src/modules/Unity/Application/surfacemanager.h	2017-02-02 09:17:48 +0000
+++ src/modules/Unity/Application/surfacemanager.h	2017-03-10 12:23:14 +0000
@@ -39,12 +39,19 @@
     Q_OBJECT
 
 public:
-    explicit SurfaceManager(QObject *parent = 0);
+    explicit SurfaceManager();
     virtual ~SurfaceManager() {}
 
     void raise(unity::shell::application::MirSurfaceInterface *surface) override;
     void activate(unity::shell::application::MirSurfaceInterface *surface) override;
 
+protected:
+    // for testing purposes
+    SurfaceManager(WindowControllerInterface *windowController,
+                   WindowModelNotifier *windowModel,
+                   SessionManager *sessionManager);
+    MirSurface* find(const miral::WindowInfo &needle) const;
+
 private Q_SLOTS:
     void onWindowAdded(const qtmir::NewWindow &windowInfo);
     void onWindowRemoved(const miral::WindowInfo &windowInfo);
@@ -59,9 +66,7 @@
     void connectToWindowModelNotifier(WindowModelNotifier *notifier);
     void rememberMirSurface(MirSurface *surface);
     void forgetMirSurface(const miral::Window &window);
-    MirSurface* find(const miral::WindowInfo &needle) const;
     MirSurface* find(const miral::Window &needle) const;
-    MirSurface* find(const std::shared_ptr<mir::scene::Surface> &needle) const;
 
     QVector<MirSurface*> m_allSurfaces;
 

=== modified file 'src/platforms/mirserver/CMakeLists.txt'
--- src/platforms/mirserver/CMakeLists.txt	2017-02-15 13:21:50 +0000
+++ src/platforms/mirserver/CMakeLists.txt	2017-03-10 12:23:14 +0000
@@ -88,6 +88,7 @@
     setqtcompositor.cpp setqtcompositor.h
     eventdispatch.cpp eventdispatch.h
     promptsessionmanager.cpp promptsessionmanager.h promptsession.h
+    mirbuffer.cpp mirbuffer.h
 )
 
 # These files get entangled by automoc so they need to go together. And some depend on mirserver-dev

=== added file 'src/platforms/mirserver/mirbuffer.cpp'
--- src/platforms/mirserver/mirbuffer.cpp	1970-01-01 00:00:00 +0000
+++ src/platforms/mirserver/mirbuffer.cpp	2017-03-10 12:23:14 +0000
@@ -0,0 +1,70 @@
+/*
+ * Copyright © 2017 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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 "mirbuffer.h"
+
+#include <mir/graphics/buffer.h>
+#include <mir/renderer/gl/texture_source.h>
+
+#include <stdexcept>
+
+qtmir::MirBuffer::MirBuffer() = default;
+qtmir::MirBuffer::~MirBuffer() = default;
+qtmir::MirBuffer::MirBuffer(std::shared_ptr<mir::graphics::Buffer> const &buffer) :
+    m_mirBuffer(buffer)
+{
+}
+
+qtmir::MirBuffer& qtmir::MirBuffer::operator=(std::shared_ptr<mir::graphics::Buffer> const &buffer)
+{
+    m_mirBuffer = buffer;
+    return *this;
+}
+
+bool qtmir::MirBuffer::hasBuffer() const
+{
+    return !!m_mirBuffer;
+}
+
+bool qtmir::MirBuffer::hasAlphaChannel() const
+{
+    return hasBuffer() && 
+        (m_mirBuffer->pixel_format() == mir_pixel_format_abgr_8888
+        || m_mirBuffer->pixel_format() == mir_pixel_format_argb_8888);
+}
+
+mir::geometry::Size qtmir::MirBuffer::size() const
+{
+    return m_mirBuffer->size();
+}
+
+void qtmir::MirBuffer::reset()
+{
+    m_mirBuffer.reset();
+}
+
+void qtmir::MirBuffer::glBindToTexture()
+{
+    namespace mrg = mir::renderer::gl;
+
+    auto const texture_source =
+        dynamic_cast<mrg::TextureSource*>(m_mirBuffer->native_buffer_base());
+    if (!texture_source)
+        throw std::logic_error("Buffer does not support GL rendering");
+
+    texture_source->gl_bind_to_texture();
+}
+

=== added file 'src/platforms/mirserver/mirbuffer.h'
--- src/platforms/mirserver/mirbuffer.h	1970-01-01 00:00:00 +0000
+++ src/platforms/mirserver/mirbuffer.h	2017-03-10 12:23:14 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2017 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * 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 QTMIR_MIR_BUFFER_H
+#define QTMIR_MIR_BUFFER_H
+
+#include <mir/geometry/size.h>
+
+#include <memory>
+
+namespace mir { namespace graphics { class Buffer; }}
+
+namespace qtmir
+{
+class MirBuffer
+{
+public:
+    MirBuffer();
+    ~MirBuffer();
+    explicit MirBuffer(std::shared_ptr<mir::graphics::Buffer> const &buffer);
+    MirBuffer& operator=(std::shared_ptr<mir::graphics::Buffer> const &buffer);
+
+    bool hasBuffer() const;
+    bool hasAlphaChannel() const;
+    mir::geometry::Size size() const;
+
+    void reset();
+    void glBindToTexture();
+
+private:
+    std::shared_ptr<mir::graphics::Buffer> m_mirBuffer;
+};
+}
+
+#endif //QTMIR_MIR_BUFFER_H

=== modified file 'tests/framework/CMakeLists.txt'
--- tests/framework/CMakeLists.txt	2016-12-02 16:22:45 +0000
+++ tests/framework/CMakeLists.txt	2017-03-10 12:23:14 +0000
@@ -46,6 +46,8 @@
 target_link_libraries(
     qtmir-test-framework-static
 
+    unityapplicationplugin
+
     ${GTEST_BOTH_LIBRARIES}
     ${GMOCK_LIBRARIES}
 )

=== added file 'tests/framework/mock_session_manager.h'
--- tests/framework/mock_session_manager.h	1970-01-01 00:00:00 +0000
+++ tests/framework/mock_session_manager.h	2017-03-10 12:23:14 +0000
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2017 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef MOCK_SESSION_MANAGER_H
+#define MOCK_SESSION_MANAGER_H
+
+#include <Unity/Application/sessionmanager.h>
+
+#include <gmock/gmock.h>
+
+namespace qtmir {
+
+struct MockSessionManager : public qtmir::SessionManager
+{
+    MOCK_CONST_METHOD1(findSession, qtmir::SessionInterface*(const mir::scene::Session*));
+};
+
+} // namespace qtmir
+
+#endif // MOCK_SESSION_MANAGER_H

=== added file 'tests/framework/mock_window_controller.h'
--- tests/framework/mock_window_controller.h	1970-01-01 00:00:00 +0000
+++ tests/framework/mock_window_controller.h	2017-03-10 12:23:14 +0000
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2017 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef MOCK_WINDOW_CONTROLLER_H
+#define MOCK_WINDOW_CONTROLLER_H
+
+#include "windowcontrollerinterface.h"
+
+#include <gmock/gmock.h>
+
+class MockWindowController : public qtmir::WindowControllerInterface
+{
+public:
+    MOCK_METHOD1(activate, void(const miral::Window &));
+    MOCK_METHOD1(raise,    void(const miral::Window &));
+
+    MOCK_METHOD2(resize, void(const miral::Window &, const QSize &));
+    MOCK_METHOD2(move,   void(const miral::Window &, const QPoint &));
+
+    MOCK_METHOD1(requestClose, void(const miral::Window &));
+    MOCK_METHOD1(forceClose,   void(const miral::Window &));
+
+    MOCK_METHOD2(requestState, void(const miral::Window &, const Mir::State));
+
+    MOCK_METHOD2(deliverKeyboardEvent, void(const miral::Window &, const MirKeyboardEvent *));
+    MOCK_METHOD2(deliverTouchEvent,    void(const miral::Window &, const MirTouchEvent *));
+    MOCK_METHOD2(deliverPointerEvent,  void(const miral::Window &, const MirPointerEvent *));
+};
+
+#endif // MOCK_WINDOW_CONTROLLER_H

=== modified file 'tests/modules/CMakeLists.txt'
--- tests/modules/CMakeLists.txt	2016-11-03 20:17:46 +0000
+++ tests/modules/CMakeLists.txt	2017-03-10 12:23:14 +0000
@@ -1,6 +1,7 @@
 add_subdirectory(Application)
 add_subdirectory(ApplicationManager)
 add_subdirectory(General)
+add_subdirectory(SessionManager)
 add_subdirectory(SharedWakelock)
-add_subdirectory(SessionManager)
+add_subdirectory(SurfaceManager)
 add_subdirectory(WindowManager)

=== added directory 'tests/modules/SurfaceManager'
=== added file 'tests/modules/SurfaceManager/CMakeLists.txt'
--- tests/modules/SurfaceManager/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ tests/modules/SurfaceManager/CMakeLists.txt	2017-03-10 12:23:14 +0000
@@ -0,0 +1,40 @@
+set(
+  SURFACE_MANAGER_TEST_SOURCES
+  surface_manager_test.cpp
+  ${CMAKE_SOURCE_DIR}/src/common/debughelpers.cpp
+)
+
+include_directories(
+  ${CMAKE_SOURCE_DIR}/src/common
+  ${CMAKE_SOURCE_DIR}/src/platforms/mirserver
+  ${CMAKE_SOURCE_DIR}/src/modules
+  ${CMAKE_SOURCE_DIR}/tests/framework
+)
+
+include_directories(
+  SYSTEM
+  ${APPLICATION_API_INCLUDE_DIRS}
+  ${MIRAL_INCLUDE_DIRS}
+  ${MIRTEST_INCLUDE_DIRS}
+)
+
+add_executable(surfacemanager_test ${SURFACE_MANAGER_TEST_SOURCES})
+
+add_dependencies(surfacemanager_test qtmir-test-framework-static)
+
+target_link_libraries(
+  surfacemanager_test
+
+  unityapplicationplugin
+
+  Qt5::Test
+
+  -L${CMAKE_BINARY_DIR}/tests/framework
+  qtmir-test-framework-static
+
+  ${MIRTEST_LDFLAGS}
+  ${GTEST_BOTH_LIBRARIES}
+  ${GMOCK_LIBRARIES}
+)
+
+add_test(SurfaceManager, surfacemanager_test)

=== added file 'tests/modules/SurfaceManager/surface_manager_test.cpp'
--- tests/modules/SurfaceManager/surface_manager_test.cpp	1970-01-01 00:00:00 +0000
+++ tests/modules/SurfaceManager/surface_manager_test.cpp	2017-03-10 12:23:14 +0000
@@ -0,0 +1,489 @@
+/*
+ * Copyright (C) 2017 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <QLoggingCategory>
+#include <QSignalSpy>
+
+// the test subject
+#include <Unity/Application/surfacemanager.h>
+#include <Unity/Application/mirsurface.h>
+
+// miral
+#include <miral/window.h>
+#include <miral/window_info.h>
+
+// mirtest
+#include <mir/test/doubles/stub_session.h>
+#include <mir/test/doubles/stub_surface.h>
+
+// local
+#include "qtmir_test.h"
+#include "fake_session.h"
+#include "mock_session_manager.h"
+#include "mock_window_controller.h"
+
+using namespace qtmir;
+using StubSurface = mir::test::doubles::StubSurface;
+using StubSession = mir::test::doubles::StubSession;
+
+struct TestableSurfaceManager : public SurfaceManager
+{
+    // just exports the test-only protected constructor
+    TestableSurfaceManager(WindowControllerInterface *windowController,
+                           WindowModelNotifier *windowModel,
+                           SessionManager *sessionManager)
+        : SurfaceManager(windowController, windowModel, sessionManager) {}
+
+    MirSurface* find(const miral::WindowInfo &needle) const
+    {
+        return SurfaceManager::find(needle);
+    }
+};
+
+class SurfaceManagerTests : public ::testing::Test
+{
+public:
+    SurfaceManagerTests()
+    {
+        // We don't want the logging spam cluttering the test results
+        QLoggingCategory::setFilterRules(QStringLiteral("qtmir.*=false"));
+
+        qRegisterMetaType<unity::shell::application::MirSurfaceInterface*>();
+        qRegisterMetaType<QVector<unity::shell::application::MirSurfaceInterface*>>();
+    }
+
+    testing::NiceMock<MockSessionManager> sessionManager;
+    testing::NiceMock<MockWindowController> wmController;
+    WindowModelNotifier wmNotifier;
+    QScopedPointer<TestableSurfaceManager> surfaceManager;
+    QScopedPointer<QCoreApplication> qtApp; // need to spin event loop for queued connections
+
+    // Needed to create miral::WindowInfo
+    const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()};
+    const std::shared_ptr<StubSurface> stubSurface{std::make_shared<StubSurface>()};
+    const miral::Window window{stubSession, stubSurface};
+    const ms::SurfaceCreationParameters spec;
+    const miral::WindowInfo windowInfo{window, spec};
+    FakeSession fakeSession;
+
+protected:
+    void SetUp() override
+    {
+        int argc = 0;
+        char* argv[0];
+        qtApp.reset(new QCoreApplication(argc, argv));
+
+        using namespace ::testing;
+        ON_CALL(sessionManager,findSession(_))
+                .WillByDefault(Return(&fakeSession));
+
+        surfaceManager.reset(new TestableSurfaceManager(&wmController,
+                                                        &wmNotifier, &sessionManager));
+    }
+};
+
+/*
+ * Test if MirAL notifies that a window was created, SurfaceManager emits the surfaceCreated
+ * signal
+ */
+TEST_F(SurfaceManagerTests, miralWindowCreationCausesSignalEmission)
+{
+    QSignalSpy newMirSurfaceSpy(surfaceManager.data(), &SurfaceManager::surfaceCreated);
+
+    // Test
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    EXPECT_EQ(1, newMirSurfaceSpy.count());
+}
+
+
+/*
+ * Test if MirAL notifies that a window was created, SurfaceManager emits the surfaceCreated
+ * signal with a corresponding MirSurface
+ */
+TEST_F(SurfaceManagerTests, miralWindowCreationCausesMirSurfaceCreation)
+{
+    QSignalSpy newMirSurfaceSpy(surfaceManager.data(), &SurfaceManager::surfaceCreated);
+
+    // Test
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    auto mirSurface = qvariant_cast<MirSurface*>(newMirSurfaceSpy.takeFirst().at(0));
+    ASSERT_TRUE(mirSurface);
+    EXPECT_EQ(window, mirSurface->window());
+}
+
+/*
+ * Test if MirAL notifies that a window was created, SurfaceManager adds the corresponding
+ * MirSurface to its internal list
+ */
+TEST_F(SurfaceManagerTests, miralWindowCreationAddsMirSurfaceToItsInternalList)
+{
+    // Test
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+    EXPECT_EQ(window, mirSurface->window());
+}
+
+/*
+ * Test SurfaceManager creates a MirSurface with a Session associated
+ */
+TEST_F(SurfaceManagerTests, createdMirSurfaceHasSessionSet)
+{
+    // Setup
+    using namespace ::testing;
+    EXPECT_CALL(sessionManager,findSession(_))
+            .WillOnce(Return(&fakeSession));
+
+    // Test
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+    EXPECT_EQ(&fakeSession, mirSurface->session());
+}
+
+/*
+ * Test when MirAL creates a surface with a parent, SurfaceManager correctly associates
+ * the parent MirSurface to the MirSurface
+ */
+TEST_F(SurfaceManagerTests, parentedMiralWindowGeneratesMirSurfaceWithCorrectParent)
+{
+    // Setup
+    miral::Window parentWindow(stubSession, stubSurface);
+    miral::Window childWindow(stubSession, stubSurface);
+    miral::WindowInfo parentWindowInfo(parentWindow, spec);
+    miral::WindowInfo childWindowInfo(childWindow, spec);
+    childWindowInfo.parent(parentWindow);
+
+    // Test
+    Q_EMIT wmNotifier.windowAdded(parentWindowInfo);
+    Q_EMIT wmNotifier.windowAdded(childWindowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    auto childMirSurface = surfaceManager->find(childWindowInfo);
+    auto parentMirSurface = surfaceManager->find(parentWindowInfo);
+    ASSERT_TRUE(childMirSurface);
+    ASSERT_TRUE(parentMirSurface);
+
+    EXPECT_EQ(parentMirSurface, childMirSurface->parentSurface());
+}
+
+/*
+ * Test when MirAL creates a surface with a parent, SurfaceManager correctly associates
+ * the child MirSurface to the parent MirSurface
+ */
+TEST_F(SurfaceManagerTests, miralWindowWithChildHasMirSurfaceWithCorrectChild)
+{
+    // Setup
+    miral::Window parentWindow(stubSession, stubSurface);
+    miral::Window childWindow(stubSession, stubSurface);
+    miral::WindowInfo parentWindowInfo(parentWindow, spec);
+    miral::WindowInfo childWindowInfo(childWindow, spec);
+    childWindowInfo.parent(parentWindow);
+
+    // Test
+    Q_EMIT wmNotifier.windowAdded(parentWindowInfo);
+    Q_EMIT wmNotifier.windowAdded(childWindowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    auto childMirSurface = surfaceManager->find(childWindowInfo);
+    auto parentMirSurface = surfaceManager->find(parentWindowInfo);
+    ASSERT_TRUE(childMirSurface);
+    ASSERT_TRUE(parentMirSurface);
+
+    ASSERT_EQ(1, parentMirSurface->childSurfaceList()->count());
+    EXPECT_EQ(childMirSurface, parentMirSurface->childSurfaceList()->first());
+}
+
+/*
+ * Test if MirAL notifies that a window is ready, SurfaceManager updates the corresponding
+ * MirSurface causing it to emit a ready() signal
+ */
+TEST_F(SurfaceManagerTests, miralWindowReadyUpdatesMirSurfaceState)
+{
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+
+    QSignalSpy mirSurfaceReadySpy(mirSurface, &MirSurface::ready);
+
+    // Test
+    Q_EMIT wmNotifier.windowReady(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    EXPECT_EQ(1, mirSurfaceReadySpy.count());
+}
+
+/*
+ * Test if MirAL notifies that a window is moved, SurfaceManager updates the corresponding
+ * MirSurface position
+ */
+TEST_F(SurfaceManagerTests, miralWindowMoveUpdatesMirSurfacePosition)
+{
+    QPoint newPosition(222,333);
+
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+
+    QSignalSpy mirSurfacePositionSpy(mirSurface, &MirSurface::positionChanged);
+
+    // Test
+    Q_EMIT wmNotifier.windowMoved(windowInfo, newPosition);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    EXPECT_EQ(1, mirSurfacePositionSpy.count());
+    EXPECT_EQ(mirSurface->position(), newPosition);
+}
+
+/*
+ * Test if MirAL notifies that a window's focus state changes, SurfaceManager updates the corresponding
+ * MirSurface focus state
+ */
+TEST_F(SurfaceManagerTests, miralWindowFocusChangeUpdatesMirSurfaceFocus)
+{
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+    ASSERT_FALSE(mirSurface->focused()); // false must be the initial state
+
+    QSignalSpy mirSurfaceFocusSpy(mirSurface, &MirSurface::focusedChanged);
+
+    // Test
+    Q_EMIT wmNotifier.windowFocusChanged(windowInfo, true);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    EXPECT_EQ(1, mirSurfaceFocusSpy.count());
+    EXPECT_EQ(mirSurface->focused(), true);
+}
+
+/*
+ * Test if MirAL notifies that a window's state changes, SurfaceManager updates the corresponding
+ * MirSurface state (just testing a single state, see no value in testing all possible states here)
+ */
+TEST_F(SurfaceManagerTests, miralWindowStateChangeUpdatesMirSurfaceState)
+{
+    auto newState = Mir::FullscreenState;
+
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+
+    QSignalSpy mirSurfaceStateSpy(mirSurface, &MirSurface::stateChanged);
+
+    // Test
+    Q_EMIT wmNotifier.windowStateChanged(windowInfo, newState);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    EXPECT_EQ(1, mirSurfaceStateSpy.count());
+    EXPECT_EQ(mirSurface->state(), newState);
+}
+
+/*
+ * Test when miral raises a list of surfaces, the raise signal is fired by SurfaceManager with
+ * a list of MirSurfaces in the matching order
+ */
+TEST_F(SurfaceManagerTests, miralsRaiseWindowListTransformedToVectorOfMirSurfaces)
+{
+    // Setup
+    miral::Window window1(stubSession, stubSurface);
+    miral::Window window2(stubSession, stubSurface);
+    miral::WindowInfo windowInfo1(window1, spec);
+    miral::WindowInfo windowInfo2(window2, spec);
+
+    // Setup: add 2 windows and get their MirSurfaces
+    Q_EMIT wmNotifier.windowAdded(windowInfo1);
+    Q_EMIT wmNotifier.windowAdded(windowInfo2);
+    qtApp->sendPostedEvents();
+    auto mirSurface1 = surfaceManager->find(windowInfo1);
+    auto mirSurface2 = surfaceManager->find(windowInfo2);
+    ASSERT_TRUE(mirSurface1);
+    ASSERT_TRUE(mirSurface2);
+
+    QSignalSpy mirSurfacesRaisedSpy(surfaceManager.data(), &SurfaceManager::surfacesRaised);
+
+    // Test
+    std::vector<miral::Window> raiseWindowList{window2, window1};
+    Q_EMIT wmNotifier.windowsRaised(raiseWindowList);
+    qtApp->sendPostedEvents();
+
+    // Check results
+    ASSERT_EQ(1, mirSurfacesRaisedSpy.count());
+    auto raiseMirSurfaceList = qvariant_cast<QVector<unity::shell::application::MirSurfaceInterface*>>(
+                                                 mirSurfacesRaisedSpy.takeFirst().at(0)); // first argument of signal
+    ASSERT_EQ(2, raiseMirSurfaceList.count());
+    EXPECT_EQ(mirSurface1, raiseMirSurfaceList.at(1));
+    EXPECT_EQ(mirSurface2, raiseMirSurfaceList.at(0));
+}
+
+/*
+ * Test focus requests fire focusRequested signal of the MirSurface
+ */
+TEST_F(SurfaceManagerTests, focusRequestCausesMirSurfaceToFireFocusRequestedSignal)
+{
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+
+    QSignalSpy mirSurfaceFocusRequestedSpy(mirSurface, &MirSurface::focusRequested);
+
+    // Test
+    Q_EMIT wmNotifier.windowRequestedRaise(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    EXPECT_EQ(1, mirSurfaceFocusRequestedSpy.count());
+}
+
+/*
+ * If MirAL notifies that a window was removed, and its corresponding MirSurface is not
+ * being displayed, test that SurfaceManager removes the corresponding MirSurface from
+ * its internal list and deletes the MirSurface
+ */
+TEST_F(SurfaceManagerTests, miralWindowRemovedDeletesSurfaceManagerInternalEntryAndMirSurface)
+{
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+
+    QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
+
+    // Test
+    Q_EMIT wmNotifier.windowRemoved(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    ASSERT_EQ(2, mirSurfaceDestroyedSpy.count()); //FIXME - should be 1
+    EXPECT_FALSE(surfaceManager->find(windowInfo));
+}
+
+/*
+ * If MirAL notifies that a window was removed, and its corresponding MirSurface *is*
+ * being displayed, test that SurfaceManager removes the corresponding MirSurface from
+ * its internal list but does *not* delete the MirSurface, but sets it as not "live"
+ */
+TEST_F(SurfaceManagerTests, miralWindowRemovedDeletesSurfaceManagerInternalEntryButNotMirSurface)
+{
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+
+    QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
+
+    mirSurface->registerView(1);
+
+    // Test
+    Q_EMIT wmNotifier.windowRemoved(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    ASSERT_EQ(0, mirSurfaceDestroyedSpy.count());
+    EXPECT_FALSE(surfaceManager->find(windowInfo));
+    EXPECT_FALSE(mirSurface->live());
+}
+
+/*
+ * If MirAL notifies that a window was removed, and its corresponding MirSurface *is*
+ * being displayed, SurfaceManager does *not* delete the MirSurface. Later when that
+ * MirSurface is not being displayed any more, SurfaceManager should delete it.
+ */
+TEST_F(SurfaceManagerTests, miralWindowRemovedSurfaceManagerDeletesMirSurfaceWhenItDoneWith)
+{
+    int viewId = 99;
+
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+
+    QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
+
+    // Setup: indicate MirSurface is being displayed
+    mirSurface->registerView(viewId);
+
+    // Setup: notify that window removed
+    Q_EMIT wmNotifier.windowRemoved(windowInfo);
+    qtApp->sendPostedEvents();
+
+    // Test
+    mirSurface->unregisterView(viewId);
+    // MirSurface is deleteLater()ed by SurfaceManager, so need to spin event loop.
+    // But DeferredDelete is special: likes to be called out specifically or it won't come out
+    qtApp->sendPostedEvents(mirSurface, QEvent::DeferredDelete);
+    qtApp->sendPostedEvents();
+
+    // Check result
+    ASSERT_EQ(2, mirSurfaceDestroyedSpy.count()); //FIXME - should be 1
+}
+
+/*
+ * Test that if a MirSurface is live, and stops being displayed, SurfaceManager does *not*
+ * delete the MirSurface.
+ */
+TEST_F(SurfaceManagerTests, surfaceManagerDoesNotDeleteLiveMirSurfaceWhenStopsBeingDisplayed)
+{
+    int viewId = 99;
+
+    // Setup: add window and get corresponding MirSurface
+    Q_EMIT wmNotifier.windowAdded(windowInfo);
+    qtApp->sendPostedEvents();
+    auto mirSurface = surfaceManager->find(windowInfo);
+    ASSERT_TRUE(mirSurface);
+    ASSERT_TRUE(mirSurface->live());
+
+    QSignalSpy mirSurfaceDestroyedSpy(mirSurface, &QObject::destroyed);
+
+    // Setup: indicate MirSurface is being displayed
+    mirSurface->registerView(viewId);
+
+    // Test
+    mirSurface->unregisterView(viewId);
+
+    // Check result
+    ASSERT_EQ(0, mirSurfaceDestroyedSpy.count());
+}

=== modified file 'tests/modules/WindowManager/mirsurface_test.cpp'
--- tests/modules/WindowManager/mirsurface_test.cpp	2017-02-02 09:29:46 +0000
+++ tests/modules/WindowManager/mirsurface_test.cpp	2017-03-10 12:23:14 +0000
@@ -110,66 +110,6 @@
     ASSERT_TRUE(spyFrameDropped.count() > 0);
 }
 
-
-TEST_F(MirSurfaceTest, DeleteMirSurfaceOnLastNonLiveUnregisterView)
-{
-    int argc = 0;
-    char* argv[0];
-    QCoreApplication qtApp(argc, argv); // app for deleteLater event
-
-    miral::Window mockWindow(stubSession, stubSurface);
-    ms::SurfaceCreationParameters spec;
-    miral::WindowInfo mockWindowInfo(mockWindow, spec);
-
-    auto surface = new MirSurface(mockWindowInfo, nullptr); // lives on the heap, as it will delete itself
-
-    bool surfaceDeleted = false;
-    QObject::connect(surface, &QObject::destroyed, [&surfaceDeleted](){ surfaceDeleted = true; });
-
-    qintptr view1 = (qintptr)1;
-    qintptr view2 = (qintptr)2;
-
-    surface->registerView(view1);
-    surface->registerView(view2);
-    surface->setLive(false);
-    EXPECT_FALSE(surfaceDeleted);
-
-    surface->unregisterView(view1);
-    surface->unregisterView(view2);
-
-    QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
-    EXPECT_TRUE(surfaceDeleted);
-}
-
-
-TEST_F(MirSurfaceTest, DISABLED_DoNotDeleteMirSurfaceOnLastLiveUnregisterView)
-{
-    int argc = 0;
-    char* argv[0];
-    QCoreApplication qtApp(argc, argv); // app for deleteLater event
-
-    miral::Window mockWindow(stubSession, stubSurface);
-    ms::SurfaceCreationParameters spec;
-    miral::WindowInfo mockWindowInfo(mockWindow, spec);
-
-    auto surface = new MirSurface(mockWindowInfo, nullptr); // lives on the heap, as it may delete itself
-
-    bool surfaceDeleted = false;
-    QObject::connect(surface, &QObject::destroyed, [&surfaceDeleted](){ surfaceDeleted = true; });
-
-    qintptr view1 = (qintptr)1;
-    qintptr view2 = (qintptr)2;
-
-    surface->registerView(view1);
-    surface->registerView(view2);
-
-    surface->unregisterView(view1);
-    surface->unregisterView(view2);
-
-    QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
-    EXPECT_FALSE(surfaceDeleted);
-}
-
 /*
  * Test that MirSurface.visible is recalculated after the client swaps the first frame.
  * A surface is not considered visible unless it has a non-hidden & non-minimized state, and