Merge lp:~kdub/mir/msh-surface-no-builder into lp:~mir-team/mir/trunk

[Kevin DuBois]

msh:: begin changes by digging the destruction of the ms::Surface out of msh::Surface. This makes the constructor for msh::Surface much simpler.

msh:: begin changes by digging the destruction of the ms::Surface out of msh::Surface. This makes the constructor for msh::Surface much simpler.

this is a 'stable intermediate change' from my spike on how to clean up msh::Surface to make it more mockable/testable/cleaner. There are TODO's noting the next steps to take.
The obvious wart is in SurfaceSource, where we pass a std::function for the destruction of the ms::Surface. This will is nicely eliminated by having a complimentary "destroy_surface_for" function to the "create_surface_for" function present in the current SessionManager

From the spike on cleaning this area of the code, (which I've decided to land in bitesized chunks), I isolated a few problems:

1) The ms::Surface is not acquired by the shell in a good RAII way. The surface stack is passed all around shell, when really, the SessionManager in shell should be aware of the SurfaceStack, and manage the Session classes for each client
2) There are a few builder clasess that have weird lifetimes for their underlying factories. In my spike branch, I eliminated two of these.
3) SurfaceController is a bit redundant, what we have in the system today is trending towards an MVC view of the stack that's a little more fine grained than what we had planned many months ago.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:796
http://jenkins.qa.ubuntu.com/job/mir-ci/849/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1076
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/961
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/87
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/87/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/503

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/849/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> The obvious wart is in SurfaceSource, where we pass a std::function for the
> destruction of the ms::Surface. This will is nicely eliminated by having a
> complimentary "destroy_surface_for" function to the "create_surface_for"
> function present in the current SessionManager.

With this planned change a shell::Surface will not know how to properly destroy itself (e.g. remove itself from the SurfaceStack). Introducing SessionManager::destroy_surface_for() will solve this for surfaces that are explicitly released by the application, but I am not clear what's the plan for properly destroying surfaces when a session is shut down or when the mir server is shut down. Will SessionManager::close_session() iterate over a session's surfaces and call destroy_surface_for() for each one?

review: Needs Information

[Kevin DuBois (kdub)]

> > The obvious wart is in SurfaceSource, where we pass a std::function for the
> > destruction of the ms::Surface. This will is nicely eliminated by having a
> > complimentary "destroy_surface_for" function to the "create_surface_for"
> > function present in the current SessionManager.
>
> With this planned change a shell::Surface will not know how to properly
> destroy itself (e.g. remove itself from the SurfaceStack). Introducing
> SessionManager::destroy_surface_for() will solve this for surfaces that are
> explicitly released by the application, but I am not clear what's the plan for
> properly destroying surfaces when a session is shut down or when the mir
> server is shut down. Will SessionManager::close_session() iterate over a
> session's surfaces and call destroy_surface_for() for each one?

I have been iterating on this for a bit, the solution is to have a proxy surface to decouple the lifetimes. However, doing this exposed the tight linkage between input and the surface stack, so this branch is not dependendant on remove-surface-target. Will switch back to WIP for a day or two until that lands

Unmerged revisions

796. By Kevin DuBois on 2013-06-27

remove commented code

795. By Kevin DuBois on 2013-06-27

make a todo comment noting where we have a stable intermediate form

794. By Kevin DuBois on 2013-06-27

clean up references to the builder throughout the tests. re-enable all msh surface tests

793. By Kevin DuBois on 2013-06-27

trim the constructor from the unused params

792. By Kevin DuBois on 2013-06-27

test to pass

791. By Kevin DuBois on 2013-06-27

flip the switch on the constructor (1 failing tes)

790. By Kevin DuBois on 2013-06-27

finish adding constructor del function to move msh::Surface to more raii

789. By Kevin DuBois on 2013-06-27

wrap the destruction into a functor. this is a temporary measure

788. By Kevin DuBois on 2013-06-27

rethink approach to be gentler to compile

787. By Kevin DuBois on 2013-06-27

update surafce test so it doesnt touch builder

=== modified file 'include/server/mir/shell/surface.h'
--- include/server/mir/shell/surface.h	2013-06-25 19:27:35 +0000
+++ include/server/mir/shell/surface.h	2013-06-27 23:29:26 +0000
@@ -26,6 +26,7 @@
 
 #include "mir_toolkit/common.h"
 
+#include <functional>
 #include <string>
 
 namespace mir
@@ -38,19 +39,17 @@
 namespace shell
 {
 class InputTargeter;
-class SurfaceBuilder;
-struct SurfaceCreationParameters;
 
 class Surface : public frontend::Surface
 {
 public:
     Surface(
-        std::shared_ptr<SurfaceBuilder> const& builder,
-        SurfaceCreationParameters const& params);
+        std::weak_ptr<surfaces::Surface> const& surface,
+        std::function<void(std::weak_ptr<surfaces::Surface>)> const& destroyer);
 
     Surface(
-        std::shared_ptr<SurfaceBuilder> const& builder,
-        SurfaceCreationParameters const& params,
+        std::weak_ptr<surfaces::Surface> const& surface,
+        std::function<void(std::weak_ptr<surfaces::Surface>)> const& destroyer,
         frontend::SurfaceId id,
         std::shared_ptr<events::EventSink> const& sink);
 
@@ -93,7 +92,6 @@
     bool set_state(MirSurfaceState s);
     void notify_change(MirSurfaceAttrib attrib, int value);
 
-    std::shared_ptr<SurfaceBuilder> const builder;
     std::weak_ptr<mir::surfaces::Surface> const surface;
 
     frontend::SurfaceId const id;
@@ -101,6 +99,7 @@
 
     MirSurfaceType type_value;
     MirSurfaceState state_value;
+    std::function<void(std::weak_ptr<surfaces::Surface>)> destr; 
 };
 }
 }

=== modified file 'include/test/mir_test_doubles/mock_surface.h'
--- include/test/mir_test_doubles/mock_surface.h	2013-06-25 05:42:49 +0000
+++ include/test/mir_test_doubles/mock_surface.h	2013-06-27 23:29:26 +0000
@@ -36,8 +36,9 @@
 
 struct MockSurface : public shell::Surface
 {
-    MockSurface(std::shared_ptr<shell::SurfaceBuilder> const& builder) :
-        shell::Surface(builder, shell::a_surface())
+    MockSurface() :
+        shell::Surface(std::weak_ptr<surfaces::Surface>(),
+                       [](std::weak_ptr<surfaces::Surface>){})
     {
     }
 

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-06-25 19:28:56 +0000
+++ src/server/shell/surface.cpp	2013-06-27 23:29:26 +0000
@@ -35,28 +35,28 @@
 namespace ms = mir::surfaces;
 
 msh::Surface::Surface(
-    std::shared_ptr<SurfaceBuilder> const& builder,
-    shell::SurfaceCreationParameters const& params,
+    std::weak_ptr<ms::Surface> const& surface,
+    std::function<void(std::weak_ptr<ms::Surface>)> const& destroyer,
     frontend::SurfaceId id,
     std::shared_ptr<events::EventSink> const& sink)
-  : builder(builder),
-    surface(builder->create_surface(params)),
+  : surface(surface),
     id(id),
     event_sink(sink),
     type_value(mir_surface_type_normal),
-    state_value(mir_surface_state_restored)
+    state_value(mir_surface_state_restored),
+    destr(destroyer)
 {
 }
 
 msh::Surface::Surface(
-    std::shared_ptr<SurfaceBuilder> const& builder,
-    shell::SurfaceCreationParameters const& params)
-  : builder(builder),
-    surface(builder->create_surface(params)),
+    std::weak_ptr<ms::Surface> const& surface,
+    std::function<void(std::weak_ptr<ms::Surface>)> const& destroyer)
+  : surface(surface),
     id(),
     event_sink(),
     type_value(mir_surface_type_normal),
-    state_value(mir_surface_state_restored)
+    state_value(mir_surface_state_restored),
+    destr(destroyer)
 {
 }
 
@@ -106,7 +106,7 @@
 
 void msh::Surface::destroy()
 {
-    builder->destroy_surface(surface);
+    destr(surface);
 }
 
 void msh::Surface::force_requests_to_complete()

=== modified file 'src/server/shell/surface_source.cpp'
--- src/server/shell/surface_source.cpp	2013-05-21 22:03:29 +0000
+++ src/server/shell/surface_source.cpp	2013-06-27 23:29:26 +0000
@@ -41,9 +41,15 @@
     frontend::SurfaceId id,
     std::shared_ptr<events::EventSink> const& sink)
 {
+    //TODO: this is still messy. We should only have a simple make_shared here, and the builder
+    //should exist up in the SessionManager 
+    auto surface = surface_builder->create_surface(params);
+    auto fn = [this](std::weak_ptr<ms::Surface> s){
+        surface_builder->destroy_surface(s);
+    };
     return std::make_shared<Surface>(
-        surface_builder,
-        params,
+        surface,
+        fn,
         id,
         sink);
 }

=== modified file 'tests/behavior-tests/session_management_context.cpp'
--- tests/behavior-tests/session_management_context.cpp	2013-06-12 10:27:50 +0000
+++ tests/behavior-tests/session_management_context.cpp	2013-06-27 23:29:26 +0000
@@ -37,6 +37,7 @@
 
 namespace mf = mir::frontend;
 namespace msh = mir::shell;
+namespace ms = mir::surfaces;
 namespace mg = mir::graphics;
 namespace mc = mir::compositor;
 namespace geom = mir::geometry;
@@ -65,18 +66,16 @@
     {
     }
 
-    std::shared_ptr<msh::Surface> create_surface(const msh::SurfaceCreationParameters& params,
+    std::shared_ptr<msh::Surface> create_surface(const msh::SurfaceCreationParameters&,
         frontend::SurfaceId id,
         std::shared_ptr<events::EventSink> const& sink) override
     {
         return std::make_shared<msh::Surface>(
-            mt::fake_shared(surface_builder),
-            params,
+            std::weak_ptr<ms::Surface>(),
+            [](std::weak_ptr<surfaces::Surface>){},
             id,
             sink);
     }
-
-    mtd::StubSurfaceBuilder surface_builder;
 };
 
 class SizedDisplay : public mg::Display

=== modified file 'tests/integration-tests/cucumber/test_session_management_context.cpp'
--- tests/integration-tests/cucumber/test_session_management_context.cpp	2013-05-21 17:16:43 +0000
+++ tests/integration-tests/cucumber/test_session_management_context.cpp	2013-06-27 23:29:26 +0000
@@ -24,7 +24,6 @@
 #include "mir/graphics/viewable_area.h"
 
 #include "mir_test_doubles/mock_session.h"
-#include "mir_test_doubles/stub_surface_builder.h"
 #include "mir_test_doubles/mock_surface.h"
 #include "mir_test_doubles/mock_shell.h"
 #include "mir_test/fake_shared.h"
@@ -136,7 +135,7 @@
     using namespace ::testing;
 
     mtd::MockSession session;
-    mtd::MockSurface surface(std::make_shared<mtd::StubSurfaceBuilder>());
+    mtd::MockSurface surface;
 
     EXPECT_CALL(shell, open_session(test_window_name, _)).Times(1)
         .WillOnce(Return(mt::fake_shared<mf::Session>(session)));

=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
--- tests/unit-tests/frontend/test_session_mediator.cpp	2013-06-25 05:42:49 +0000
+++ tests/unit-tests/frontend/test_session_mediator.cpp	2013-06-27 23:29:26 +0000
@@ -62,7 +62,7 @@
     {
         using namespace ::testing;
 
-        mock_surface = std::make_shared<mtd::MockSurface>(mt::fake_shared(surface_builder));
+        mock_surface = std::make_shared<mtd::MockSurface>();
         mock_buffer = std::make_shared<NiceMock<mtd::MockBuffer>>(geom::Size(), geom::Stride(), geom::PixelFormat());
 
         EXPECT_CALL(*mock_surface, size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));

=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
--- tests/unit-tests/shell/test_application_session.cpp	2013-05-21 18:24:52 +0000
+++ tests/unit-tests/shell/test_application_session.cpp	2013-06-27 23:29:26 +0000
@@ -22,7 +22,6 @@
 #include "mir_test/fake_shared.h"
 #include "mir_test_doubles/mock_surface_factory.h"
 #include "mir_test_doubles/mock_surface.h"
-#include "mir_test_doubles/stub_surface_builder.h"
 #include "mir_test_doubles/stub_surface.h"
 
 #include "mir/shell/surface.h"
@@ -43,8 +42,7 @@
 {
     using namespace ::testing;
 
-    mtd::StubSurfaceBuilder surface_builder;
-    auto const mock_surface = std::make_shared<mtd::MockSurface>(mt::fake_shared(surface_builder));
+    auto const mock_surface = std::make_shared<mtd::MockSurface>();
 
     mtd::MockSurfaceFactory surface_factory;
     ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(Return(mock_surface));
@@ -65,15 +63,14 @@
     using namespace ::testing;
 
     mtd::MockSurfaceFactory surface_factory;
-    mtd::StubSurfaceBuilder surface_builder;
     {
         InSequence seq;
         EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
-            .WillOnce(Return(std::make_shared<NiceMock<mtd::MockSurface>>(mt::fake_shared(surface_builder))));
-        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
-            .WillOnce(Return(std::make_shared<NiceMock<mtd::MockSurface>>(mt::fake_shared(surface_builder))));
-        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
-            .WillOnce(Return(std::make_shared<NiceMock<mtd::MockSurface>>(mt::fake_shared(surface_builder))));
+            .WillOnce(Return(std::make_shared<NiceMock<mtd::MockSurface>>()));
+        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
+            .WillOnce(Return(std::make_shared<NiceMock<mtd::MockSurface>>()));
+        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
+            .WillOnce(Return(std::make_shared<NiceMock<mtd::MockSurface>>()));
     }
 
     msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo");
@@ -100,8 +97,7 @@
 {
     using namespace ::testing;
 
-    mtd::StubSurfaceBuilder surface_builder;
-    auto const mock_surface = std::make_shared<mtd::MockSurface>(mt::fake_shared(surface_builder));
+    auto const mock_surface = std::make_shared<mtd::MockSurface>();
 
     mtd::MockSurfaceFactory surface_factory;
     ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(Return(mock_surface));

=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
--- tests/unit-tests/shell/test_session_manager.cpp	2013-06-19 16:41:13 +0000
+++ tests/unit-tests/shell/test_session_manager.cpp	2013-06-27 23:29:26 +0000
@@ -109,8 +109,9 @@
 
     ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(
        Return(std::make_shared<msh::Surface>(
-           mt::fake_shared(surface_builder),
-           msh::a_surface())));
+           std::weak_ptr<ms::Surface>(),
+           [](std::weak_ptr<ms::Surface>){}
+           )));
 
 
     EXPECT_CALL(container, insert_session(_)).Times(1);
@@ -144,8 +145,9 @@
     using namespace ::testing;
     ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(
         Return(std::make_shared<msh::Surface>(
-            mt::fake_shared(surface_builder),
-            msh::a_surface())));
+            std::weak_ptr<ms::Surface>(),
+            [](std::weak_ptr<ms::Surface>){}
+            )));
 
     // Once for session creation and once for surface creation
     {

=== modified file 'tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp'
--- tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp	2013-06-12 15:36:31 +0000
+++ tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp	2013-06-27 23:29:26 +0000
@@ -29,7 +29,6 @@
 #include "mir_test_doubles/mock_surface_factory.h"
 #include "mir_test_doubles/stub_surface.h"
 #include "mir_test_doubles/mock_surface.h"
-#include "mir_test_doubles/stub_surface_builder.h"
 #include "mir_test_doubles/stub_input_targeter.h"
 #include "mir_test_doubles/mock_input_targeter.h"
 
@@ -95,7 +94,7 @@
     using namespace ::testing;
     
     NiceMock<MockShellSession> app1;
-    mtd::MockSurface mock_surface(std::make_shared<mtd::StubSurfaceBuilder>());
+    mtd::MockSurface mock_surface;
     {
         InSequence seq;
         EXPECT_CALL(app1, default_surface()).Times(1)

=== modified file 'tests/unit-tests/shell/test_surface.cpp'
--- tests/unit-tests/shell/test_surface.cpp	2013-06-25 19:27:35 +0000
+++ tests/unit-tests/shell/test_surface.cpp	2013-06-27 23:29:26 +0000
@@ -23,7 +23,6 @@
 #include "mir/shell/surface_builder.h"
 
 #include "mir_test_doubles/stub_buffer_stream.h"
-#include "mir_test_doubles/mock_buffer_stream.h"
 #include "mir_test_doubles/mock_buffer.h"
 #include "mir_test_doubles/stub_buffer.h"
 #include "mir_test_doubles/mock_input_targeter.h"
@@ -46,152 +45,57 @@
 
 namespace
 {
-class StubSurfaceBuilder : public msh::SurfaceBuilder
-{
-public:
-    StubSurfaceBuilder() :
-        stub_buffer_stream_(std::make_shared<mtd::StubBufferStream>()),
-        dummy_surface()
-    {
-    }
-
-    std::weak_ptr<ms::Surface> create_surface(msh::SurfaceCreationParameters const& )
-    {
-        dummy_surface = std::make_shared<ms::Surface>(msh::a_surface().name, msh::a_surface().top_left, stub_buffer_stream_,
-            std::shared_ptr<mi::InputChannel>(), []{});
-        return dummy_surface;
-    }
-
-    void destroy_surface(std::weak_ptr<ms::Surface> const& )
-    {
-        reset_surface();
-    }
-
-    void reset_surface()
-    {
-        dummy_surface.reset();
-    }
-
-    std::shared_ptr<mtd::StubBufferStream> stub_buffer_stream()
-    {
-        return stub_buffer_stream_;
-    }
-
-private:
-    std::shared_ptr<mtd::StubBufferStream> const stub_buffer_stream_;
-    std::shared_ptr<ms::Surface> dummy_surface;
-};
-
-class MockSurfaceBuilder : public msh::SurfaceBuilder
-{
-public:
-    MockSurfaceBuilder()
-    {
-        using namespace testing;
-        ON_CALL(*this, create_surface(_)).
-            WillByDefault(Invoke(&self, &StubSurfaceBuilder::create_surface));
-
-        ON_CALL(*this, destroy_surface(_)).
-            WillByDefault(Invoke(&self, &StubSurfaceBuilder::destroy_surface));
-    }
-
-    MOCK_METHOD1(create_surface, std::weak_ptr<ms::Surface> (const msh::SurfaceCreationParameters&));
-
-    MOCK_METHOD1(destroy_surface, void (std::weak_ptr<ms::Surface> const&));
-
-private:
-    StubSurfaceBuilder self;
-};
-
-typedef testing::NiceMock<mtd::MockBufferStream> StubBufferStream;
-
-
 struct ShellSurface : testing::Test
 {
-    std::shared_ptr<StubBufferStream> const buffer_stream;
-    StubSurfaceBuilder surface_builder;
+    std::shared_ptr<mtd::StubBufferStream> const stub_buffer_stream;
+    std::shared_ptr<ms::Surface> weak_surface;
 
     ShellSurface() :
-        buffer_stream(std::make_shared<StubBufferStream>())
+        stub_buffer_stream(std::make_shared<mtd::StubBufferStream>())
     {
         using namespace testing;
 
-        ON_CALL(*buffer_stream, stream_size()).WillByDefault(Return(geom::Size()));
-        ON_CALL(*buffer_stream, get_stream_pixel_format()).WillByDefault(Return(geom::PixelFormat::abgr_8888));
-        ON_CALL(*buffer_stream, secure_client_buffer()).WillByDefault(Return(std::shared_ptr<mtd::StubBuffer>()));
+        weak_surface = std::make_shared<ms::Surface>(std::string("oov"), geom::Point(),
+                                                     stub_buffer_stream,
+                                                     std::shared_ptr<mi::InputChannel>(), [](){});
     }
 };
 }
 
-TEST_F(ShellSurface, creation_and_destruction)
-{
-    using namespace testing;
-
-    msh::SurfaceCreationParameters const params;
-    MockSurfaceBuilder surface_builder;
-
-    InSequence sequence;
-    EXPECT_CALL(surface_builder, create_surface(params)).Times(1);
-    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(1);
-
-    msh::Surface test(
-        mt::fake_shared(surface_builder),
-        params);
-}
-
-TEST_F(ShellSurface, creation_throws_means_no_destroy)
-{
-    using namespace testing;
-
-    msh::SurfaceCreationParameters const params;
-    MockSurfaceBuilder surface_builder;
-
-    InSequence sequence;
-    EXPECT_CALL(surface_builder, create_surface(params)).Times(1)
-        .WillOnce(Throw(std::runtime_error(__PRETTY_FUNCTION__)));
-    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(Exactly(0));
-
-    EXPECT_THROW({
-        msh::Surface test(
-            mt::fake_shared(surface_builder),
-            params);
-    }, std::runtime_error);
-}
-
+//TODO: this should be eliminated with the msh cleanup --kdub
 TEST_F(ShellSurface, destroy)
 {
-    using namespace testing;
-    MockSurfaceBuilder surface_builder;
-
-    InSequence sequence;
-    EXPECT_CALL(surface_builder, create_surface(_)).Times(AnyNumber());
-    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(0);
-
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
-
-    Mock::VerifyAndClearExpectations(&test);
-    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(1);
-
-    // Check destroy_surface is called immediately
+    bool destroy_called = false;
+    {
+        msh::Surface test(weak_surface,[&](std::weak_ptr<ms::Surface>)
+            {
+                destroy_called = true;
+            });
+    }
+    EXPECT_TRUE(destroy_called);
+}
+
+TEST_F(ShellSurface, destroy_manual)
+{
+    bool destroy_called = false;
+    msh::Surface test(weak_surface,[&](std::weak_ptr<ms::Surface>)
+        {
+            destroy_called = true;
+        });
+
     test.destroy();
-
-    Mock::VerifyAndClearExpectations(&test);
-    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(0);
+    EXPECT_TRUE(destroy_called);
 }
 
 TEST_F(ShellSurface, size_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.size();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.size();
@@ -200,15 +104,13 @@
 
 TEST_F(ShellSurface, top_left_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.top_left();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.top_left();
@@ -217,15 +119,13 @@
 
 TEST_F(ShellSurface, name_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.name();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.name();
@@ -234,15 +134,13 @@
 
 TEST_F(ShellSurface, pixel_format_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.pixel_format();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.pixel_format();
@@ -251,15 +149,13 @@
 
 TEST_F(ShellSurface, hide_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.hide();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.hide();
@@ -268,15 +164,13 @@
 
 TEST_F(ShellSurface, show_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.show();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.show();
@@ -285,15 +179,13 @@
 
 TEST_F(ShellSurface, visible_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.visible();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.visible();
@@ -302,15 +194,13 @@
 
 TEST_F(ShellSurface, destroy_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.destroy();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_NO_THROW({
         test.destroy();
@@ -319,15 +209,13 @@
 
 TEST_F(ShellSurface, force_request_to_complete_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.force_requests_to_complete();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_NO_THROW({
         test.force_requests_to_complete();
@@ -336,15 +224,13 @@
 
 TEST_F(ShellSurface, advance_client_buffer_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_NO_THROW({
         test.advance_client_buffer();
     });
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
         test.advance_client_buffer();
@@ -353,11 +239,9 @@
 
 TEST_F(ShellSurface, input_fds_throw_behavior)
 {
-    msh::Surface test(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
-    surface_builder.reset_surface();
+    weak_surface.reset();
 
     EXPECT_THROW({
             test.server_input_fd();
@@ -371,12 +255,10 @@
 {
     using namespace testing;
 
-    msh::Surface surf(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_THROW({
-        surf.configure(static_cast<MirSurfaceAttrib>(111), 222);
+        test.configure(static_cast<MirSurfaceAttrib>(111), 222);
     }, std::logic_error);
 }
 
@@ -384,9 +266,7 @@
 {
     using namespace testing;
 
-    msh::Surface surf(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface surf(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_EQ(mir_surface_type_normal, surf.type());
 
@@ -418,9 +298,7 @@
 {
     using namespace testing;
 
-    msh::Surface surf(
-            mt::fake_shared(surface_builder),
-            msh::a_surface());
+    msh::Surface surf(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     EXPECT_EQ(mir_surface_state_restored, surf.state());
 
@@ -452,9 +330,7 @@
 {
     using namespace ::testing;
 
-    msh::Surface test(
-        mt::fake_shared(surface_builder),
-        msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
     
     mtd::MockInputTargeter targeter;
     EXPECT_CALL(targeter, focus_changed(_)).Times(1);
@@ -466,10 +342,8 @@
 {
     using namespace ::testing;
 
-    msh::Surface test(
-        mt::fake_shared(surface_builder),
-        msh::a_surface());
-    surface_builder.reset_surface();
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
+    weak_surface.reset();
 
     mtd::StubInputTargeter targeter;
     
@@ -480,9 +354,7 @@
 
 TEST_F(ShellSurface, with_most_recent_buffer_do_uses_compositor_buffer)
 {
-    msh::Surface test(
-        mt::fake_shared(surface_builder),
-        msh::a_surface());
+    msh::Surface test(weak_surface,[](std::weak_ptr<ms::Surface>){});
 
     mc::Buffer* buf_ptr{nullptr};
 
@@ -492,6 +364,6 @@
             buf_ptr = &buffer;
         });
 
-    EXPECT_EQ(surface_builder.stub_buffer_stream()->stub_compositor_buffer.get(),
+    EXPECT_EQ(stub_buffer_stream->stub_compositor_buffer.get(),
               buf_ptr);
 }