Merge lp:~alan-griffiths/mir/refactor-shell-cleaner-Surface-class into lp:~mir-team/mir/trunk

[Alan Griffiths]

shell: clean up the dodgy Surface class that resulted from ProxySurface

shell: clean up the dodgy Surface class that resulted from ProxySurface

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:513
http://jenkins.qa.ubuntu.com/job/mir-ci/114/
Executed test runs:

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/114/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:513
http://jenkins.qa.ubuntu.com/job/mir-ci/116/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/117/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/117/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/116/rebuild

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

+1

review: Approve

[Robert Ancell (robert-ancell)]

review: Approve

=== modified file 'include/server/mir/default_server_configuration.h'
--- include/server/mir/default_server_configuration.h	2013-03-15 22:34:53 +0000
+++ include/server/mir/default_server_configuration.h	2013-03-18 15:53:36 +0000
@@ -48,13 +48,15 @@
 {
 class SessionManager;
 class SurfaceFactory;
-class SurfaceController;
+class SurfaceSource;
+class SurfaceBuilder;
 }
 namespace surfaces
 {
 class BufferBundleFactory;
 class SurfaceStackModel;
 class SurfaceStack;
+class SurfaceController;
 }
 namespace graphics
 {
@@ -108,6 +110,8 @@
     virtual std::initializer_list<std::shared_ptr<input::EventFilter> const> the_event_filters();
     virtual std::shared_ptr<input::InputManager> the_input_manager();
 
+    virtual std::shared_ptr<shell::SurfaceBuilder> the_surface_builder();
+
 protected:
     virtual std::shared_ptr<options::Option> the_options() const;
     virtual std::shared_ptr<input::InputChannelFactory> the_input_channel_factory();
@@ -126,10 +130,11 @@
     CachedPtr<graphics::Renderer> renderer;
     CachedPtr<compositor::BufferBundleManager> buffer_bundle_manager;
     CachedPtr<surfaces::SurfaceStack> surface_stack;
-    CachedPtr<shell::SurfaceController> surface_controller;
+    CachedPtr<shell::SurfaceSource> surface_source;
     CachedPtr<compositor::Compositor> compositor;
     CachedPtr<logging::Logger> logger;
     CachedPtr<graphics::DisplayReport> display_report;
+    CachedPtr<surfaces::SurfaceController> surface_controller;
 
 private:
     std::shared_ptr<options::Option> options;

=== added file 'include/server/mir/shell/surface_builder.h'
--- include/server/mir/shell/surface_builder.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/shell/surface_builder.h	2013-03-18 15:53:36 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2013 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 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 Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+
+#ifndef MIR_SHELL_SURFACE_BUILDER_H_
+#define MIR_SHELL_SURFACE_BUILDER_H_
+
+#include <memory>
+
+namespace mir
+{
+namespace frontend { class SurfaceCreationParameters; }
+namespace surfaces { class Surface; }
+
+namespace shell
+{
+class SurfaceBuilder
+{
+public:
+    virtual std::weak_ptr<surfaces::Surface> create_surface(frontend::SurfaceCreationParameters const& params) = 0;
+
+    virtual void destroy_surface(std::weak_ptr<surfaces::Surface> const& surface) = 0;
+protected:
+    SurfaceBuilder() = default;
+    virtual ~SurfaceBuilder() {} // should be "= default;" but that causes "noexcept" spread
+    SurfaceBuilder(SurfaceBuilder const&) = delete;
+    SurfaceBuilder& operator=(SurfaceBuilder const&) = delete;
+};
+}
+}
+
+
+#endif /* MIR_SHELL_SURFACE_BUILDER_H_ */

=== modified file 'include/server/mir/shell/surface_factory.h'
--- include/server/mir/shell/surface_factory.h	2013-03-14 12:48:20 +0000
+++ include/server/mir/shell/surface_factory.h	2013-03-18 15:53:36 +0000
@@ -34,11 +34,10 @@
 class SurfaceFactory
 {
 public:
-    virtual ~SurfaceFactory() {}
-
     virtual std::shared_ptr<frontend::Surface> create_surface(const frontend::SurfaceCreationParameters& params) = 0;
 
 protected:
+    virtual ~SurfaceFactory() {}
     SurfaceFactory() = default;
     SurfaceFactory(const SurfaceFactory&) = delete;
     SurfaceFactory& operator=(const SurfaceFactory&) = delete;

=== renamed file 'include/server/mir/shell/surface_controller.h' => 'include/server/mir/shell/surface_source.h'
--- include/server/mir/shell/surface_controller.h	2013-03-15 22:34:53 +0000
+++ include/server/mir/shell/surface_source.h	2013-03-18 15:53:36 +0000
@@ -16,8 +16,8 @@
  * Authored by: Thomas Voss <[e-mail]>
  */
 
-#ifndef MIR_SHELL_SURFACE_CONTROLLER_H_
-#define MIR_SHELL_SURFACE_CONTROLLER_H_
+#ifndef MIR_SHELL_SURFACE_SOURCE_H_
+#define MIR_SHELL_SURFACE_SOURCE_H_
 
 #include "mir/shell/surface_factory.h"
 
@@ -30,33 +30,28 @@
 class InputChannelFactory;
 }
 
-/// Management of Surface objects. Includes the model (SurfaceStack and Surface
-/// classes) and controller (SurfaceController) elements of an MVC design.
-namespace surfaces
-{
-class SurfaceStackModel;
-}
-
 namespace shell
 {
-class SurfaceController : public SurfaceFactory
+class SurfaceBuilder;
+
+class SurfaceSource : public SurfaceFactory
 {
 public:
-  explicit SurfaceController(std::shared_ptr<surfaces::SurfaceStackModel> const& surface_stack, std::shared_ptr<input::InputChannelFactory> const& input_factory);
-    virtual ~SurfaceController() {}
+    explicit SurfaceSource(std::shared_ptr<SurfaceBuilder> const& surface_builder, std::shared_ptr<input::InputChannelFactory> const& input_factory);
+    virtual ~SurfaceSource() {}
 
     std::shared_ptr<frontend::Surface> create_surface(const frontend::SurfaceCreationParameters& params);
 
 protected:
-    SurfaceController(const SurfaceController&) = delete;
-    SurfaceController& operator=(const SurfaceController&) = delete;
+    SurfaceSource(const SurfaceSource&) = delete;
+    SurfaceSource& operator=(const SurfaceSource&) = delete;
 
 private:
-    std::shared_ptr<surfaces::SurfaceStackModel> const surface_stack;
+    std::shared_ptr<SurfaceBuilder> const surface_builder;
     std::shared_ptr<input::InputChannelFactory> const input_factory;
 };
 
 }
 }
 
-#endif // MIR_SHELL_SURFACE_CONTROLLER_H_
+#endif // MIR_SHELL_SURFACE_SOURCE_H_

=== added file 'include/server/mir/surfaces/surface_controller.h'
--- include/server/mir/surfaces/surface_controller.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/surfaces/surface_controller.h	2013-03-18 15:53:36 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2013 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 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 Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+
+#ifndef MIR_SURFACES_SURFACE_CONTROLLER_H_
+#define MIR_SURFACES_SURFACE_CONTROLLER_H_
+
+#include "mir/shell/surface_builder.h"
+
+namespace mir
+{
+namespace surfaces
+{
+class SurfaceStackModel;
+
+/// Will grow up to provide synchronization of model updates
+class SurfaceController : public shell::SurfaceBuilder
+{
+public:
+    explicit SurfaceController(std::shared_ptr<SurfaceStackModel> const& surface_stack);
+
+    virtual std::weak_ptr<Surface> create_surface(frontend::SurfaceCreationParameters const& params);
+
+    virtual void destroy_surface(std::weak_ptr<Surface> const& surface);
+protected:
+    std::shared_ptr<SurfaceStackModel> const surface_stack;
+};
+
+}
+}
+
+
+#endif /* MIR_SURFACES_SURFACE_CONTROLLER_H_ */

=== modified file 'include/server/mir/surfaces/surface_stack.h'
--- include/server/mir/surfaces/surface_stack.h	2013-03-14 12:48:20 +0000
+++ include/server/mir/surfaces/surface_stack.h	2013-03-18 15:53:36 +0000
@@ -40,6 +40,8 @@
 class SurfaceCreationParameters;
 }
 
+/// Management of Surface objects. Includes the model (SurfaceStack and Surface
+/// classes) and controller (SurfaceController) elements of an MVC design.
 namespace surfaces
 {
 class BufferBundleFactory;
@@ -49,7 +51,7 @@
 {
 public:
     explicit SurfaceStack(std::shared_ptr<BufferBundleFactory> const& bb_factory);
-    virtual ~SurfaceStack() {}
+    virtual ~SurfaceStack() = default;
 
     // From RenderView
     virtual void for_each_if(compositor::FilterForRenderables &filter, compositor::OperatorForRenderables &renderable_operator);

=== modified file 'src/server/default_server_configuration.cpp'
--- src/server/default_server_configuration.cpp	2013-03-15 22:34:53 +0000
+++ src/server/default_server_configuration.cpp	2013-03-18 15:53:36 +0000
@@ -45,8 +45,9 @@
 #include "mir/logging/dumb_console_logger.h"
 #include "mir/logging/session_mediator_report.h"
 #include "mir/logging/display_report.h"
-#include "mir/shell/surface_controller.h"
+#include "mir/shell/surface_source.h"
 #include "mir/surfaces/surface_stack.h"
+#include "mir/surfaces/surface_controller.h"
 
 namespace mc = mir::compositor;
 namespace geom = mir::geometry;
@@ -326,10 +327,20 @@
 std::shared_ptr<msh::SurfaceFactory>
 mir::DefaultServerConfiguration::the_surface_factory()
 {
+    return surface_source(
+        [this]()
+        {
+            return std::make_shared<msh::SurfaceSource>(the_surface_builder(), the_input_channel_factory());
+        });
+}
+
+std::shared_ptr<msh::SurfaceBuilder>
+mir::DefaultServerConfiguration::the_surface_builder()
+{
     return surface_controller(
         [this]()
         {
-            return std::make_shared<msh::SurfaceController>(the_surface_stack_model(), the_input_channel_factory());
+            return std::make_shared<ms::SurfaceController>(the_surface_stack_model());
         });
 }
 

=== modified file 'src/server/shell/CMakeLists.txt'
--- src/server/shell/CMakeLists.txt	2013-03-14 18:09:51 +0000
+++ src/server/shell/CMakeLists.txt	2013-03-18 15:53:36 +0000
@@ -8,7 +8,7 @@
   single_visibility_focus_mechanism.cpp
   consuming_placement_strategy.cpp
   organising_surface_factory.cpp
-  surface_controller.cpp
+  surface_source.cpp
   surface.cpp
 )
 

=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-03-14 17:45:38 +0000
+++ src/server/shell/application_session.cpp	2013-03-18 15:53:36 +0000
@@ -19,7 +19,7 @@
 #include "mir/shell/application_session.h"
 #include "mir/frontend/surface.h"
 
-#include "mir/shell/surface_controller.h"
+#include "mir/shell/surface_factory.h"
 
 #include <boost/throw_exception.hpp>
 
@@ -30,10 +30,9 @@
 
 namespace mf = mir::frontend;
 namespace msh = mir::shell;
-namespace ms = mir::surfaces;
 
 msh::ApplicationSession::ApplicationSession(
-    std::shared_ptr<msh::SurfaceFactory> const& surface_factory,
+    std::shared_ptr<SurfaceFactory> const& surface_factory,
     std::string const& session_name) :
     surface_factory(surface_factory),
     session_name(session_name),

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-03-15 22:34:53 +0000
+++ src/server/shell/surface.cpp	2013-03-18 15:53:36 +0000
@@ -18,7 +18,7 @@
 
 #include "surface.h"
 
-#include "mir/surfaces/surface_stack_model.h"
+#include "mir/shell/surface_builder.h"
 #include "mir/input/input_channel.h"
 
 #include <boost/throw_exception.hpp>
@@ -29,28 +29,22 @@
 namespace mc = mir::compositor;
 namespace mi = mir::input;
 
-msh::Surface::Surface(std::weak_ptr<mir::surfaces::Surface> const& surface,
-		      std::shared_ptr<input::InputChannel> const& input_channel)
-  : surface(surface),
-    input_channel(input_channel),
-    deleter([](std::weak_ptr<mir::surfaces::Surface> const&){})
-{
-}
-
 msh::Surface::Surface(
-    std::weak_ptr<mir::surfaces::Surface> const& surface,
-    std::shared_ptr<input::InputChannel> const& input_channel,
-    std::function<void(std::weak_ptr<mir::surfaces::Surface> const&)> const& deleter)
-:
-    surface(surface),
+    std::shared_ptr<SurfaceBuilder> const& builder,
+    frontend::SurfaceCreationParameters const& params,
+    std::shared_ptr<input::InputChannel> const& input_channel)
+  : builder(builder),
     input_channel(input_channel),
-    deleter(deleter)
+    surface(builder->create_surface(params))
 {
 }
 
 msh::Surface::~Surface()
 {
-    destroy();
+    if (surface.lock())
+    {
+        destroy();
+    }
 }
 
 void msh::Surface::hide()
@@ -71,7 +65,7 @@
 
 void msh::Surface::destroy()
 {
-    deleter(surface);
+    builder->destroy_surface(surface);
 }
 
 void msh::Surface::shutdown()

=== modified file 'src/server/shell/surface.h'
--- src/server/shell/surface.h	2013-03-15 22:34:53 +0000
+++ src/server/shell/surface.h	2013-03-18 15:53:36 +0000
@@ -23,8 +23,6 @@
 #include "mir/frontend/surface.h"
 #include "mir/surfaces/surface.h"
 
-#include <functional>
-
 namespace mir
 {
 namespace frontend
@@ -36,21 +34,17 @@
 class InputChannel;
 }
 
-namespace surfaces
-{
-class SurfaceStackModel;
-}
-
 namespace shell
 {
+class SurfaceBuilder;
 
 class Surface : public frontend::Surface
 {
 public:
-    explicit Surface(std::weak_ptr<mir::surfaces::Surface> const& surface, std::shared_ptr<input::InputChannel> const& input_channel);
-    Surface(std::weak_ptr<mir::surfaces::Surface> const& surface, 
-	    std::shared_ptr<input::InputChannel> const& input_channel,
-	    std::function<void(std::weak_ptr<mir::surfaces::Surface> const&)> const& deleter);
+    Surface(
+        std::shared_ptr<SurfaceBuilder> const& builder,
+        frontend::SurfaceCreationParameters const& params,
+        std::shared_ptr<input::InputChannel> const& input_channel);
     ~Surface();
 
     void hide();
@@ -73,11 +67,10 @@
     int client_input_fd() const;
 
 private:
+    std::shared_ptr<SurfaceBuilder> const builder;
+    std::shared_ptr<mir::input::InputChannel> const input_channel;
     std::weak_ptr<mir::surfaces::Surface> const surface;
-    std::shared_ptr<mir::input::InputChannel> const input_channel;
-    std::function<void(std::weak_ptr<mir::surfaces::Surface> const&)> const deleter;
 };
-
 }
 }
 

=== renamed file 'src/server/shell/surface_controller.cpp' => 'src/server/shell/surface_source.cpp'
--- src/server/shell/surface_controller.cpp	2013-03-15 22:34:53 +0000
+++ src/server/shell/surface_source.cpp	2013-03-18 15:53:36 +0000
@@ -16,8 +16,8 @@
  * Authored by: Thomas Voss <[e-mail]>
  */
 
-#include "mir/shell/surface_controller.h"
-#include "mir/surfaces/surface_stack_model.h"
+#include "mir/shell/surface_source.h"
+#include "mir/shell/surface_builder.h"
 #include "mir/frontend/surface.h"
 #include "mir/input/input_channel_factory.h"
 
@@ -31,19 +31,20 @@
 namespace mf = mir::frontend;
 
 
-msh::SurfaceController::SurfaceController(std::shared_ptr<ms::SurfaceStackModel> const& surface_stack,
-					  std::shared_ptr<mi::InputChannelFactory> const& input_factory)
-  : surface_stack(surface_stack),
+msh::SurfaceSource::SurfaceSource(
+    std::shared_ptr<SurfaceBuilder> const& surface_builder,
+    std::shared_ptr<mi::InputChannelFactory> const& input_factory) :
+    surface_builder(surface_builder),
     input_factory(input_factory)
 {
-    assert(surface_stack);
+    assert(surface_builder);
 }
 
-std::shared_ptr<mf::Surface> msh::SurfaceController::create_surface(const mf::SurfaceCreationParameters& params)
+std::shared_ptr<mf::Surface> msh::SurfaceSource::create_surface(const mf::SurfaceCreationParameters& params)
 {
     return std::make_shared<Surface>(
-        surface_stack->create_surface(params),
-	input_factory->make_input_channel(),
-        [=] (std::weak_ptr<mir::surfaces::Surface> const& surface) { surface_stack->destroy_surface(surface); });
+        surface_builder,
+        params,
+        input_factory->make_input_channel());
 }
 

=== modified file 'src/server/surfaces/CMakeLists.txt'
--- src/server/surfaces/CMakeLists.txt	2013-03-14 17:49:25 +0000
+++ src/server/surfaces/CMakeLists.txt	2013-03-18 15:53:36 +0000
@@ -3,4 +3,5 @@
 
   surface.cpp
   surface_stack.cpp
+  surface_controller.cpp
 )

=== added file 'src/server/surfaces/surface_controller.cpp'
--- src/server/surfaces/surface_controller.cpp	1970-01-01 00:00:00 +0000
+++ src/server/surfaces/surface_controller.cpp	2013-03-18 15:53:36 +0000
@@ -0,0 +1,37 @@
+/*
+ * Copyright © 2013 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 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 Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#include "mir/surfaces/surface_controller.h"
+#include "mir/surfaces/surface_stack_model.h"
+
+namespace ms = mir::surfaces;
+
+ms::SurfaceController::SurfaceController(std::shared_ptr<SurfaceStackModel> const& surface_stack) :
+    surface_stack(surface_stack)
+{
+}
+
+std::weak_ptr<ms::Surface> ms::SurfaceController::create_surface(frontend::SurfaceCreationParameters const& params)
+{
+    return surface_stack->create_surface(params);
+}
+
+void ms::SurfaceController::destroy_surface(std::weak_ptr<Surface> const& surface)
+{
+    surface_stack->destroy_surface(surface);
+}

=== modified file 'tests/integration-tests/frontend/test_session_manager.cpp'
--- tests/integration-tests/frontend/test_session_manager.cpp	2013-03-14 17:45:38 +0000
+++ tests/integration-tests/frontend/test_session_manager.cpp	2013-03-18 15:53:36 +0000
@@ -18,8 +18,6 @@
 
 #include "mir/shell/session_manager.h"
 #include "mir/surfaces/buffer_bundle.h"
-#include "mir/shell/surface_controller.h"
-#include "mir/surfaces/surface_stack.h"
 #include "mir/surfaces/surface.h"
 #include "mir/compositor/buffer_swapper.h"
 #include "mir/shell/focus_sequence.h"

=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
--- tests/unit-tests/shell/test_session_manager.cpp	2013-03-18 03:33:22 +0000
+++ tests/unit-tests/shell/test_session_manager.cpp	2013-03-18 15:53:36 +0000
@@ -29,6 +29,7 @@
 #include "mir_test_doubles/mock_surface_factory.h"
 #include "mir_test_doubles/mock_focus_setter.h"
 #include "mir_test_doubles/null_buffer_bundle.h"
+#include "mir/shell/surface_builder.h"
 
 #include "src/server/shell/surface.h"
 
@@ -46,6 +47,27 @@
 
 namespace
 {
+class StubSurfaceBuilder : public msh::SurfaceBuilder
+{
+public:
+    StubSurfaceBuilder() :
+        buffer_bundle(new mtd::NullBufferBundle()),
+        dummy_surface(std::make_shared<ms::Surface>(mf::a_surface().name, buffer_bundle))
+    {
+    }
+
+    std::weak_ptr<ms::Surface> create_surface(mf::SurfaceCreationParameters const& )
+    {
+        return dummy_surface;
+    }
+
+    void destroy_surface(std::weak_ptr<ms::Surface> const& )
+    {
+    }
+private:
+    std::shared_ptr<ms::BufferBundle> const buffer_bundle;
+    std::shared_ptr<ms::Surface>  dummy_surface;
+};
 
 struct MockSessionContainer : public msh::SessionContainer
 {
@@ -72,6 +94,7 @@
     {
     }
 
+    StubSurfaceBuilder surface_builder;
     mtd::MockSurfaceFactory surface_factory;
     MockSessionContainer container;
     MockFocusSequence sequence;
@@ -102,14 +125,13 @@
     using namespace ::testing;
 
     EXPECT_CALL(surface_factory, create_surface(_)).Times(1);
-    std::shared_ptr<ms::BufferBundle> buffer_bundle(new mtd::NullBufferBundle());
-    std::shared_ptr<ms::Surface> dummy_surface(
-        std::make_shared<ms::Surface>(
-            mf::a_surface().name,
-            buffer_bundle));
+
     std::shared_ptr<mi::InputChannel> null_input_channel;
     ON_CALL(surface_factory, create_surface(_)).WillByDefault(
-       Return(std::make_shared<msh::Surface>(dummy_surface, null_input_channel)));
+       Return(std::make_shared<msh::Surface>(
+           mt::fake_shared(surface_builder),
+           mf::a_surface(),
+           null_input_channel)));
 
     EXPECT_CALL(container, insert_session(_)).Times(1);
     EXPECT_CALL(container, remove_session(_)).Times(1);
@@ -168,18 +190,14 @@
 
 TEST_F(SessionManagerSetup, create_surface_for_session_forwards_and_then_focuses_session)
 {
-    using namespace ::testing;    
-    
-    std::shared_ptr<ms::BufferBundle> buffer_bundle(new mtd::NullBufferBundle());
-    std::shared_ptr<ms::Surface> dummy_surface(
-        std::make_shared<ms::Surface>(
-            mf::a_surface().name,
-            buffer_bundle));
+    using namespace ::testing;
     std::shared_ptr<mi::InputChannel> null_input_channel;
     ON_CALL(surface_factory, create_surface(_)).WillByDefault(
-        Return(std::make_shared<msh::Surface>(dummy_surface, null_input_channel)));
+        Return(std::make_shared<msh::Surface>(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel)));
 
-    
     // Once for session creation and once for surface creation
     {
         InSequence seq;

=== modified file 'tests/unit-tests/shell/test_surface.cpp'
--- tests/unit-tests/shell/test_surface.cpp	2013-03-15 22:34:53 +0000
+++ tests/unit-tests/shell/test_surface.cpp	2013-03-18 15:53:36 +0000
@@ -21,7 +21,9 @@
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/surfaces/surface_stack_model.h"
 #include "mir/input/input_channel.h"
+#include "mir/shell/surface_builder.h"
 
+#include "mir_test_doubles/null_buffer_bundle.h"
 #include "mir_test_doubles/mock_buffer_bundle.h"
 #include "mir_test_doubles/mock_buffer.h"
 #include "mir_test_doubles/stub_buffer.h"
@@ -42,43 +44,48 @@
 
 namespace
 {
-typedef testing::NiceMock<mtd::MockBufferBundle> StubBufferBundle;
-
-class MockSurfaceStackModel : public ms::SurfaceStackModel
+class MockSurfaceBuilder : public msh::SurfaceBuilder
 {
 public:
-    MockSurfaceStackModel()
+    MockSurfaceBuilder() :
+        buffer_bundle(new mtd::NullBufferBundle()),
+        dummy_surface()
     {
         using namespace testing;
-
         ON_CALL(*this, create_surface(_)).
-            WillByDefault(Invoke(this, &MockSurfaceStackModel::do_create_surface));
+            WillByDefault(Invoke(this, &MockSurfaceBuilder::do_create_surface));
 
         ON_CALL(*this, destroy_surface(_)).
-            WillByDefault(Invoke(this, &MockSurfaceStackModel::do_destroy_surface));
+            WillByDefault(Invoke(this, &MockSurfaceBuilder::do_destroy_surface));
     }
 
     MOCK_METHOD1(create_surface, std::weak_ptr<ms::Surface> (const mf::SurfaceCreationParameters&));
 
     MOCK_METHOD1(destroy_surface, void (std::weak_ptr<ms::Surface> const&));
 
+    std::weak_ptr<ms::Surface> do_create_surface(mf::SurfaceCreationParameters const& )
+    {
+        dummy_surface = std::make_shared<ms::Surface>(mf::a_surface().name, buffer_bundle);
+        return dummy_surface;
+    }
+
+    void do_destroy_surface(std::weak_ptr<ms::Surface> const& )
+    {
+        reset_surface();
+    }
+
+    void reset_surface()
+    {
+        dummy_surface.reset();
+    }
+
 private:
-    std::weak_ptr<ms::Surface> do_create_surface(const mf::SurfaceCreationParameters& params)
-    {
-        surface = std::make_shared<ms::Surface>(
-            params.name,
-            std::make_shared<StubBufferBundle>());
-        return surface;
-    }
-
-    void do_destroy_surface(std::weak_ptr<ms::Surface> const&)
-    {
-        surface.reset();
-    }
-
-    std::shared_ptr<ms::Surface> surface;
+    std::shared_ptr<ms::BufferBundle> const buffer_bundle;
+    std::shared_ptr<ms::Surface> dummy_surface;
 };
 
+typedef testing::NiceMock<mtd::MockBufferBundle> StubBufferBundle;
+
 
 struct MockInputChannel : public mi::InputChannel
 {
@@ -90,6 +97,7 @@
 {
     std::shared_ptr<StubBufferBundle> buffer_bundle;
     std::shared_ptr<mi::InputChannel> null_input_channel;
+    MockSurfaceBuilder surface_builder;
 
     ShellSurface() :
         buffer_bundle(std::make_shared<StubBufferBundle>()),
@@ -108,171 +116,173 @@
 {
     using namespace testing;
 
-    MockSurfaceStackModel surface_stack;
     mf::SurfaceCreationParameters params;
 
     InSequence sequence;
-    EXPECT_CALL(surface_stack, create_surface(_)).Times(1);
-    EXPECT_CALL(surface_stack, destroy_surface(_)).Times(1);
+    EXPECT_CALL(surface_builder, create_surface(_)).Times(1);
+    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(1);
 
     msh::Surface test(
-        surface_stack.create_surface(params),
-	null_input_channel,
-        [&](std::weak_ptr<mir::surfaces::Surface> const& s) {surface_stack.destroy_surface(s);});
+        mt::fake_shared(surface_builder),
+        params,
+        null_input_channel);
 }
 
 TEST_F(ShellSurface, destroy)
 {
     using namespace testing;
 
-    NiceMock<MockSurfaceStackModel> surface_stack;
-    mf::SurfaceCreationParameters params;
-
     msh::Surface test(
-        surface_stack.create_surface(params),
-	null_input_channel,
-        [&](std::weak_ptr<mir::surfaces::Surface> const& s) {surface_stack.destroy_surface(s);});
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
-    EXPECT_CALL(surface_stack, destroy_surface(_)).Times(1);
+    EXPECT_CALL(surface_builder, destroy_surface(_)).Times(1);
 
     test.destroy();
-
-    Mock::VerifyAndClearExpectations(&surface_stack);
 }
 
 TEST_F(ShellSurface, client_buffer_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.client_buffer();
+        test.client_buffer();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_THROW({
-        proxy_surface.client_buffer();
+        test.client_buffer();
     }, std::runtime_error);
 }
 
 TEST_F(ShellSurface, size_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.size();
+        test.size();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_THROW({
-        proxy_surface.size();
+        test.size();
     }, std::runtime_error);
 }
 
 TEST_F(ShellSurface, pixel_format_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.pixel_format();
+        test.pixel_format();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_THROW({
-        proxy_surface.pixel_format();
+        test.pixel_format();
     }, std::runtime_error);
 }
 
 TEST_F(ShellSurface, hide_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.hide();
+        test.hide();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_NO_THROW({
-        proxy_surface.hide();
+        test.hide();
     });
 }
 
 TEST_F(ShellSurface, show_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.show();
+        test.show();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_NO_THROW({
-        proxy_surface.show();
+        test.show();
     });
 }
 
 TEST_F(ShellSurface, destroy_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.destroy();
+        test.destroy();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_NO_THROW({
-        proxy_surface.destroy();
+        test.destroy();
     });
 }
 
 TEST_F(ShellSurface, shutdown_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.shutdown();
+        test.shutdown();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_NO_THROW({
-        proxy_surface.shutdown();
+        test.shutdown();
     });
 }
 
 TEST_F(ShellSurface, advance_client_buffer_throw_behavior)
 {
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-
-    msh::Surface proxy_surface(surface, null_input_channel);
+    msh::Surface test(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
 
     EXPECT_NO_THROW({
-        proxy_surface.advance_client_buffer();
+        test.advance_client_buffer();
     });
 
-    surface.reset();
+    surface_builder.reset_surface();
 
     EXPECT_NO_THROW({
-        proxy_surface.advance_client_buffer();
+        test.advance_client_buffer();
     });
 }
 
@@ -282,13 +292,11 @@
     const int testing_client_fd = 17;
 
     MockInputChannel mock_package;
-    auto surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-    auto input_surface = std::make_shared<ms::Surface>(__PRETTY_FUNCTION__, buffer_bundle);
-    msh::Surface proxy_surface(surface, null_input_channel);
-    msh::Surface input_proxy_surface(surface, mt::fake_shared(mock_package));
+    msh::Surface proxy_surface(mt::fake_shared(surface_builder), mf::a_surface(), null_input_channel);
+    msh::Surface input_proxy_surface(mt::fake_shared(surface_builder), mf::a_surface(), mt::fake_shared(mock_package));
 
     EXPECT_CALL(mock_package, client_fd()).Times(1).WillOnce(Return(testing_client_fd));
-    
+
     EXPECT_TRUE(input_proxy_surface.supports_input());
     EXPECT_FALSE(proxy_surface.supports_input());
 

=== modified file 'tests/unit-tests/surfaces/CMakeLists.txt'
--- tests/unit-tests/surfaces/CMakeLists.txt	2013-03-15 22:34:53 +0000
+++ tests/unit-tests/surfaces/CMakeLists.txt	2013-03-18 15:53:36 +0000
@@ -2,6 +2,7 @@
   APPEND UNIT_TEST_SOURCES
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_surface_stack.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_surface_controller.cpp
 )
 
 set(UNIT_TEST_SOURCES ${UNIT_TEST_SOURCES} PARENT_SCOPE)

=== modified file 'tests/unit-tests/surfaces/test_surface_controller.cpp'
--- tests/unit-tests/surfaces/test_surface_controller.cpp	2013-03-11 19:26:34 +0000
+++ tests/unit-tests/surfaces/test_surface_controller.cpp	2013-03-18 15:53:36 +0000
@@ -19,58 +19,38 @@
 #include "mir/surfaces/surface_controller.h"
 #include "mir/surfaces/surface_stack_model.h"
 #include "mir/shell/surface_creation_parameters.h"
-#include "mir/input/input_channel.h"
-#include "mir/input/input_channel_factory.h"
 
 #include "mir_test/fake_shared.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
+namespace mf = mir::frontend;
 namespace ms = mir::surfaces;
-namespace msh = mir::shell;
-namespace mi = mir::input;
 namespace mt = mir::test;
 
 namespace
 {
-
 struct MockSurfaceStackModel : public ms::SurfaceStackModel
 {
-    MOCK_METHOD1(create_surface, std::weak_ptr<ms::Surface>(msh::SurfaceCreationParameters const&));
+    MOCK_METHOD1(create_surface, std::weak_ptr<ms::Surface>(mf::SurfaceCreationParameters const&));
     MOCK_METHOD1(destroy_surface, void(std::weak_ptr<ms::Surface> const&));
 };
-
-struct MockInputChannel : public mi::InputChannel
-{
-    MOCK_CONST_METHOD0(client_fd, int());
-    MOCK_CONST_METHOD0(server_fd, int());
-};
-
-struct MockInputChannelFactory : public mi::InputChannelFactory
-{
-    MOCK_METHOD0(make_input_channel, std::shared_ptr<mi::InputChannel>());
-};
-
 }
 
-TEST(SurfaceStack, create_surface)
+TEST(SurfaceController, create_and_destroy_surface)
 {
     using namespace ::testing;
 
     std::weak_ptr<ms::Surface> null_surface;
     MockSurfaceStackModel model;
-    MockInputChannelFactory input_factory;
-    MockInputChannel package;
-    
-    EXPECT_CALL(model, create_surface(_)).Times(1)
-        .WillOnce(Return(null_surface));
-    EXPECT_CALL(input_factory, make_input_channel()).Times(1)
-        .WillOnce(Return(mt::fake_shared(package)));
+
+    ms::SurfaceController controller(mt::fake_shared(model));
+
+    InSequence seq;
+    EXPECT_CALL(model, create_surface(_)).Times(1).WillOnce(Return(null_surface));
     EXPECT_CALL(model, destroy_surface(_)).Times(1);
-        
-    ms::SurfaceController controller(mt::fake_shared(model), mt::fake_shared(input_factory));
-    {
-        auto surface = controller.create_surface(msh::a_surface());
-    }
+
+    auto surface = controller.create_surface(mf::a_surface());
+    controller.destroy_surface(surface);
 }