Mir

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

parenting-server-api
Merge into development-branch

Proposed by Daniel van Vugt on 2014-11-06

Status:

Rejected

Rejected by:

Cemil Azizoglu on 2014-11-20

Proposed branch:

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

Merge into:

lp:mir

Diff against target:

498 lines (+191/-10)

21 files modified

client-ABI-sha1sums (+1/-1)
common-ABI-sha1sums (+1/-1)
include/common/mir_toolkit/common.h (+6/-0)
include/server/mir/frontend/session.h (+2/-0)
include/server/mir/frontend/surface.h (+3/-0)
platform-ABI-sha1sums (+1/-1)
server-ABI-sha1sums (+3/-3)
src/server/frontend/session_mediator.cpp (+1/-2)
src/server/scene/application_session.cpp (+17/-0)
src/server/scene/application_session.h (+1/-0)
src/server/scene/basic_surface.cpp (+37/-0)
src/server/scene/basic_surface.h (+5/-0)
tests/include/mir_test_doubles/mock_frontend_surface.h (+3/-0)
tests/include/mir_test_doubles/mock_surface.h (+3/-0)
tests/include/mir_test_doubles/stub_scene_session.h (+4/-0)
tests/include/mir_test_doubles/stub_scene_surface.h (+2/-0)
tests/include/mir_test_doubles/stub_session.h (+4/-0)
tests/unit-tests/client/test_client_mir_surface.cpp (+2/-1)
tests/unit-tests/scene/test_application_session.cpp (+46/-0)
tests/unit-tests/scene/test_basic_surface.cpp (+14/-1)
tests/unit-tests/scene/test_surface_impl.cpp (+35/-0)

To merge this branch:

bzr merge lp:~vanvugt/mir/parenting-server-api

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Cemil Azizoglu (community)			Needs Resubmitting on 2014-11-18
Robert Carr (community)			Disapprove on 2014-11-17
PS Jenkins bot (community)	continuous-integration	2014-11-06	Approve on 2014-11-17
Alberto Aguirre (community)		2014-11-06	Abstain on 2014-11-13
Alan Griffiths		2014-11-06	Needs Resubmitting on 2014-11-11
Review via email: mp+240828@code.launchpad.net

This proposal supersedes a proposal from 2014-11-03.

Commit message

Introduce basic surface parent information in the server API.

Description of the change

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

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-04: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-04: Posted in a previous version of this proposal

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

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-05: Posted in a previous version of this proposal

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.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-05: Posted in a previous version of this proposal

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-05: Posted in a previous version of this proposal

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-11-05: Posted in a previous version of this proposal

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?

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-05: Posted in a previous version of this proposal

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

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-06: Posted in a previous version of this proposal

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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-06:

PASSED: Continuous integration, rev:2034
http://jenkins.qa.ubuntu.com/job/mir-ci/2023/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/12
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/12/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-06:

Looks good.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-07:

FAILED: Continuous integration, rev:2036
http://jenkins.qa.ubuntu.com/job/mir-ci/2034/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/30
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/9/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/23
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/23/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/9
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/9/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3303/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15538

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-07:

Failure is lp:1390388

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-09:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-10:

I'm not yet sure that this solves the right problem.

review: Abstain

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-10:

> I'm not yet sure that this solves the right problem.

Actually, I've decided that it is clear that deciding this needs resolution of the question about who "owns" the decision about strategy.

On the basis that there is a reasonable school of thought that it isn't owned by the client or frontend code then we should resolve that discussion before landing changes to frontend to support it.

*Needs discussion* at tomorrow's meeting.

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-11:

I completely agree, and have worked to remove as much from the frontend as possible, over multiple iterations.

The key problem is parent "ID" only exists in the frontend. And correlating that with a parent ptr in the frontend appears to be much cleaner than the alternative of duplicating ID information into the scene; BasicSurface (where it's meaningless, hence absent right now). Not only that but pushing all ID responsibility into the lower layer would also require additional lookup functionality get pushed down there. Very ugly. This is cleaner.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-11-11:

> I completely agree, and have worked to remove as much from the frontend as
> possible, over multiple iterations.
>
> The key problem is parent "ID" only exists in the frontend. And correlating
> that with a parent ptr in the frontend appears to be much cleaner than the
> alternative of duplicating ID information into the scene; BasicSurface (where
> it's meaningless, hence absent right now). Not only that but pushing all ID
> responsibility into the lower layer would also require additional lookup
> functionality get pushed down there. Very ugly. This is cleaner.

I agree that any mapping of ID to ptr belongs in frontend.

However, following Thomas's meeting it is clear that some rework is needed to fit the design philosophy as there will not be a (re)parenting request from the client for an existing surface. C.f.

https://lists.ubuntu.com/archives/mir-devel/2014-November/000961.html

review: Needs Resubmitting

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-12:

I disagree :)

The design you point to is artificially limited, and dramatically overcomplicated. It fails to support shells other than Unity without at least requiring ongoing maintenance and unbounded further expansion in complexity. It's not sensible at all, so I stand by this design.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-12:

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-12:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-11-13:

I guess abstain as another approach will be taken.

review: Abstain

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-14:

PASSED: Continuous integration, rev:2042
http://jenkins.qa.ubuntu.com/job/mir-ci/2103/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/123
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/123
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/92
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/92/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/103
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/103/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3390
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15823

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

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-11-14:

This one is also at a dead-end.

review: Disapprove

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-17:

It's the only working solution right now for an important feature.

I'll keep the review up till someone proposes a better solution (or lands a worse one :)

lp:~vanvugt/mir/parenting-server-api updated on 2014-11-17

2043. By Daniel van Vugt on 2014-11-17: Merge latest trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-11-17:

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

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-11-17:

Outcome is clear...registering disapprove to encourage Daniel to save time and not continue updating :)

review: Disapprove

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-18:

I've learned to never expect or assume any branch will land at all :)

Actually I'm surprised this branch is unpopular. Not mentioning anything about client API design here it would appear to support the alternative client APIs being proposed rather neatly. And most importantly, without any protocol modifications.

I've been thinking if this branch lands then I can at least unblock and carry on with the server side logic for relative positioning. Again without mentioning client APIs.

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-11-18:

The objection is that parent should not be an attribute, as was decided in Thomas' meeting. It should be set once and not be changed, which goes against the approach this MP is taking. Hence the Resubmit. This is my understanding of it (Others please jump in, if I'm missing something).

review: Needs Resubmitting

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-11-19:

I'm also wondering about type morphing, which design wants us to support... What if a surface morphs from type A to type B and only type B requires parenting?

In that case you'd have to reject a simple set_type() request and add more client functions that do an atomic set_type_B(surface, newparent). So there's the scalability issue still, and you do gain atomicity, but I'm not sure that atomicity is more important than the scalability advantage.

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2014-11-20:

> I'm also wondering about type morphing, which design wants us to support...
> What if a surface morphs from type A to type B and only type B requires
> parenting?
>
> In that case you'd have to reject a simple set_type() request and add more
> client functions that do an atomic set_type_B(surface, newparent). So there's
> the scalability issue still, and you do gain atomicity, but I'm not sure that
> atomicity is more important than the scalability advantage.

Yes the API that has been agreed upon with tvoss would require additional functions to handle this case. I don't think this is the correct venue if you have concerns about this approach. Talking to Thomas directly might be more effective.

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().

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Daniel van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'client-ABI-sha1sums'
 --- client-ABI-sha1sums	2014-11-12 05:01:53 +0000
 +++ client-ABI-sha1sums	2014-11-17 01:55:23 +0000
@@ -8,7 +8,7 @@
 f85e3d00314a7df869e56c3701a45310909fae2  include/client/mir_toolkit/mir_surface.h
  b141c4d79802ad626d969249c0004744e5c2a525  include/client/mir_toolkit/mir_wait.h
  9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
--2100c0674d9d882c1845550847357f6a5de5af66  include/common/mir_toolkit/common.h
++77a5955e53e1901766372b4fb29e34872ef62667  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-11 07:34:31 +0000
 +++ common-ABI-sha1sums	2014-11-17 01:55:23 +0000
@@ -11,7 +11,7 @@
  be7d58c9fde2ce91cc66dd6144b76e08b536266b  include/common/mir/int_wrapper.h
 ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
  9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
--2100c0674d9d882c1845550847357f6a5de5af66  include/common/mir_toolkit/common.h
++77a5955e53e1901766372b4fb29e34872ef62667  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-11-11 07:34:31 +0000
 +++ include/common/mir_toolkit/common.h	2014-11-17 01:55:23 +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;
@@ -90,6 +91,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-17 01:55:23 +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-17 01:55:23 +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-14 17:46:49 +0000
 +++ platform-ABI-sha1sums	2014-11-17 01:55:23 +0000
@@ -11,7 +11,7 @@
  be7d58c9fde2ce91cc66dd6144b76e08b536266b  include/common/mir/int_wrapper.h
 ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
  9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
--2100c0674d9d882c1845550847357f6a5de5af66  include/common/mir_toolkit/common.h
++77a5955e53e1901766372b4fb29e34872ef62667  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-14 17:46:49 +0000
 +++ server-ABI-sha1sums	2014-11-17 01:55:23 +0000
@@ -11,7 +11,7 @@
  be7d58c9fde2ce91cc66dd6144b76e08b536266b  include/common/mir/int_wrapper.h
 ae8473df05dd9e048a73797f01a2f34f7447554  include/common/mir/time/types.h
  9907751d046e4aea81881cf19e5df52c7a6a813e  include/common/mir_toolkit/client_types.h
--2100c0674d9d882c1845550847357f6a5de5af66  include/common/mir_toolkit/common.h
++77a5955e53e1901766372b4fb29e34872ef62667  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
 ce482df448fd2fc5f0c4ae5ac8b7fecbd228c9  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
 aa2298c4e2cdc1bbb7cb5f250a914ae16c9  include/server/mir/frontend/shell.h
--f67b9788943a4b9121b3891dfbe979d57922ea78  include/server/mir/frontend/surface.h
++d33a31128bcb6908ec769bfdb9897781748d2472  include/server/mir/frontend/surface.h
 b43a84cce0ad671ed68fe2ba796fbc7b79e31  include/server/mir/frontend/surface_id.h
  f95c2bddf13d15993ef5d6a0ad7b9106ae550b87  include/server/mir/input/composite_event_filter.h
 acb03b35d383dfefd65e8dfb872c42bcc11  include/server/mir/input/cursor_images.h
 === modified file 'src/server/frontend/session_mediator.cpp'
 --- src/server/frontend/session_mediator.cpp	2014-11-13 16:41:26 +0000
 +++ src/server/frontend/session_mediator.cpp	2014-11-17 01:55:23 +0000
@@ -359,8 +359,7 @@
          auto const id = mf::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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-17 01:55:23 +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-11-11 04:02:18 +0000
 +++ tests/unit-tests/scene/test_application_session.cpp	2014-11-17 01:55:23 +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;
++    NiceMock<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-17 01:55:23 +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-17 01:55:23 +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;