Merge lp:~alan-griffiths/mir/introducing-SurfaceObserver into lp:mir

[Alan Griffiths]

scene: Simplify BasicSurface creation by separating out an initial cut of SurfaceObserver.

scene: Simplify BasicSurface creation by separating out an initial cut of SurfaceObserver.

In this iteration SurfaceObserver is only used to support the MirEvents needed to feed the frontend EventSink. There are other "Observer" requirements that will be rolled into this hierarchy.

[Andreas Pokorny (andreas-pokorny)]

Approve a part from:

the name SurfaceEventSource is a bit misleading... on second read: was a but misleading. Now I am only curious whether we need it inside the public headers. Would someone replacing the scene or extending the surfaces need that implementation? Or would decorating the surface factory be sufficient?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1523
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1201/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1419
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1417
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/998
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/933
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/933/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/938
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/938/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/999
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/999/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/928
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5426

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1201/rebuild

review: Approve (continuous-integration)

[Gerry Boland (gerboland)]

I like this design a lot, +1 from me

review: Approve (api)

[Alan Griffiths (alan-griffiths)]

> Approve a part from:
>
> the name SurfaceEventSource is a bit misleading... on second read: was a but
> misleading. Now I am only curious whether we need it inside the public
> headers. Would someone replacing the scene or extending the surfaces need that
> implementation? Or would decorating the surface factory be sufficient?

I thought about it. Making it public ensures that it is easy for users that need to synthesize surface events (rather requiring duplication of our code for populating the MirEvent structure).

[Alexandros Frantzis (afrantzis)]

Looks good.

> Making it public ensures that it is easy for users that need to synthesize surface events

I am OK with this, but I guess we could wait until it is actually needed before we promote it to public (unless you have a specific use case in mind for the near future).

review: Approve

[Daniel van Vugt (vanvugt)]

I know Observer is a common pattern but that doesn't make it the best solution. Maybe think for a while if there's a better name/design that's less abstract than being based on a verb.

[Alan Griffiths (alan-griffiths)]

> I know Observer is a common pattern but that doesn't make it the best
> solution. Maybe think for a while if there's a better name/design that's less
> abstract than being based on a verb.

This design has been under discussion since the end of last year.

The name has been chosen intentionally:

/1/ it matches the discussions about architecture improvements that has been taking place over the last three weeks;
/2/ and, "Observer" is widely understood from the GOF patterns catalog.

I think the fact that our most active user says "I like this design a lot" is more important than any concern you have that there has been too little thought go into it.

[Andreas Pokorny (andreas-pokorny)]

review: Approve

=== modified file 'examples/render_surfaces.cpp'
--- examples/render_surfaces.cpp	2014-03-31 11:00:33 +0000
+++ examples/render_surfaces.cpp	2014-03-31 11:00:33 +0000
@@ -466,7 +466,6 @@
         for (auto& m : moveables)
         {
             auto const s = surface_coordinator->add_surface(
-                    mf::SurfaceId(),
                     msh::a_surface().of_size(surface_size)
                                    .of_pixel_format(surface_pf)
                                    .of_buffer_usage(mg::BufferUsage::hardware),

=== modified file 'include/server/mir/scene/surface.h'
--- include/server/mir/scene/surface.h	2014-03-25 10:21:00 +0000
+++ include/server/mir/scene/surface.h	2014-03-31 11:00:33 +0000
@@ -29,6 +29,8 @@
 
 namespace scene
 {
+class SurfaceObserver;
+
 class Surface :
     public graphics::Renderable,
     public input::Surface,
@@ -47,6 +49,9 @@
     // TODO cleanest interface for this.
     virtual std::shared_ptr<input::InputChannel> input_channel() const = 0;
     virtual void on_change(std::function<void()> change_notification) = 0;
+
+    virtual void add_observer(std::shared_ptr<SurfaceObserver> const& observer) = 0;
+    virtual void remove_observer(std::shared_ptr<SurfaceObserver> const& observer) = 0;
 };
 }
 }

=== modified file 'include/server/mir/scene/surface_coordinator.h'
--- include/server/mir/scene/surface_coordinator.h	2014-03-31 11:00:33 +0000
+++ include/server/mir/scene/surface_coordinator.h	2014-03-31 11:00:33 +0000
@@ -21,13 +21,11 @@
 #define MIR_SCENE_SURFACE_COORDINATOR_H_
 
 #include "mir/scene/surface_ranker.h"
-#include "mir/frontend/surface_id.h"
 
 #include <memory>
 
 namespace mir
 {
-namespace frontend { class EventSink; }
 namespace shell
 {
 struct SurfaceCreationParameters;
@@ -36,14 +34,14 @@
 namespace scene
 {
 class Surface;
+class SurfaceObserver;
 
 class SurfaceCoordinator : public SurfaceRanker
 {
 public:
     virtual std::shared_ptr<Surface> add_surface(
-        frontend::SurfaceId id,
         shell::SurfaceCreationParameters const& params,
-        std::shared_ptr<frontend::EventSink> const& event_sink) = 0;
+        std::shared_ptr<SurfaceObserver> const& observer) = 0;
 
     virtual void remove_surface(std::weak_ptr<Surface> const& surface) = 0;
 protected:

=== added file 'include/server/mir/scene/surface_event_source.h'
--- include/server/mir/scene/surface_event_source.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/scene/surface_event_source.h	2014-03-31 11:00:33 +0000
@@ -0,0 +1,49 @@
+/*
+ * Copyright © 2014 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/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#ifndef MIR_SCENE_SURFACE_EVENT_SOURCE_H_
+#define MIR_SCENE_SURFACE_EVENT_SOURCE_H_
+
+#include "mir/scene/surface_observer.h"
+#include "mir/frontend/surface_id.h"
+#include "mir/frontend/event_sink.h"
+
+#include <memory>
+
+namespace mir
+{
+namespace scene
+{
+class SurfaceEventSource : public SurfaceObserver
+{
+public:
+    SurfaceEventSource(
+        frontend::SurfaceId id,
+        std::shared_ptr<frontend::EventSink> const& event_sink);
+
+    void attrib_change(MirSurfaceAttrib attrib, int value);
+    void resize(geometry::Size const& size);
+
+private:
+    frontend::SurfaceId const id;
+    std::shared_ptr<frontend::EventSink> const event_sink;
+};
+}
+}
+
+#endif // MIR_SCENE_SURFACE_EVENT_SOURCE_H_

=== modified file 'include/server/mir/scene/surface_factory.h'
--- include/server/mir/scene/surface_factory.h	2014-03-31 11:00:33 +0000
+++ include/server/mir/scene/surface_factory.h	2014-03-31 11:00:33 +0000
@@ -19,14 +19,11 @@
 #ifndef MIR_SCENE_SURFACE_FACTORY_H_
 #define MIR_SCENE_SURFACE_FACTORY_H_
 
-#include "mir/frontend/surface_id.h"
 #include "mir/shell/surface_creation_parameters.h"
 #include <memory>
-#include <functional>
 
 namespace mir
 {
-namespace frontend { class EventSink; }
 namespace scene
 {
 class Surface;
@@ -38,9 +35,8 @@
     virtual ~SurfaceFactory() = default;
 
     virtual std::shared_ptr<Surface> create_surface(
-        frontend::SurfaceId id,
-        shell::SurfaceCreationParameters const& params,
-        std::shared_ptr<frontend::EventSink> const& event_sink) = 0;
+        shell::SurfaceCreationParameters const& params) = 0;
+
 private:
     SurfaceFactory(const SurfaceFactory&) = delete;
     SurfaceFactory& operator=(const SurfaceFactory&) = delete;

=== added file 'include/server/mir/scene/surface_observer.h'
--- include/server/mir/scene/surface_observer.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/scene/surface_observer.h	2014-03-31 11:00:33 +0000
@@ -0,0 +1,45 @@
+/*
+ * Copyright © 2014 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/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#ifndef MIR_SCENE_SURFACE_OBSERVER_H_
+#define MIR_SCENE_SURFACE_OBSERVER_H_
+
+#include "mir/geometry/size.h"
+#include "mir_toolkit/common.h"
+
+namespace mir
+{
+namespace scene
+{
+// Initial cut - supporting the frontend requirement, more will follow
+class SurfaceObserver
+{
+public:
+    virtual void attrib_change(MirSurfaceAttrib attrib, int value) = 0;
+    virtual void resize(geometry::Size const& size) = 0;
+
+protected:
+    SurfaceObserver() = default;
+    virtual ~SurfaceObserver() = default;
+    SurfaceObserver(SurfaceObserver const&) = delete;
+    SurfaceObserver& operator=(SurfaceObserver const&) = delete;
+};
+}
+}
+
+#endif // MIR_SCENE_SURFACE_OBSERVER_H_

=== modified file 'include/server/mir/shell/surface_factory.h'
--- include/server/mir/shell/surface_factory.h	2014-03-25 17:04:27 +0000
+++ include/server/mir/shell/surface_factory.h	2014-03-31 11:00:33 +0000
@@ -19,15 +19,12 @@
 #ifndef MIR_SHELL_SURFACE_FACTORY_H_
 #define MIR_SHELL_SURFACE_FACTORY_H_
 
-#include "mir/frontend/surface_id.h"
 #include <memory>
 
 namespace mir
 {
-namespace frontend
-{
-class EventSink;
-}
+namespace scene { class SurfaceObserver; }
+
 namespace shell
 {
 class Session;
@@ -40,8 +37,7 @@
     virtual std::shared_ptr<Surface> create_surface(
         Session* session,
         SurfaceCreationParameters const& params,
-        frontend::SurfaceId id,
-        std::shared_ptr<frontend::EventSink> const& sink) = 0;
+        std::shared_ptr<scene::SurfaceObserver> const& observer) = 0;
 
     virtual void destroy_surface(std::shared_ptr<Surface> const& surface) = 0;
 

=== modified file 'include/test/mir_test_doubles/mock_surface.h'
--- include/test/mir_test_doubles/mock_surface.h	2014-03-26 14:20:14 +0000
+++ include/test/mir_test_doubles/mock_surface.h	2014-03-31 11:00:33 +0000
@@ -35,14 +35,12 @@
 {
     MockSurface() :
         scene::BasicSurface(
-            frontend::SurfaceId{},
             {},
             {{},{}},
             true,
             {},
             {},
             {},
-            {},
             mir::report::null_scene_report())
     {
     }
@@ -65,6 +63,7 @@
 
     MOCK_METHOD2(configure, int(MirSurfaceAttrib, int));
     MOCK_METHOD1(take_input_focus, void(std::shared_ptr<shell::InputTargeter> const&));
+    MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::SurfaceObserver> const&));
 };
 
 }

=== modified file 'include/test/mir_test_doubles/mock_surface_factory.h'
--- include/test/mir_test_doubles/mock_surface_factory.h	2014-03-24 17:30:12 +0000
+++ include/test/mir_test_doubles/mock_surface_factory.h	2014-03-31 11:00:33 +0000
@@ -33,11 +33,10 @@
 
 struct MockSurfaceFactory : public shell::SurfaceFactory
 {
-    MOCK_METHOD4(create_surface, std::shared_ptr<shell::Surface>(
+    MOCK_METHOD3(create_surface, std::shared_ptr<shell::Surface>(
         shell::Session*,
         const shell::SurfaceCreationParameters&,
-        frontend::SurfaceId,
-        std::shared_ptr<frontend::EventSink> const&));
+        std::shared_ptr<scene::SurfaceObserver> const&));
 
     void destroy_surface(std::shared_ptr<shell::Surface> const& /*surface*/) override {}
 };

=== modified file 'src/server/scene/CMakeLists.txt'
--- src/server/scene/CMakeLists.txt	2014-03-31 11:00:33 +0000
+++ src/server/scene/CMakeLists.txt	2014-03-31 11:00:33 +0000
@@ -13,5 +13,6 @@
   surface_allocator.cpp
   surface_stack.cpp
   surface_controller.cpp
+  surface_event_source.cpp
   threaded_snapshot_strategy.cpp
 )

=== modified file 'src/server/scene/application_session.cpp'
--- src/server/scene/application_session.cpp	2014-03-26 13:02:43 +0000
+++ src/server/scene/application_session.cpp	2014-03-31 11:00:33 +0000
@@ -18,6 +18,7 @@
 
 #include "application_session.h"
 #include "mir/shell/surface.h"
+#include "mir/scene/surface_event_source.h"
 #include "mir/shell/surface_factory.h"
 #include "snapshot_strategy.h"
 #include "mir/shell/session_listener.h"
@@ -71,7 +72,9 @@
 mf::SurfaceId ms::ApplicationSession::create_surface(const msh::SurfaceCreationParameters& params)
 {
     auto const id = next_id();
-    auto surf = surface_factory->create_surface(this, params, id, event_sink);
+
+    auto const observer = std::make_shared<scene::SurfaceEventSource>(id, event_sink);
+    auto surf = surface_factory->create_surface(this, params, observer);
 
     std::unique_lock<std::mutex> lock(surfaces_mutex);
     surfaces[id] = surf;

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2014-03-26 14:20:14 +0000
+++ src/server/scene/basic_surface.cpp	2014-03-31 11:00:33 +0000
@@ -31,7 +31,7 @@
 #include <boost/throw_exception.hpp>
 
 #include <stdexcept>
-#include <cstring>
+#include <algorithm>
 
 namespace mc = mir::compositor;
 namespace ms = mir::scene;
@@ -58,17 +58,45 @@
     notifier();
 }
 
+void ms::SurfaceObservers::attrib_change(MirSurfaceAttrib attrib, int value)
+{
+    std::unique_lock<decltype(mutex)> lock(mutex);
+    // TBD Maybe we should copy observers so we can release the lock?
+    for (auto const& p : observers)
+        p->attrib_change(attrib, value);
+}
+
+void ms::SurfaceObservers::resize(geometry::Size const& size)
+{
+    std::unique_lock<decltype(mutex)> lock(mutex);
+    // TBD Maybe we should copy observers so we can release the lock?
+    for (auto const& p : observers)
+        p->resize(size);
+}
+
+void ms::SurfaceObservers::add(std::shared_ptr<SurfaceObserver> const& observer)
+{
+    if (observer)
+    {
+        std::unique_lock<decltype(mutex)> lock(mutex);
+        observers.push_back(observer);
+    }
+}
+
+void ms::SurfaceObservers::remove(std::shared_ptr<SurfaceObserver> const& observer)
+{
+    std::unique_lock<decltype(mutex)> lock(mutex);
+    observers.erase(std::remove(observers.begin(),observers.end(), observer), observers.end());
+}
+
 ms::BasicSurface::BasicSurface(
-    frontend::SurfaceId id,
     std::string const& name,
     geometry::Rectangle rect,
     bool nonrectangular,
-    std::shared_ptr<mc::BufferStream> const& buffer_stream,
+    std::shared_ptr<compositor::BufferStream> const& buffer_stream,
     std::shared_ptr<input::InputChannel> const& input_channel,
-    std::shared_ptr<frontend::EventSink> const& event_sink,
     std::shared_ptr<SurfaceConfigurator> const& configurator,
     std::shared_ptr<SceneReport> const& report) :
-    id(id),
     notify_change([](){}),
     surface_name(name),
     surface_rect(rect),
@@ -79,7 +107,6 @@
     input_rectangles{surface_rect},
     surface_buffer_stream(buffer_stream),
     server_input_channel(input_channel),
-    event_sink(event_sink),
     configurator(configurator),
     report(report),
     type_value(mir_surface_type_normal),
@@ -222,13 +249,7 @@
         surface_rect.size = size;
     }
     notify_change();
-    MirEvent e;
-    memset(&e, 0, sizeof e);
-    e.type = mir_event_type_resize;
-    e.resize.surface_id = id.as_value();
-    e.resize.width = size.width.as_int();
-    e.resize.height = size.height.as_int();
-    event_sink->handle_event(e);
+    observers.resize(size);
 }
 
 geom::Point ms::BasicSurface::top_left() const
@@ -363,29 +384,12 @@
         state_value = s;
         valid = true;
 
-        notify_attrib_change(mir_surface_attrib_state, s);
+        observers.attrib_change(mir_surface_attrib_state, s);
     }
 
     return valid;
 }
 
-void ms::BasicSurface::notify_attrib_change(MirSurfaceAttrib attrib, int value)
-{
-    MirEvent e;
-
-    // This memset is not really required. However it does avoid some
-    // harmless uninitialized memory reads that valgrind will complain
-    // about, due to gaps in MirEvent.
-    memset(&e, 0, sizeof e);
-
-    e.type = mir_event_type_surface;
-    e.surface.id = id.as_value();
-    e.surface.attrib = attrib;
-    e.surface.value = value;
-
-    event_sink->handle_event(e);
-}
-
 void ms::BasicSurface::take_input_focus(std::shared_ptr<msh::InputTargeter> const& targeter)
 {
     targeter->focus_changed(input_channel());
@@ -415,7 +419,7 @@
         result = state();
         break;
     case mir_surface_attrib_focus:
-        notify_attrib_change(attrib, value);
+        observers.attrib_change(attrib, value);
         break;
     case mir_surface_attrib_swapinterval:
         allow_dropping = (value == 0);
@@ -443,3 +447,13 @@
     set_hidden(false);
 }
 
+
+void ms::BasicSurface::add_observer(std::shared_ptr<SurfaceObserver> const& observer)
+{
+    observers.add(observer);
+}
+
+void ms::BasicSurface::remove_observer(std::shared_ptr<SurfaceObserver> const& observer)
+{
+    observers.remove(observer);
+}

=== modified file 'src/server/scene/basic_surface.h'
--- src/server/scene/basic_surface.h	2014-03-26 14:20:14 +0000
+++ src/server/scene/basic_surface.h	2014-03-31 11:00:33 +0000
@@ -20,8 +20,8 @@
 #define MIR_SCENE_BASIC_SURFACE_H_
 
 #include "mir/scene/surface.h"
+#include "mir/scene/surface_observer.h"
 
-#include "mir/frontend/surface_id.h"
 #include "mir/geometry/rectangle.h"
 
 #include "mir_toolkit/common.h"
@@ -71,17 +71,30 @@
     std::function<void()> notify_change;
 };
 
+class SurfaceObservers : public SurfaceObserver
+{
+public:
+
+    void attrib_change(MirSurfaceAttrib attrib, int value) override;
+    void resize(geometry::Size const& size) override;
+
+    void add(std::shared_ptr<SurfaceObserver> const& observer);
+    void remove(std::shared_ptr<SurfaceObserver> const& observer);
+
+private:
+    std::mutex mutex;
+    std::vector<std::shared_ptr<SurfaceObserver>> observers;
+};
+
 class BasicSurface : public Surface
 {
 public:
     BasicSurface(
-        frontend::SurfaceId id,
         std::string const& name,
         geometry::Rectangle rect,
         bool nonrectangular,
         std::shared_ptr<compositor::BufferStream> const& buffer_stream,
         std::shared_ptr<input::InputChannel> const& input_channel,
-        std::shared_ptr<frontend::EventSink> const& event_sink,
         std::shared_ptr<SurfaceConfigurator> const& configurator,
         std::shared_ptr<SceneReport> const& report);
 
@@ -136,13 +149,15 @@
     void hide() override;
     void show() override;
 
+    void add_observer(std::shared_ptr<SurfaceObserver> const& observer) override;
+    void remove_observer(std::shared_ptr<SurfaceObserver> const& observer) override;
+
 private:
     bool set_type(MirSurfaceType t);  // Use configure() to make public changes
     bool set_state(MirSurfaceState s);
-    void notify_attrib_change(MirSurfaceAttrib attrib, int value);
 
+    SurfaceObservers observers;
     std::mutex mutable guard;
-    frontend::SurfaceId const id;
     ThreadsafeCallback notify_change;
     std::string const surface_name;
     geometry::Rectangle surface_rect;
@@ -154,7 +169,6 @@
     std::vector<geometry::Rectangle> input_rectangles;
     std::shared_ptr<compositor::BufferStream> const surface_buffer_stream;
     std::shared_ptr<input::InputChannel> const server_input_channel;
-    std::shared_ptr<frontend::EventSink> const event_sink;
     std::shared_ptr<SurfaceConfigurator> const configurator;
     std::shared_ptr<SceneReport> const report;
 

=== modified file 'src/server/scene/surface_allocator.cpp'
--- src/server/scene/surface_allocator.cpp	2014-03-26 14:20:14 +0000
+++ src/server/scene/surface_allocator.cpp	2014-03-31 11:00:33 +0000
@@ -48,9 +48,7 @@
 }
 
 std::shared_ptr<ms::Surface> ms::SurfaceAllocator::create_surface(
-    frontend::SurfaceId id,
-    msh::SurfaceCreationParameters const& params,
-    std::shared_ptr<frontend::EventSink> const& event_sink)
+    shell::SurfaceCreationParameters const& params)
 {
     mg::BufferProperties buffer_properties{params.size,
                                            params.pixel_format,
@@ -60,14 +58,14 @@
 
     bool nonrectangular = has_alpha(params.pixel_format);
     auto input_channel = input_factory->make_input_channel();
-    return std::make_shared<BasicSurface>(
-        id,
+    auto const surface = std::make_shared<BasicSurface>(
         params.name,
         actual_size,
         nonrectangular,
         buffer_stream,
         input_channel,
-        event_sink,
         configurator,
         report);
+
+    return surface;
 }

=== modified file 'src/server/scene/surface_allocator.h'
--- src/server/scene/surface_allocator.h	2014-03-31 11:00:33 +0000
+++ src/server/scene/surface_allocator.h	2014-03-31 11:00:33 +0000
@@ -42,9 +42,7 @@
                      std::shared_ptr<SceneReport> const& report);
 
     std::shared_ptr<Surface> create_surface(
-        frontend::SurfaceId id,
-        shell::SurfaceCreationParameters const& params,
-        std::shared_ptr<frontend::EventSink> const& event_sink) override;
+        shell::SurfaceCreationParameters const& params) override;
 
 private:
     std::shared_ptr<BufferStreamFactory> const buffer_stream_factory;

=== modified file 'src/server/scene/surface_controller.cpp'
--- src/server/scene/surface_controller.cpp	2014-03-31 11:00:33 +0000
+++ src/server/scene/surface_controller.cpp	2014-03-31 11:00:33 +0000
@@ -19,6 +19,7 @@
 #include "surface_controller.h"
 #include "surface_stack_model.h"
 #include "mir/scene/surface_factory.h"
+#include "mir/scene/surface.h"
 
 namespace ms = mir::scene;
 namespace msh = mir::shell;
@@ -32,11 +33,11 @@
 }
 
 std::shared_ptr<ms::Surface> ms::SurfaceController::add_surface(
-    frontend::SurfaceId id,
     shell::SurfaceCreationParameters const& params,
-    std::shared_ptr<frontend::EventSink> const& event_sink)
+    std::shared_ptr<SurfaceObserver> const& observer)
 {
-    auto const surface = surface_factory->create_surface(id, params, event_sink);
+    auto const surface = surface_factory->create_surface(params);
+    surface->add_observer(observer);
     surface_stack->add_surface(surface, params.depth, params.input_mode);
     return surface;
 }

=== modified file 'src/server/scene/surface_controller.h'
--- src/server/scene/surface_controller.h	2014-03-31 11:00:33 +0000
+++ src/server/scene/surface_controller.h	2014-03-31 11:00:33 +0000
@@ -24,11 +24,6 @@
 
 namespace mir
 {
-namespace shell
-{
-class Session;
-}
-
 namespace scene
 {
 class SurfaceStackModel;
@@ -43,9 +38,8 @@
         std::shared_ptr<SurfaceStackModel> const& surface_stack);
 
     std::shared_ptr<Surface> add_surface(
-        frontend::SurfaceId id,
         shell::SurfaceCreationParameters const& params,
-        std::shared_ptr<frontend::EventSink> const& event_sink) override;
+        std::shared_ptr<SurfaceObserver> const& observer) override;
 
     void remove_surface(std::weak_ptr<Surface> const& surface) override;
 

=== added file 'src/server/scene/surface_event_source.cpp'
--- src/server/scene/surface_event_source.cpp	1970-01-01 00:00:00 +0000
+++ src/server/scene/surface_event_source.cpp	2014-03-31 11:00:33 +0000
@@ -0,0 +1,62 @@
+/*
+ * Copyright © 2014 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/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#include "mir/scene/surface_event_source.h"
+
+#include <cstring>
+#include <algorithm>
+
+namespace ms = mir::scene;
+namespace geom = mir::geometry;
+
+ms::SurfaceEventSource::SurfaceEventSource(
+    frontend::SurfaceId id,
+    std::shared_ptr<frontend::EventSink> const& event_sink) :
+    id(id),
+    event_sink(event_sink)
+{
+}
+
+void ms::SurfaceEventSource::resize(geom::Size const& size)
+{
+    MirEvent e;
+    memset(&e, 0, sizeof e);
+    e.type = mir_event_type_resize;
+    e.resize.surface_id = id.as_value();
+    e.resize.width = size.width.as_int();
+    e.resize.height = size.height.as_int();
+    event_sink->handle_event(e);
+}
+
+
+void ms::SurfaceEventSource::attrib_change(MirSurfaceAttrib attrib, int value)
+{
+    MirEvent e;
+
+    // This memset is not really required. However it does avoid some
+    // harmless uninitialized memory reads that valgrind will complain
+    // about, due to gaps in MirEvent.
+    memset(&e, 0, sizeof e);
+
+    e.type = mir_event_type_surface;
+    e.surface.id = id.as_value();
+    e.surface.attrib = attrib;
+    e.surface.value = value;
+
+    event_sink->handle_event(e);
+}

=== modified file 'src/server/shell/organising_surface_factory.cpp'
--- src/server/shell/organising_surface_factory.cpp	2014-03-31 11:00:33 +0000
+++ src/server/shell/organising_surface_factory.cpp	2014-03-31 11:00:33 +0000
@@ -42,13 +42,12 @@
 
 std::shared_ptr<msh::Surface> msh::OrganisingSurfaceFactory::create_surface(
     Session* session,
-    shell::SurfaceCreationParameters const& params,
-    frontend::SurfaceId id,
-    std::shared_ptr<mf::EventSink> const& sender)
+    SurfaceCreationParameters const& params,
+    std::shared_ptr<scene::SurfaceObserver> const& observer)
 {
     auto placed_params = placement_strategy->place(*session, params);
 
-    return surface_coordinator->add_surface(id, placed_params, sender);
+    return surface_coordinator->add_surface(placed_params, observer);
 }
 
 void msh::OrganisingSurfaceFactory::destroy_surface(std::shared_ptr<Surface> const& surface)

=== modified file 'src/server/shell/organising_surface_factory.h'
--- src/server/shell/organising_surface_factory.h	2014-03-31 11:00:33 +0000
+++ src/server/shell/organising_surface_factory.h	2014-03-31 11:00:33 +0000
@@ -41,9 +41,8 @@
 
     std::shared_ptr<Surface> create_surface(
         Session* session,
-        shell::SurfaceCreationParameters const& params,
-        frontend::SurfaceId id,
-        std::shared_ptr<frontend::EventSink> const& sink) override;
+        SurfaceCreationParameters const& params,
+        std::shared_ptr<scene::SurfaceObserver> const& observer) override;
 
     void destroy_surface(std::shared_ptr<Surface> const& surface) override;
 

=== modified file 'tests/acceptance-tests/test_client_input.cpp'
--- tests/acceptance-tests/test_client_input.cpp	2014-03-26 14:20:14 +0000
+++ tests/acceptance-tests/test_client_input.cpp	2014-03-31 11:00:33 +0000
@@ -335,12 +335,12 @@
     {
     }
 
-    std::shared_ptr<msh::Surface> create_surface(msh::Session* session,
-                                                 msh::SurfaceCreationParameters const& params,
-                                                 mf::SurfaceId id,
-                                                 std::shared_ptr<mf::EventSink> const& sink)
+    std::shared_ptr<msh::Surface> create_surface(
+        msh::Session* session,
+        msh::SurfaceCreationParameters const& params,
+        std::shared_ptr<ms::SurfaceObserver> const& observer) override
     {
-        auto surface = underlying_factory->create_surface(session, params, id, sink);
+        auto surface = underlying_factory->create_surface(session, params, observer);
 
         surface->set_input_region(input_rectangles);
 

=== modified file 'tests/integration-tests/graphics/android/test_internal_client.cpp'
--- tests/integration-tests/graphics/android/test_internal_client.cpp	2014-03-31 11:00:33 +0000
+++ tests/integration-tests/graphics/android/test_internal_client.cpp	2014-03-31 11:00:33 +0000
@@ -33,6 +33,7 @@
 #include "mir/scene/scene_report.h"
 #include "src/server/scene/surface_allocator.h"
 #include "mir/scene/surface.h"
+#include "mir/scene/surface_event_source.h"
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/frontend/surface_id.h"
 #include "mir/input/input_channel_factory.h"
@@ -98,7 +99,8 @@
     auto surface_allocator = std::make_shared<ms::SurfaceAllocator>(buffer_stream_factory, stub_input_factory, std::make_shared<mtd::NullSurfaceConfigurator>(), scene_report);
     auto ss = std::make_shared<ms::SurfaceStack>(stub_input_registrar, scene_report);
     auto surface_controller = std::make_shared<ms::SurfaceController>(surface_allocator, ss);
-    auto surface = surface_controller->add_surface(id, params, std::shared_ptr<mf::EventSink>());
+    auto const observer = std::make_shared<ms::SurfaceEventSource>(id, std::shared_ptr<mf::EventSink>());
+    auto surface = surface_controller->add_surface(params, observer);
     surface->allow_framedropping(true);
     auto mir_surface = as_internal_surface(surface);
 

=== modified file 'tests/integration-tests/test_session_manager.cpp'
--- tests/integration-tests/test_session_manager.cpp	2014-03-06 06:05:17 +0000
+++ tests/integration-tests/test_session_manager.cpp	2014-03-31 11:00:33 +0000
@@ -60,7 +60,7 @@
 
     }
 
-    mtd::MockSurfaceFactory surface_factory;
+    mtd::MockSurfaceFactory surface_factory; // TODO this isn't used as a mock
     ms::DefaultSessionContainer container;
     mtd::MockFocusSetter focus_setter;
     std::shared_ptr<mf::Session> new_session;

=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
--- tests/unit-tests/scene/test_application_session.cpp	2014-03-31 11:00:33 +0000
+++ tests/unit-tests/scene/test_application_session.cpp	2014-03-31 11:00:33 +0000
@@ -84,7 +84,7 @@
     mtd::NullEventSink sender;
     mtd::MockSurfaceFactory surface_factory;
 
-    EXPECT_CALL(surface_factory, create_surface(_, _, _, _))
+    EXPECT_CALL(surface_factory, create_surface(_, _, _))
         .WillOnce(Return(mock_surface));
 
     mtd::MockSessionListener listener;
@@ -115,9 +115,9 @@
 
     mtd::NullEventSink sender;
     mtd::MockSurfaceFactory surface_factory;
-    ON_CALL(surface_factory, create_surface(_,_,_,_)).WillByDefault(Return(mock_surface));
+    ON_CALL(surface_factory, create_surface(_,_,_)).WillByDefault(Return(mock_surface));
 
-    EXPECT_CALL(surface_factory, create_surface(_, _, _, _));
+    EXPECT_CALL(surface_factory, create_surface(_, _, _));
 
     mtd::MockSessionListener listener;
     EXPECT_CALL(listener, surface_created(_, _)).Times(1);
@@ -146,11 +146,11 @@
 
     {
         InSequence seq;
-        EXPECT_CALL(surface_factory, create_surface(_, _, _, _)).Times(1)
-            .WillOnce(Return(make_mock_surface()));
-        EXPECT_CALL(surface_factory, create_surface(_, _, _, _)).Times(1)
-            .WillOnce(Return(make_mock_surface()));
-        EXPECT_CALL(surface_factory, create_surface(_, _, _, _)).Times(1)
+        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
+            .WillOnce(Return(make_mock_surface()));
+        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
+            .WillOnce(Return(make_mock_surface()));
+        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1)
             .WillOnce(Return(make_mock_surface()));
     }
 
@@ -189,7 +189,7 @@
     auto mock_surface = make_mock_surface();
 
     mtd::MockSurfaceFactory surface_factory;
-    ON_CALL(surface_factory, create_surface(_, _, _, _)).WillByDefault(Return(mock_surface));
+    ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(Return(mock_surface));
 
     ms::ApplicationSession app_session(
         mt::fake_shared(surface_factory),
@@ -199,7 +199,7 @@
         std::make_shared<msh::NullSessionListener>(),
         mt::fake_shared(sender));
 
-    EXPECT_CALL(surface_factory, create_surface(_, _, _, _));
+    EXPECT_CALL(surface_factory, create_surface(_, _, _));
 
     {
         InSequence seq;
@@ -269,7 +269,7 @@
         std::static_pointer_cast<msh::SurfaceBufferAccess>(default_surface);
     auto const snapshot_strategy = std::make_shared<MockSnapshotStrategy>();
 
-    EXPECT_CALL(surface_factory, create_surface(_,_,_,_))
+    EXPECT_CALL(surface_factory, create_surface(_,_,_))
         .WillOnce(Return(default_surface));
 
     EXPECT_CALL(*snapshot_strategy,

=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp	2014-03-26 14:20:14 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp	2014-03-31 11:00:33 +0000
@@ -85,7 +85,6 @@
     std::function<void()> mock_change_cb;
     std::shared_ptr<testing::NiceMock<mtd::MockBufferStream>> mock_buffer_stream =
         std::make_shared<testing::NiceMock<mtd::MockBufferStream>>();
-    std::shared_ptr<StubEventSink> const stub_event_sink = std::make_shared<StubEventSink>();
     std::shared_ptr<StubSurfaceConfigurator> const stub_configurator = std::make_shared<StubSurfaceConfigurator>();
     std::shared_ptr<ms::SceneReport> const report = mr::null_scene_report();
 };
@@ -95,13 +94,11 @@
 TEST_F(BasicSurfaceTest, basics)
 {
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -117,13 +114,11 @@
         .Times(1);
 
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -144,13 +139,11 @@
         .Times(1);
 
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -173,13 +166,11 @@
         .Times(1);
 
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -204,13 +195,11 @@
         .Times(1);
 
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -225,13 +214,11 @@
 {
     using namespace testing;
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -242,13 +229,11 @@
 TEST_F(BasicSurfaceTest, test_surface_should_be_rendered_in)
 {
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -281,13 +266,11 @@
         .Times(1);
 
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -303,13 +286,11 @@
         .Times(1);
 
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -324,13 +305,11 @@
     geom::Point pt(1,1);
     geom::Size one_by_one{geom::Width{1}, geom::Height{1}};
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         geom::Rectangle{pt, one_by_one},
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 
@@ -367,13 +346,11 @@
     };
 
     ms::BasicSurface surface{
-        mf::SurfaceId(),
         name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report};
 

=== modified file 'tests/unit-tests/scene/test_session_manager.cpp'
--- tests/unit-tests/scene/test_session_manager.cpp	2014-03-31 11:00:33 +0000
+++ tests/unit-tests/scene/test_session_manager.cpp	2014-03-31 11:00:33 +0000
@@ -85,13 +85,11 @@
     }
 
     std::shared_ptr<ms::Surface> dummy_surface = std::make_shared<ms::BasicSurface>(
-        mf::SurfaceId(),
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         std::make_shared<mtd::StubBufferStream>(),
         std::shared_ptr<mi::InputChannel>(),
-        std::shared_ptr<mf::EventSink>(),
         std::shared_ptr<ms::SurfaceConfigurator>(),
         mir::report::null_scene_report());
     mtd::MockSurfaceFactory surface_factory;
@@ -121,9 +119,9 @@
 {
     using namespace ::testing;
 
-    EXPECT_CALL(surface_factory, create_surface(_, _, _, _)).Times(1);
+    EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1);
 
-    ON_CALL(surface_factory, create_surface(_, _, _, _)).WillByDefault(
+    ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(
        Return(dummy_surface));
 
     EXPECT_CALL(container, insert_session(_)).Times(1);
@@ -153,7 +151,7 @@
 TEST_F(SessionManagerSetup, create_surface_for_session_forwards_and_then_focuses_session)
 {
     using namespace ::testing;
-    ON_CALL(surface_factory, create_surface(_, _, _, _)).WillByDefault(
+    ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(
        Return(dummy_surface));
 
     // Once for session creation and once for surface creation
@@ -161,7 +159,7 @@
         InSequence seq;
 
         EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1); // Session creation
-        EXPECT_CALL(surface_factory, create_surface(_, _, _, _)).Times(1);
+        EXPECT_CALL(surface_factory, create_surface(_, _, _)).Times(1);
         EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1); // Post Surface creation
     }
 

=== modified file 'tests/unit-tests/scene/test_surface.cpp'
--- tests/unit-tests/scene/test_surface.cpp	2014-03-26 14:20:14 +0000
+++ tests/unit-tests/scene/test_surface.cpp	2014-03-31 11:00:33 +0000
@@ -21,6 +21,7 @@
 #include "mir/frontend/event_sink.h"
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/scene/surface_configurator.h"
+#include "mir/scene/surface_event_source.h"
 #include "mir/input/input_channel.h"
 
 #include "mir_test_doubles/mock_buffer_stream.h"
@@ -209,7 +210,6 @@
     std::function<void()> change_notification;
     int notification_count;
     mtd::StubBuffer stub_buffer;
-    std::shared_ptr<StubEventSink> const stub_event_sink = std::make_shared<StubEventSink>();
     std::shared_ptr<StubSurfaceConfigurator> const stub_configurator = std::make_shared<StubSurfaceConfigurator>();
 };
 
@@ -219,13 +219,11 @@
 {
     using namespace testing;
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -241,13 +239,11 @@
 TEST_F(SurfaceCreation, test_surface_gets_right_name)
 {
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -257,13 +253,11 @@
 TEST_F(SurfaceCreation, test_surface_queries_state_for_size)
 {
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -274,13 +268,11 @@
 {
     using namespace testing;
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -302,13 +294,11 @@
     mtd::StubBuffer stub_buffer;
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -324,13 +314,11 @@
 TEST_F(SurfaceCreation, test_surface_gets_top_left)
 {
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -343,13 +331,11 @@
     geom::Point p{55, 66};
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -366,18 +352,19 @@
         .Times(1);
 
     auto const mock_event_sink = std::make_shared<MockEventSink>();
+    auto const observer = std::make_shared<ms::SurfaceEventSource>(mf::SurfaceId(), mock_event_sink);
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        mock_event_sink,
         stub_configurator,
         report);
 
+    surf.add_observer(observer);
+
     ASSERT_THAT(surf.size(), Ne(new_size));
 
     EXPECT_CALL(*mock_event_sink, handle_event(_)).Times(1);
@@ -390,18 +377,19 @@
     using namespace testing;
     geom::Size const new_size{123, 456};
     auto const mock_event_sink = std::make_shared<MockEventSink>();
+    auto const observer = std::make_shared<ms::SurfaceEventSource>(mf::SurfaceId(), mock_event_sink);
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        mock_event_sink,
         stub_configurator,
         report);
 
+    surf.add_observer(observer);
+
     ASSERT_THAT(surf.size(), Ne(new_size));
 
     EXPECT_CALL(*mock_buffer_stream, resize(new_size)).Times(1);
@@ -428,13 +416,11 @@
         .WillOnce(Throw(std::runtime_error("bad resize")));
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -459,13 +445,11 @@
     };
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -484,13 +468,11 @@
 {
     auto mock_channel = std::make_shared<MockInputChannel>();
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         mock_channel,
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -503,13 +485,11 @@
 
     float alpha = 0.5f;
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -524,13 +504,11 @@
     EXPECT_CALL(*mock_buffer_stream, force_requests_to_complete()).Times(Exactly(1));
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -545,13 +523,11 @@
         .Times(1);
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -561,13 +537,11 @@
 TEST_F(SurfaceCreation, test_surface_next_buffer_tells_state_on_first_frame)
 {
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -589,13 +563,11 @@
     using namespace testing;
 
     ms::BasicSurface surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_event_sink,
         stub_configurator,
         report);
 
@@ -608,12 +580,10 @@
     EXPECT_CALL(channel, client_fd()).Times(1).WillOnce(Return(client_fd));
 
     ms::BasicSurface input_surf(
-        mf::SurfaceId(),
         surface_name,
         rect,
         false,
         mock_buffer_stream,mt::fake_shared(channel),
-        stub_event_sink,
         stub_configurator,
         report);
 

=== modified file 'tests/unit-tests/scene/test_surface_controller.cpp'
--- tests/unit-tests/scene/test_surface_controller.cpp	2014-03-31 11:00:33 +0000
+++ tests/unit-tests/scene/test_surface_controller.cpp	2014-03-31 11:00:33 +0000
@@ -21,6 +21,7 @@
 #include "mir/scene/surface_factory.h"
 #include "mir/shell/surface_creation_parameters.h"
 
+#include "mir_test_doubles/mock_surface.h"
 #include "mir_test/fake_shared.h"
 
 #include <gtest/gtest.h>
@@ -30,15 +31,14 @@
 namespace msh = mir::shell;
 namespace ms = mir::scene;
 namespace mt = mir::test;
+namespace mtd = mir::test::doubles;
 
 namespace
 {
 struct MockSurfaceAllocator : public ms::SurfaceFactory
 {
-    MOCK_METHOD3(create_surface, std::shared_ptr<ms::Surface>(
-        mf::SurfaceId id,
-        msh::SurfaceCreationParameters const&,
-        std::shared_ptr<mf::EventSink> const&));
+    MOCK_METHOD1(create_surface, std::shared_ptr<ms::Surface>(
+        msh::SurfaceCreationParameters const&));
 };
 
 struct MockSurfaceStackModel : public ms::SurfaceStackModel
@@ -56,19 +56,24 @@
 {
     using namespace ::testing;
 
-    std::shared_ptr<ms::Surface> null_surface;
+    mtd::MockSurface mock_surface;
+    std::shared_ptr<ms::Surface> const expect_surface = mt::fake_shared(mock_surface);
+    auto const surface = std::make_shared<mtd::MockSurface>();
     testing::NiceMock<MockSurfaceAllocator> mock_surface_allocator;
     MockSurfaceStackModel model;
 
     ms::SurfaceController controller(mt::fake_shared(mock_surface_allocator), mt::fake_shared(model));
 
     InSequence seq;
-    EXPECT_CALL(mock_surface_allocator, create_surface(_,_,_)).Times(1).WillOnce(Return(null_surface));
+    EXPECT_CALL(mock_surface_allocator, create_surface(_)).Times(1).WillOnce(Return(expect_surface));
+    EXPECT_CALL(mock_surface, add_observer(_)).Times(1);
     EXPECT_CALL(model, add_surface(_,_,_)).Times(1);
     EXPECT_CALL(model, remove_surface(_)).Times(1);
 
-    auto surface = controller.add_surface(mf::SurfaceId(), msh::a_surface(), {});
-    controller.remove_surface(surface);
+    auto actual_surface = controller.add_surface(msh::a_surface(), std::shared_ptr<ms::SurfaceObserver>());
+
+    EXPECT_THAT(actual_surface, Eq(expect_surface));
+    controller.remove_surface(actual_surface);
 }
 
 TEST(SurfaceController, raise_surface)

=== modified file 'tests/unit-tests/scene/test_surface_impl.cpp'
--- tests/unit-tests/scene/test_surface_impl.cpp	2014-03-31 11:00:33 +0000
+++ tests/unit-tests/scene/test_surface_impl.cpp	2014-03-31 11:00:33 +0000
@@ -17,6 +17,8 @@
  */
 
 #include "src/server/scene/basic_surface.h"
+#include "mir/scene/surface_observer.h"
+#include "mir/scene/surface_event_source.h"
 #include "src/server/report/null_report_factory.h"
 #include "mir/frontend/event_sink.h"
 #include "mir/graphics/display_configuration.h"
@@ -65,7 +67,6 @@
 
     Surface() :
         buffer_stream(std::make_shared<StubBufferStream>()),
-        stub_sender(std::make_shared<mtd::NullEventSink>()),
         null_configurator(std::make_shared<mtd::NullSurfaceConfigurator>())
     {
         using namespace testing;
@@ -76,7 +77,6 @@
             .WillByDefault(InvokeArgument<1>(nullptr));
     }
     mf::SurfaceId stub_id;
-    std::shared_ptr<mf::EventSink> stub_sender;
     std::shared_ptr<ms::SurfaceConfigurator> null_configurator;
     std::shared_ptr<ms::SceneReport> const report = mr::null_scene_report();
 };
@@ -87,13 +87,11 @@
     using namespace testing;
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_sender,
         null_configurator,
         report);
 
@@ -107,13 +105,11 @@
     using namespace testing;
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_sender,
         null_configurator,
         report);
 
@@ -148,13 +144,11 @@
     using namespace testing;
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_sender,
         null_configurator,
         report);
 
@@ -196,18 +190,19 @@
 
     geom::Size const new_size{123, 456};
     auto sink = std::make_shared<MockEventSink>();
+    auto const observer = std::make_shared<ms::SurfaceEventSource>(stub_id, sink);
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        sink,
         null_configurator,
         report);
 
+    surf.add_observer(observer);
+
     MirEvent e;
     memset(&e, 0, sizeof e);
     e.type = mir_event_type_resize;
@@ -228,18 +223,19 @@
     geom::Size const new_size{123, 456};
     geom::Size const new_size2{789, 1011};
     auto sink = std::make_shared<MockEventSink>();
+    auto const observer = std::make_shared<ms::SurfaceEventSource>(stub_id, sink);
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        sink,
         null_configurator,
         report);
 
+    surf.add_observer(observer);
+
     MirEvent e;
     memset(&e, 0, sizeof e);
     e.type = mir_event_type_resize;
@@ -272,13 +268,11 @@
 TEST_F(Surface, remembers_alpha)
 {
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_sender,
         null_configurator,
         report);
 
@@ -311,17 +305,19 @@
             .Times(1);
     }
 
+    auto const observer = std::make_shared<ms::SurfaceEventSource>(stub_id, mt::fake_shared(sink));
+
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        mt::fake_shared(sink),
         null_configurator,
         report);
 
+    surf.add_observer(observer);
+
     surf.configure(mir_surface_attrib_focus, mir_surface_focused);
     surf.configure(mir_surface_attrib_focus, mir_surface_unfocused);
 }
@@ -337,13 +333,11 @@
     EXPECT_CALL(configurator, attribute_set(_, mir_surface_attrib_state, mir_surface_state_minimized)).Times(1);
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_sender,
         mt::fake_shared(configurator),
         report);
 
@@ -355,13 +349,11 @@
     using namespace ::testing;
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_sender,
         null_configurator,
         report);
 
@@ -376,13 +368,11 @@
     auto stub_buffer_stream = std::make_shared<mtd::StubBufferStream>();
 
     ms::BasicSurface surf(
-        stub_id,
         std::string("stub"),
         geom::Rectangle{{},{}},
         false,
         stub_buffer_stream,
         std::shared_ptr<mi::InputChannel>(),
-        stub_sender,
         null_configurator,
         report);
 

=== modified file 'tests/unit-tests/scene/test_surface_stack.cpp'
--- tests/unit-tests/scene/test_surface_stack.cpp	2014-03-26 14:20:14 +0000
+++ tests/unit-tests/scene/test_surface_stack.cpp	2014-03-31 11:00:33 +0000
@@ -128,35 +128,29 @@
         default_params = msh::a_surface().of_size(geom::Size{geom::Width{1024}, geom::Height{768}});
 
         stub_surface1 = std::make_shared<ms::BasicSurface>(
-            mf::SurfaceId(__LINE__),
             std::string("stub"),
             geom::Rectangle{{},{}},
             false,
             std::make_shared<mtd::StubBufferStream>(),
             std::shared_ptr<mir::input::InputChannel>(),
-            std::shared_ptr<mf::EventSink>(),
             std::shared_ptr<ms::SurfaceConfigurator>(),
             report);
 
         stub_surface2 = std::make_shared<ms::BasicSurface>(
-            mf::SurfaceId(__LINE__),
             std::string("stub"),
             geom::Rectangle{{},{}},
             false,
             std::make_shared<mtd::StubBufferStream>(),
             std::shared_ptr<mir::input::InputChannel>(),
-            std::shared_ptr<mf::EventSink>(),
             std::shared_ptr<ms::SurfaceConfigurator>(),
             report);
 
         stub_surface3 = std::make_shared<ms::BasicSurface>(
-            mf::SurfaceId(__LINE__),
             std::string("stub"),
             geom::Rectangle{{},{}},
             false,
             std::make_shared<mtd::StubBufferStream>(),
             std::shared_ptr<mir::input::InputChannel>(),
-            std::shared_ptr<mf::EventSink>(),
             std::shared_ptr<ms::SurfaceConfigurator>(),
             report);
     }
@@ -568,13 +562,11 @@
     for(auto i = 0u; i < num_surfaces; i++)
     {
         auto const surface = std::make_shared<ms::BasicSurface>(
-            mf::SurfaceId(__LINE__),
             std::string("stub"),
             geom::Rectangle{geom::Point{3 * i, 4 * i},geom::Size{1 * i, 2 * i}},
             true,
             std::make_shared<mtd::StubBufferStream>(),
             std::shared_ptr<mir::input::InputChannel>(),
-            std::shared_ptr<mf::EventSink>(),
             std::shared_ptr<ms::SurfaceConfigurator>(),
             report);
 

=== modified file 'tests/unit-tests/shell/test_organising_surface_factory.cpp'
--- tests/unit-tests/shell/test_organising_surface_factory.cpp	2014-03-31 11:00:33 +0000
+++ tests/unit-tests/shell/test_organising_surface_factory.cpp	2014-03-31 11:00:33 +0000
@@ -20,6 +20,8 @@
 #include "mir/shell/placement_strategy.h"
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/shell/session.h"
+#include "mir/scene/surface.h"
+#include "mir/scene/surface_event_source.h"
 #include "mir/scene/surface_coordinator.h"
 
 #include "mir_test_doubles/stub_shell_session.h"
@@ -38,10 +40,9 @@
 {
 struct MockSurfaceCoordinator : public ms::SurfaceCoordinator
 {
-    MOCK_METHOD3(add_surface, std::shared_ptr<ms::Surface>(
-        mf::SurfaceId,
+    MOCK_METHOD2(add_surface, std::shared_ptr<ms::Surface>(
         msh::SurfaceCreationParameters const&,
-        std::shared_ptr<mf::EventSink> const&));
+        std::shared_ptr<ms::SurfaceObserver> const&));
 
     void remove_surface(std::weak_ptr<ms::Surface> const& /*surface*/) override {}
     void raise(std::weak_ptr<ms::Surface> const& /*surface*/)  override {}
@@ -57,10 +58,11 @@
     void SetUp()
     {
         using namespace ::testing;
-        ON_CALL(*surface_coordinator, add_surface(_, _, _)).WillByDefault(Return(null_surface));
+        ON_CALL(*surface_coordinator, add_surface(_, _)).WillByDefault(Return(null_surface));
     }
     std::shared_ptr<ms::Surface> null_surface;
     std::shared_ptr<MockSurfaceCoordinator> surface_coordinator = std::make_shared<MockSurfaceCoordinator>();
+    std::shared_ptr<ms::SurfaceObserver> const observer = std::make_shared<ms::SurfaceEventSource>(mf::SurfaceId(), std::make_shared<mtd::NullEventSink>());
     std::shared_ptr<MockPlacementStrategy> placement_strategy = std::make_shared<MockPlacementStrategy>();
 };
 
@@ -73,13 +75,13 @@
     msh::OrganisingSurfaceFactory factory(surface_coordinator, placement_strategy);
 
     mtd::StubShellSession session;
-    EXPECT_CALL(*surface_coordinator, add_surface(_, _, _)).Times(1);
+    EXPECT_CALL(*surface_coordinator, add_surface(_, _)).Times(1);
 
     auto params = msh::a_surface();
     EXPECT_CALL(*placement_strategy, place(Ref(session), Ref(params))).Times(1)
         .WillOnce(Return(msh::a_surface()));
 
-    factory.create_surface(&session, params, mf::SurfaceId(), std::make_shared<mtd::NullEventSink>());
+    factory.create_surface(&session, params, observer);
 }
 
 TEST_F(OrganisingSurfaceFactorySetup, forwards_create_surface_parameters_from_placement_strategy_to_underlying_factory)
@@ -88,14 +90,13 @@
 
     msh::OrganisingSurfaceFactory factory(surface_coordinator, placement_strategy);
 
-    std::shared_ptr<mf::EventSink> sink = std::make_shared<mtd::NullEventSink>();
     auto params = msh::a_surface();
     auto placed_params = params;
     placed_params.size.width = geom::Width{100};
 
     EXPECT_CALL(*placement_strategy, place(_, Ref(params))).Times(1)
         .WillOnce(Return(placed_params));
-    EXPECT_CALL(*surface_coordinator, add_surface(mf::SurfaceId(), placed_params, sink));
+    EXPECT_CALL(*surface_coordinator, add_surface(placed_params, _));
 
-    factory.create_surface(nullptr, params, mf::SurfaceId(), sink);
+    factory.create_surface(nullptr, params, observer);
 }