Merge lp:~vanvugt/mir/parenting-server-api into lp:mir

[Daniel van Vugt]

Introduce basic surface parent information in the server API.

Server ABI broken, but we have already bumped it for this series.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2026
http://jenkins.qa.ubuntu.com/job/mir-ci/1985/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2352
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2362
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/331
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/331/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1224
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1224/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3259
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15400

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

59 + virtual void set_parent(std::weak_ptr<Surface> const&, int id) = 0;

It can't be right that both a pointer and an int are passed here.

Presumably they are both ways of referencing the same parent and passing them separately makes the interface easy to abuse.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alan: Actually yes that's the messy part, but is the lesser evil.

The problem is that surface ID is only meaningful in the upper layer (SessionMediator) where it's a key into the map. Down in the scene::Surface and scene::BasicSurface it's meaningless to mention IDs. So the obvious answer then is don't mention any surface ID in the lower layers (set_parent). You can indeed omit the "int id", but that creates two problems:
  1. query() doesn't work at all, because the surface implementation doesn't known its parent ID.
  2. Any future use of query(mir_surface_attrib_parent) in the upper layers would require bespoke hacks in the SessionMediator (return parent() and linear search for the surface pointer to get the ID).

So the options are either live with issues #1 & #2, or the current design to pass in the surface ID separately for storage in the BasicSurface's attribute list (which costs nothing as the memory is already reserved).

That said, making query() return no useful result looks like it could suffice for now. And it would simplify the interface as you point out. I might try it.

[Daniel van Vugt (vanvugt)]

OK, I tried making set_parent a one-parameter function. It got really ugly. I then had to document that query(mir_surface_attrib_parent) is non-functional, also lost parent-changed event support, and the resulting hacks required to make tests work again were too ugly. Because "parent" then became effectively not a surface attribute any more.

set_parent with two parameters is certainly cleaner, and more functional.

Alternatively, we could clean it up by adding an id() to scene::Surface, but I think that's unnecessary, more complex, and would be equally easy to abuse. The current proposal seems best.

[lp:~vanvugt/mir/parenting-server-api]

2027. By Daniel van Vugt on 2014-11-05

Merge latest development-branch

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2027
http://jenkins.qa.ubuntu.com/job/mir-ci/1994/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2365
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2375
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/340
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/340/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1237
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1237/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3272
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15445

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Hm. As mentioned on the client-side of this proposal I suspect it would make more sense for parent to _not_ be a surface attribute, but a create-time option; that should neatly resolve the problem that the int parameter of
59 + virtual void set_parent(std::weak_ptr<Surface> const&, int id) = 0;
is solving?

[Alberto Aguirre (albaguirre)]

59 + virtual void set_parent(std::weak_ptr<Surface> const&, int id) = 0;

The int id needs to go away.

"Alternatively, we could clean it up by adding an id() to scene::Surface, but I think that's unnecessary, more complex, and would be equally easy to abuse."

Why would that be equally easy to abuse? Ids for a surface would be created at construction time and never change.

" Because "parent" then became effectively not a surface attribute any more."

Perhaps that's a strong hint that it isn't then.

So I think the path to resolve this "needs fixing" is indeed to ad an id to Surface.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

It's ugly because:
  * id() to a Surface object is meaningless. Only external users of the surface need the ID.
  * You either have to make the constructor take an ID parameter (more annoying coupling), or add a set_id() function which is the part that would be equally easy to abuse.

But if it unblocks us, I might give it a go.

[lp:~vanvugt/mir/parenting-server-api]

2028. By Daniel van Vugt on 2014-11-06

Merge latest trunk

2029. By Daniel van Vugt on 2014-11-06

Merge latest trunk

2030. By Daniel van Vugt on 2014-11-06

Remove the controversial "int id" parameter.

2031. By Daniel van Vugt on 2014-11-06

(Re)introduce Session::configure_surface for binding attribute values
with session information (parent surface IDs).

Fun fact: the mock classes already have this method from a former life.
Somebody forgot to delete it :)

2032. By Daniel van Vugt on 2014-11-06

Add a unit test for ApplicationSession::configure_surface()

2033. By Daniel van Vugt on 2014-11-06

Expand new test to verify parenting occurs

2034. By Daniel van Vugt on 2014-11-06

Also verify that configure_surface() gives the correct shared_ptr to
set_parent().

2035. By Daniel van Vugt on 2014-11-07

Merge latest trunk

2036. By Daniel van Vugt on 2014-11-07

Add a dummy expectation to silence gmock (part of LP: #1390312)

2037. By Daniel van Vugt on 2014-11-10

Merge latest trunk

2038. By Daniel van Vugt on 2014-11-11

Merge latest trunk

2039. By Daniel van Vugt on 2014-11-12

Merge latest trunk and fix conflicts.

2040. By Daniel van Vugt on 2014-11-12

Tidy up ApplicationSession unit-tests' mocks.

2041. By Daniel van Vugt on 2014-11-13

Merge latest trunk

2042. By Daniel van Vugt on 2014-11-14

Merge latest trunk

2043. By Daniel van Vugt on 2014-11-17

Merge latest trunk

Unmerged revisions

2043. By Daniel van Vugt on 2014-11-17

Merge latest trunk

2042. By Daniel van Vugt on 2014-11-14

Merge latest trunk

2041. By Daniel van Vugt on 2014-11-13

Merge latest trunk

2040. By Daniel van Vugt on 2014-11-12

Tidy up ApplicationSession unit-tests' mocks.

2039. By Daniel van Vugt on 2014-11-12

Merge latest trunk and fix conflicts.

2038. By Daniel van Vugt on 2014-11-11

Merge latest trunk

2037. By Daniel van Vugt on 2014-11-10

Merge latest trunk

2036. By Daniel van Vugt on 2014-11-07

Add a dummy expectation to silence gmock (part of LP: #1390312)

2035. By Daniel van Vugt on 2014-11-07

Merge latest trunk

2034. By Daniel van Vugt on 2014-11-06

Also verify that configure_surface() gives the correct shared_ptr to
set_parent().

=== modified file 'client-ABI-sha1sums'
--- client-ABI-sha1sums	2014-10-30 04:58:27 +0000
+++ client-ABI-sha1sums	2014-11-06 09:26:44 +0000
@@ -8,7 +8,7 @@
 8609754db3be20e11e43858dd2c36b5bd480d5ec  include/client/mir_toolkit/mir_surface.h
 b141c4d79802ad626d969249c0004744e5c2a525  include/client/mir_toolkit/mir_wait.h
 9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
-183d9e5e6cfe48b3d9145a28541dd4202ff6137b  include/common/mir_toolkit/common.h
+5931de6ae2db9c8949f8b8435185c9ec20de396e  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 bdaceadd56e41d2cb708f7c4da97acf84dfc75b7  include/common/mir_toolkit/event.h
 4975998aa1056ed0d39dcc538127453e516ad8e9  include/common/mir_toolkit/mesa/native_display.h

=== modified file 'common-ABI-sha1sums'
--- common-ABI-sha1sums	2014-11-03 13:44:52 +0000
+++ common-ABI-sha1sums	2014-11-06 09:26:44 +0000
@@ -11,7 +11,7 @@
 be7d58c9fde2ce91cc66dd6144b76e08b536266b  include/common/mir/int_wrapper.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
-183d9e5e6cfe48b3d9145a28541dd4202ff6137b  include/common/mir_toolkit/common.h
+5931de6ae2db9c8949f8b8435185c9ec20de396e  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 bdaceadd56e41d2cb708f7c4da97acf84dfc75b7  include/common/mir_toolkit/event.h
 4975998aa1056ed0d39dcc538127453e516ad8e9  include/common/mir_toolkit/mesa/native_display.h

=== modified file 'include/common/mir_toolkit/common.h'
--- include/common/mir_toolkit/common.h	2014-10-01 06:25:56 +0000
+++ include/common/mir_toolkit/common.h	2014-11-06 09:26:44 +0000
@@ -42,6 +42,7 @@
     mir_surface_attrib_focus,
     mir_surface_attrib_dpi,
     mir_surface_attrib_visibility,
+    mir_surface_attrib_parent,
     /* Must be last */
     mir_surface_attribs
 } MirSurfaceAttrib;
@@ -86,6 +87,11 @@
     mir_surface_visibility_exposed
 } MirSurfaceVisibility;
 
+enum MirSurfaceParentNone
+{
+    mir_surface_parent_none = -1
+};
+
 typedef enum MirLifecycleState
 {
     mir_lifecycle_state_will_suspend,

=== modified file 'include/server/mir/frontend/session.h'
--- include/server/mir/frontend/session.h	2014-10-01 06:25:56 +0000
+++ include/server/mir/frontend/session.h	2014-11-06 09:26:44 +0000
@@ -49,6 +49,8 @@
     virtual SurfaceId create_surface(scene::SurfaceCreationParameters const& params) = 0;
     virtual void destroy_surface(SurfaceId surface) = 0;
     virtual std::shared_ptr<Surface> get_surface(SurfaceId surface) const = 0;
+    virtual int configure_surface(SurfaceId id, MirSurfaceAttrib at,
+                                  int value) = 0;
 
     virtual std::string name() const = 0;
 

=== modified file 'include/server/mir/frontend/surface.h'
--- include/server/mir/frontend/surface.h	2014-10-01 06:25:56 +0000
+++ include/server/mir/frontend/surface.h	2014-11-06 09:26:44 +0000
@@ -52,6 +52,9 @@
     virtual bool supports_input() const = 0;
     virtual int client_input_fd() const = 0;
 
+    virtual std::shared_ptr<Surface> parent() const = 0;
+    virtual void set_parent(std::weak_ptr<Surface> const&) = 0;
+
     virtual int configure(MirSurfaceAttrib attrib, int value) = 0;
     virtual int query(MirSurfaceAttrib attrib) = 0;
 

=== modified file 'platform-ABI-sha1sums'
--- platform-ABI-sha1sums	2014-11-05 10:06:14 +0000
+++ platform-ABI-sha1sums	2014-11-06 09:26:44 +0000
@@ -11,7 +11,7 @@
 be7d58c9fde2ce91cc66dd6144b76e08b536266b  include/common/mir/int_wrapper.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
-183d9e5e6cfe48b3d9145a28541dd4202ff6137b  include/common/mir_toolkit/common.h
+5931de6ae2db9c8949f8b8435185c9ec20de396e  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 bdaceadd56e41d2cb708f7c4da97acf84dfc75b7  include/common/mir_toolkit/event.h
 4975998aa1056ed0d39dcc538127453e516ad8e9  include/common/mir_toolkit/mesa/native_display.h

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2014-11-05 10:15:32 +0000
+++ server-ABI-sha1sums	2014-11-06 09:26:44 +0000
@@ -11,7 +11,7 @@
 be7d58c9fde2ce91cc66dd6144b76e08b536266b  include/common/mir/int_wrapper.h
 9ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
 9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
-183d9e5e6cfe48b3d9145a28541dd4202ff6137b  include/common/mir_toolkit/common.h
+5931de6ae2db9c8949f8b8435185c9ec20de396e  include/common/mir_toolkit/common.h
 fce4c1a9e0d037244f7e9e96ea2d8eaab4fc404c  include/common/mir_toolkit/cursors.h
 bdaceadd56e41d2cb708f7c4da97acf84dfc75b7  include/common/mir_toolkit/event.h
 4975998aa1056ed0d39dcc538127453e516ad8e9  include/common/mir_toolkit/mesa/native_display.h
@@ -59,10 +59,10 @@
 fae0008af826abbc4b4324d49e9c03d43b08765a  include/server/mir/frontend/screencast.h
 b729a7710c37d9f1fba56a6f8e8eae1c2559f57a  include/server/mir/frontend/session_authorizer.h
 34ce482df448fd2fc5f0c4ae5ac8b7fecbd228c9  include/server/mir/frontend/session_credentials.h
-e5ea465ed7e05f0e1d6a837d0d6b3a04c2d7fc19  include/server/mir/frontend/session.h
+1ae5b9c89ead7529b3c20301caf5869c4a52f75f  include/server/mir/frontend/session.h
 d754e5bb057f3018d7228170cf9a0c66d75d8cc2  include/server/mir/frontend/session_mediator_report.h
 68468aa2298c4e2cdc1bbb7cb5f250a914ae16c9  include/server/mir/frontend/shell.h
-f67b9788943a4b9121b3891dfbe979d57922ea78  include/server/mir/frontend/surface.h
+d33a31128bcb6908ec769bfdb9897781748d2472  include/server/mir/frontend/surface.h
 618b43a84cce0ad671ed68fe2ba796fbc7b79e31  include/server/mir/frontend/surface_id.h
 f95c2bddf13d15993ef5d6a0ad7b9106ae550b87  include/server/mir/input/composite_event_filter.h
 67719acb03b35d383dfefd65e8dfb872c42bcc11  include/server/mir/input/cursor_images.h

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2014-10-30 18:37:11 +0000
+++ src/server/frontend/session_mediator.cpp	2014-11-06 09:26:44 +0000
@@ -359,8 +359,7 @@
 
         auto const id = frontend::SurfaceId(request->surfaceid().value());
         int value = request->ivalue();
-        auto const surface = session->get_surface(id);
-        int newvalue = surface->configure(attrib, value);
+        int newvalue = session->configure_surface(id, attrib, value);
 
         response->set_ivalue(newvalue);
     }

=== modified file 'src/server/scene/application_session.cpp'
--- src/server/scene/application_session.cpp	2014-10-01 06:25:56 +0000
+++ src/server/scene/application_session.cpp	2014-11-06 09:26:44 +0000
@@ -105,6 +105,23 @@
     return checked_find(id)->second;
 }
 
+int ms::ApplicationSession::configure_surface(mf::SurfaceId id,
+                                              MirSurfaceAttrib attrib,
+                                              int value)
+{
+    auto surface = get_surface(id);
+
+    if (attrib == mir_surface_attrib_parent)
+    {
+        std::shared_ptr<mf::Surface> parent;
+        if (value != mir_surface_parent_none)
+            parent = get_surface(mf::SurfaceId(value));
+        surface->set_parent(parent);
+    }
+
+    return surface->configure(attrib, value);
+}
+
 void ms::ApplicationSession::take_snapshot(SnapshotCallback const& snapshot_taken)
 {
     if (auto surface = default_surface())

=== modified file 'src/server/scene/application_session.h'
--- src/server/scene/application_session.h	2014-11-03 06:51:26 +0000
+++ src/server/scene/application_session.h	2014-11-06 09:26:44 +0000
@@ -55,6 +55,7 @@
     frontend::SurfaceId create_surface(SurfaceCreationParameters const& params) override;
     void destroy_surface(frontend::SurfaceId surface) override;
     std::shared_ptr<frontend::Surface> get_surface(frontend::SurfaceId surface) const override;
+    int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int) override;
 
     void take_snapshot(SnapshotCallback const& snapshot_taken) override;
     std::shared_ptr<Surface> default_surface() const override;

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2014-10-21 16:21:14 +0000
+++ src/server/scene/basic_surface.cpp	2014-11-06 09:26:44 +0000
@@ -35,6 +35,7 @@
 #include <algorithm>
 
 namespace mc = mir::compositor;
+namespace mf = mir::frontend;
 namespace ms = mir::scene;
 namespace msh = mir::shell;
 namespace mg = mir::graphics;
@@ -141,6 +142,7 @@
     attrib_values[mir_surface_attrib_focus] = mir_surface_unfocused;
     attrib_values[mir_surface_attrib_dpi] = 0;
     attrib_values[mir_surface_attrib_visibility] = mir_surface_visibility_exposed;
+    attrib_values[mir_surface_attrib_parent] = mir_surface_parent_none;
 }
 
 void ms::BasicSurface::force_requests_to_complete()
@@ -166,6 +168,38 @@
     return surface_name;
 }
 
+void ms::BasicSurface::set_parent(std::weak_ptr<mf::Surface> const& p)
+{
+    std::unique_lock<std::mutex> lk(guard);
+    weak_parent = p;
+}
+
+int ms::BasicSurface::set_parent_id(int id)
+{
+    std::unique_lock<std::mutex> lk(guard);
+    if (attrib_values[mir_surface_attrib_parent] != id)
+    {
+        attrib_values[mir_surface_attrib_parent] = id;
+        lk.unlock();
+        observers.attrib_changed(mir_surface_attrib_parent, id);
+    }
+    return id;
+}
+
+std::shared_ptr<mf::Surface> ms::BasicSurface::parent() const
+{
+    std::unique_lock<std::mutex> lk(guard);
+    return weak_parent.lock();
+}
+
+/* Coming soon: Parent-relative placement. Something like...
+void ms::BasicSurface::place(geometry::Displacement const& disp)
+{
+   if (auto p = parent())
+       move_to(p->screen_position().top_left() + disp);
+}
+*/
+
 void ms::BasicSurface::move_to(geometry::Point const& top_left)
 {
     {
@@ -500,6 +534,9 @@
     case mir_surface_attrib_visibility:
         result = set_visibility(static_cast<MirSurfaceVisibility>(result));
         break;
+    case mir_surface_attrib_parent:
+        result = set_parent_id(result);
+        break;
     default:
         BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
                                                "attribute."));

=== modified file 'src/server/scene/basic_surface.h'
--- src/server/scene/basic_surface.h	2014-11-03 06:51:26 +0000
+++ src/server/scene/basic_surface.h	2014-11-06 09:26:44 +0000
@@ -147,6 +147,9 @@
 
     int dpi() const;
 
+    void set_parent(std::weak_ptr<frontend::Surface> const& parent) override;
+    std::shared_ptr<frontend::Surface> parent() const override;
+
 private:
     bool visible(std::unique_lock<std::mutex>&) const;
     MirSurfaceType set_type(MirSurfaceType t);  // Use configure() to make public changes
@@ -155,6 +158,7 @@
     MirSurfaceVisibility set_visibility(MirSurfaceVisibility v);
     int set_swap_interval(int);
     MirSurfaceFocusState set_focus_state(MirSurfaceFocusState f);
+    int set_parent_id(int);
 
     SurfaceObservers observers;
     std::mutex mutable guard;
@@ -173,6 +177,7 @@
     std::shared_ptr<SurfaceConfigurator> const configurator;
     std::shared_ptr<graphics::CursorImage> cursor_image_;
     std::shared_ptr<SceneReport> const report;
+    std::weak_ptr<frontend::Surface> weak_parent;
 
     void initialize_attributes();
     int attrib_values[mir_surface_attribs];

=== modified file 'tests/include/mir_test_doubles/mock_frontend_surface.h'
--- tests/include/mir_test_doubles/mock_frontend_surface.h	2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/mock_frontend_surface.h	2014-11-06 09:26:44 +0000
@@ -62,6 +62,9 @@
 
     MOCK_CONST_METHOD0(supports_input, bool());
     MOCK_CONST_METHOD0(client_input_fd, int());
+
+    MOCK_CONST_METHOD0(parent, std::shared_ptr<frontend::Surface>());
+    MOCK_METHOD1(set_parent, void(std::weak_ptr<Surface> const&));
     
     MOCK_METHOD1(set_cursor_image, void(std::shared_ptr<graphics::CursorImage> const&));
 

=== modified file 'tests/include/mir_test_doubles/mock_surface.h'
--- tests/include/mir_test_doubles/mock_surface.h	2014-10-27 22:31:16 +0000
+++ tests/include/mir_test_doubles/mock_surface.h	2014-11-06 09:26:44 +0000
@@ -67,6 +67,9 @@
     MOCK_METHOD1(take_input_focus, void(std::shared_ptr<shell::InputTargeter> const&));
     MOCK_METHOD1(add_observer, void(std::shared_ptr<scene::SurfaceObserver> const&));
     MOCK_METHOD1(remove_observer, void(std::weak_ptr<scene::SurfaceObserver> const&));
+
+    MOCK_METHOD1(set_parent, void(std::weak_ptr<frontend::Surface> const&));
+    MOCK_METHOD0(parent, std::shared_ptr<frontend::Surface>());
 };
 
 }

=== modified file 'tests/include/mir_test_doubles/stub_scene_session.h'
--- tests/include/mir_test_doubles/stub_scene_session.h	2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/stub_scene_session.h	2014-11-06 09:26:44 +0000
@@ -43,6 +43,10 @@
     {
         return std::shared_ptr<frontend::Surface>();
     }
+    int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int value) override
+    {
+        return value;
+    }
     std::string name() const override
     {
         return std::string();

=== modified file 'tests/include/mir_test_doubles/stub_scene_surface.h'
--- tests/include/mir_test_doubles/stub_scene_surface.h	2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/stub_scene_surface.h	2014-11-06 09:26:44 +0000
@@ -95,6 +95,8 @@
 
     bool supports_input() const override { return true;}
     int client_input_fd() const override { return fd;}
+    std::shared_ptr<frontend::Surface> parent() const override { return {}; }
+    void set_parent(std::weak_ptr<frontend::Surface> const&) override { }
     int configure(MirSurfaceAttrib, int) override { return 0; }
     int query(MirSurfaceAttrib) override { return 0; }
     void with_most_recent_buffer_do(std::function<void(graphics::Buffer&)> const& ) override {}

=== modified file 'tests/include/mir_test_doubles/stub_session.h'
--- tests/include/mir_test_doubles/stub_session.h	2014-10-01 06:25:56 +0000
+++ tests/include/mir_test_doubles/stub_session.h	2014-11-06 09:26:44 +0000
@@ -41,6 +41,10 @@
     {
         return std::shared_ptr<frontend::Surface>();
     }
+    int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int value) override
+    {
+        return value;
+    }
     std::string name() const override
     {
         return std::string();

=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2014-10-28 13:59:51 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2014-11-06 09:26:44 +0000
@@ -186,7 +186,8 @@
     { mir_surface_attrib_swapinterval, 1 },
     { mir_surface_attrib_focus, mir_surface_focused },
     { mir_surface_attrib_dpi, 19 },
-    { mir_surface_attrib_visibility, mir_surface_visibility_exposed }
+    { mir_surface_attrib_visibility, mir_surface_visibility_exposed },
+    { mir_surface_attrib_parent, 123 }
 };
 
 struct MockBuffer : public mcl::ClientBuffer

=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
--- tests/unit-tests/scene/test_application_session.cpp	2014-10-01 06:25:56 +0000
+++ tests/unit-tests/scene/test_application_session.cpp	2014-11-06 09:26:44 +0000
@@ -109,6 +109,52 @@
     session.destroy_surface(surf);
 }
 
+TEST(ApplicationSession, configures_surface)
+{
+    using namespace ::testing;
+
+    auto mock_surface = make_mock_surface();
+    auto mock_parent_surface = make_mock_surface();
+
+    mtd::NullEventSink sender;
+    mtd::MockSurfaceCoordinator surface_coordinator;
+
+    EXPECT_CALL(surface_coordinator, add_surface(_, _))
+        .WillOnce(Return(mock_parent_surface))
+        .WillOnce(Return(mock_surface));
+
+    ms::ApplicationSession session(
+        mt::fake_shared(surface_coordinator),
+        __LINE__,
+        "Foo",
+        std::make_shared<mtd::NullSnapshotStrategy>(),
+        std::make_shared<ms::NullSessionListener>(),
+        mt::fake_shared(sender));
+
+    ms::SurfaceCreationParameters params;
+    auto parent_id = session.create_surface(params);
+    auto child_id = session.create_surface(params);
+
+    EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_type,
+                                         mir_surface_type_dialog))
+        .WillOnce(Return(mir_surface_type_dialog));
+    std::weak_ptr<mf::Surface> parent_that_was_set;
+    EXPECT_CALL(*mock_surface, set_parent(_))
+        .WillOnce(SaveArg<0>(&parent_that_was_set));
+    EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_parent,
+                                         parent_id.as_value()))
+        .WillOnce(Return(parent_id.as_value()));
+
+    session.configure_surface(child_id, mir_surface_attrib_type,
+                              mir_surface_type_dialog);
+    session.configure_surface(child_id, mir_surface_attrib_parent,
+                              parent_id.as_value());
+
+    EXPECT_EQ(mock_parent_surface, parent_that_was_set.lock());
+    session.destroy_surface(child_id);
+    session.destroy_surface(parent_id);
+}
+
 TEST(ApplicationSession, listener_notified_of_surface_destruction_on_session_destruction)
 {
     using namespace ::testing;

=== modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
--- tests/unit-tests/scene/test_basic_surface.cpp	2014-10-01 06:25:56 +0000
+++ tests/unit-tests/scene/test_basic_surface.cpp	2014-11-06 09:26:44 +0000
@@ -528,6 +528,13 @@
     -1
 };
 
+AttributeTestParameters const surface_parent_test_parameters{
+    mir_surface_attrib_parent,
+    mir_surface_parent_none,
+    123,
+    mir_surface_parent_none
+};
+
 }
 
 TEST_P(BasicSurfaceAttributeTest, default_value)
@@ -591,9 +598,12 @@
     auto const& attribute = params.attribute;
     auto const& invalid_value = params.an_invalid_value;
     
-    EXPECT_THROW({
+    if (invalid_value != params.default_value)
+    {
+        EXPECT_THROW({
             surface.configure(attribute, invalid_value);
         }, std::logic_error);
+    }
 }
 
 INSTANTIATE_TEST_CASE_P(SurfaceTypeAttributeTest, BasicSurfaceAttributeTest,
@@ -614,6 +624,9 @@
 INSTANTIATE_TEST_CASE_P(SurfaceFocusAttributeTest, BasicSurfaceAttributeTest,
    ::testing::Values(surface_focus_test_parameters));
 
+INSTANTIATE_TEST_CASE_P(SurfaceParentAttributeTest, BasicSurfaceAttributeTest,
+   ::testing::Values(surface_parent_test_parameters));
+
 TEST_F(BasicSurfaceTest, configure_returns_value_set_by_configurator)
 {
     using namespace testing;

=== modified file 'tests/unit-tests/scene/test_surface_impl.cpp'
--- tests/unit-tests/scene/test_surface_impl.cpp	2014-10-01 06:25:56 +0000
+++ tests/unit-tests/scene/test_surface_impl.cpp	2014-11-06 09:26:44 +0000
@@ -327,6 +327,41 @@
     EXPECT_FLOAT_EQ(1.0f, surf.alpha());
 }
 
+TEST_F(Surface, remembers_parent)
+{
+    auto parent = std::make_shared<ms::BasicSurface>(
+        std::string("stub"),
+        geom::Rectangle{{},{}},
+        false,
+        buffer_stream,
+        std::shared_ptr<mi::InputChannel>(),
+        stub_input_sender,
+        null_configurator,
+        std::shared_ptr<mg::CursorImage>(),
+        report);
+
+    ms::BasicSurface child(
+        std::string("stub"),
+        geom::Rectangle{{},{}},
+        false,
+        buffer_stream,
+        std::shared_ptr<mi::InputChannel>(),
+        stub_input_sender,
+        null_configurator,
+        std::shared_ptr<mg::CursorImage>(),
+        report);
+
+    EXPECT_FALSE(child.parent());
+
+    child.set_parent(parent);
+    EXPECT_EQ(parent, child.parent());
+    EXPECT_TRUE(!!child.parent());
+
+    child.set_parent({});
+    EXPECT_NE(parent, child.parent());
+    EXPECT_FALSE(child.parent());
+}
+
 TEST_F(Surface, sends_focus_notifications_when_focus_gained_and_lost)
 {
     using namespace testing;