Merge lp:~vanvugt/mir/surface-types into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Add support for surface types (client API, protocol and server).

This work will also form the foundation of supporting surface states.

Fixed conflicts from nightly upstream landings. Also moved type out of surfaces::Surface and into shell::Surface.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:554
http://jenkins.qa.ubuntu.com/job/mir-ci/69/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/70//console

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't think that surfaces::Surface needs configure(MirSurfaceAttrib attrib, int value) - AFICS attributes are a feature of shell::Surface. (And, as discussed elsewhere, surfaces::ProxySurface belongs in shell, possibly as shell::Window.)

~~~~

90 +

Trailing whitespace.

~~~~

216 === added file 'include/shared/mir/common.h'

This is not the place for parts of the toolkit API - this is for namespace mir

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

Is there a clear distinction between SurfaceSetting and SurfaceParameters? (And is it worth supplying SurfaceSetting to create_surface and avoiding a round-trip?)

[Kevin DuBois (kdub)]

the new surface type get/set for client api looks ok to me

The acceptance test takes quite a while on the galaxy nexus, perhaps it can be shortened by using less iterations in line 757?
[ RUN ] DefaultDisplayServerTestFixture.surface_types
[ OK ] DefaultDisplayServerTestFixture.surface_types (1384 ms)
(i see about 180ms on my desktop)

small nit but with
615 +int ms::Surface::configure(MirSurfaceAttrib attrib, int value)
the name 'configure' didn't make it obvious what is going on in the function.

Also with this function, (related to the comment in it) is the surface type something that mir cares about, or the shell? This is tricky to talk about without a 'shell api', but its still a bit hazy to me what our requirements for tracking the data that the shell cares about.

although the tests look good, they do have multiple testing expectations in them because they are long.

[Daniel van Vugt (vanvugt)]

Alan,

Extraneous "configure" methods:
I only added methods where I absolutely needed to, to make the code compile and run. However that was many revisions ago so I'll check to see if it's still required...

Trailing whitespace at line 90:
Will fix now.

216 === added file 'include/shared/mir/common.h'
"common.h" is not part of the "toolkit". It is used by the client toolkit and in the server.

SurfaceSetting vs SurfaceParameters
Yes they will always be different. SurfaceParameters are required information to create a surface. SurfaceSetting is something you might optionally change later; like type, state, hidden, width, height (most of which are not implemented yet but require this branch to land first).

Kevin,

Stress test takes too long:
I think 1.3 seconds is OK for a stress test, but I can shorten it if you really want.

"configure" is not self explanatory:
I kind of agree but have not yet thought of a better method name other than "set_attribute" (making the protocol call something like "set_surface_attribute"). I prefer "configure_surface". More suggestions?

Who cares about surface type?...
You're absolutely right that surface type is something only the shell will care about and act upon. However for consistency between different shells and avoiding duplication, it is reasonable for the server to store the type value in each server surface instance. Even though it will only be used by the shell. This will change as new attributes are added like "state" which will potentially be used by the server directly.

Test length:
Yes, I will always make code as concise as possible without hurting readability.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Minor fixes done.

Alan -
I can put "common.h" anywhere of course. Just remember:
  1. It's not part of the "toolkit". "toolkit" means client-only but "common.h" is also used in the server and shell.
  2. common.h has to be installed under "include/mir/..." to make it clear to external users that it's part of mir. And it is implicitly in the "mir" namespace by virtue of the "Mir" type prefixes. We can't use "namespace mir" however because it is C code, not C++.

Alan & Kevin -
I agree that "type" is not a deep server-level attribute. It only makes sense at the shell level. I could move it out of surfaces::Surface now, and into a proxy class. However I think that should be done later because the "Proxy" classes clearly need some refactoring, everyone seems to agree. I'd rather not make assumptions about what form that will take and move any members into *ProxySurface until that's done.

Also keep in mind that _most_ future configurable attributes (state, hidden, width, height etc) will be low-level and live in the server surfaces::Surface rather than the shell.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> Minor fixes done.
>
> Alan -
> I can put "common.h" anywhere of course. Just remember:
> 1. It's not part of the "toolkit". "toolkit" means client-only but
> "common.h" is also used in the server and shell.
> 2. common.h has to be installed under "include/mir/..." to make it clear to
> external users that it's part of mir. And it is implicitly in the "mir"
> namespace by virtue of the "Mir" type prefixes. We can't use "namespace mir"
> however because it is C code, not C++.

As it is included by toolkit I think it is part of toolkit. If it were really "part of mir", then there would be no reason for it to be C code. (Is it part of some hypothetical "common" that we've just identified - if so, is "common.h" isn't a good name.)

> Alan & Kevin -
> I agree that "type" is not a deep server-level attribute. It only makes sense
> at the shell level. I could move it out of surfaces::Surface now, and into a
> proxy class. However I think that should be done later because the "Proxy"
> classes clearly need some refactoring, everyone seems to agree. I'd rather not
> make assumptions about what form that will take and move any members into
> *ProxySurface until that's done.

Perhaps it is best to wait on that refactoring and reassess after that?

> Also keep in mind that _most_ future configurable attributes (state, hidden,
> width, height etc) will be low-level and live in the server surfaces::Surface
> rather than the shell.

Are you envisaging using the same mechanism?

[Daniel van Vugt (vanvugt)]

Absolutely, all attributes that can be changed after construction will use configure() eventually.

Please consider landing this branch before any ProxySurface refactoring. I am tired of fixing conflicts and refactoring it every day.

[Daniel van Vugt (vanvugt)]

Also, these arguments are blocking me from starting on two other deliverables which will depend on this work.

[Alan Griffiths (alan-griffiths)]

> Absolutely, all attributes that can be changed after construction will use
> configure() eventually.

Does that imply that e.g. you see resizing happening by setting the width and height in two configure calls? I'm not sure that is a good plan.

Why wouldn't there be a single resize(graphics::Size) call?

> Please consider landing this branch before any ProxySurface refactoring. I am
> tired of fixing conflicts and refactoring it every day.

I'm equally tired of reviewing it every day - there has to be a better way to communicate.

[Daniel van Vugt (vanvugt)]

Good point. Width/height are bad examples. But I will reuse configure() for surface states soon.

[Daniel van Vugt (vanvugt)]

I tried moving common.h to: include/shared/mir_toolkit/common.h
But that fails for reasons alluded to earlier. That is such common headers need to have the same relative path in the source tree as in the install tree. Because mir_client_library.h has to be able to find common.h in both situations.

Any suggestions?

[Alan Griffiths (alan-griffiths)]

> I tried moving common.h to: include/shared/mir_toolkit/common.h
> But that fails for reasons alluded to earlier. That is such common headers
> need to have the same relative path in the source tree as in the install tree.
> Because mir_client_library.h has to be able to find common.h in both
> situations.
>
> Any suggestions?

I guess this comes as part of the various problems with directory names.

IMO include/shared/mir_toolkit and include/client/mir_toolkit should be installed to <target>/mir_toolkit - and that would facilitate #include <mir_toolkit/common.h>

[Kevin DuBois (kdub)]

>
> I guess this comes as part of the various problems with directory names.
>
> IMO include/shared/mir_toolkit and include/client/mir_toolkit should be
> installed to <target>/mir_toolkit - and that would facilitate #include
> <mir_toolkit/common.h>

+1

[Alexandros Frantzis (afrantzis)]

== include/shared/mir vs include/shared/mir_toolkit ==

The concept is that whatever is under */mir_toolkit (and only that) will be part of the toolkit interface. Since a toolkit/client needs this, mir_toolkit is the way to go. Keep in mind that this is still under shared/.

So, this is how I see things currently:

1. server/*, client/*, shared/* => which mir components need to access the definitions in the file
2. */mir, */mir_toolkit => whether the definitions are internal to mir, or (also) exposed to the toolkit API

== surfaces::Surface::set_type(),type() ==

I agree that this doesn't belong there. I don't have a problem putting it there *temporarily* to unblock work, if the related surface refactorings aren't expected to be done soon.

== configure() ==

At the Surface level, and for the limited number of attributes I am aware of, I would prefer to have explicit calls : app_session->set_surface_state(id, ...) or app_session->surface(id)->set_state(...) (since we are already exposing the surface) etc

At the RPC level: one approach is to use configure(attrib, value), another is to have <attrib>(value) calls. A third approach (not neccessarily preferable for this use case, just mentioning it for completeness) is to have, e.g.:

message SurfaceSettings {
    optional SurfaceId surfaceid = 1;
    optional int32 type = 2;
    optional int32 state = 3;
    optional bool visible = 4;
    ...
}

The frontend could then act depending on which values are actually included in the message. This method allows the client to set and retrieve all of the settings in one go.

For the number of attributes and the use cases I am aware of, my preference would be once again to be explicit, and have separate calls <attrib>().

Marking as "Needs information" => "Needs discussion"

review: Needs Information

[Daniel van Vugt (vanvugt)]

I thought it was implicit that having two different top-level include directories:
  <mir/foo>
  <mir_toolkit/bar>
was obviously a bad idea. I thought that was obvious but clearly it's just my opinion now. I think eventually everything should be under "mir/" for consistency:
  <mir/foo>
  <mir/toolkit/bar>
But that's a cleanup for a another day and a different proposal.

Alexandros:
configure() needs to be generic because there are so many layers of classes in the Mir code, it does not scale to add a new function for every attribute at every layer. When I added configure() I was surprised at how many classes I had to modify. It becomes unmaintainable and excessively over-complicated if you have to do that again and again for each new attribute. Consider also reporting -- we don't want to change every type of report to add new methods to those every time a new attribute is added.
Also, we should not assume to have designed all the required attributes any time soon. New attributes will be added and some even removed over time. We don't want to go changing the protocol and half a dozen foundational classes each time.

[Daniel van Vugt (vanvugt)]

I've now been working on this branch for a month and it's been under review for two weeks. It has worked perfectly without bugs for the entire duration. Remember we're only arguing about style issues that can be fixed later.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

8 +Package: mircommon-dev

I don't think this is right as it stands - but that is probably part of a pre-existing problem with the install packages and directory structure.

We need to be able to install the client process toolkit headers. (shared/mir_toolkit/* + client/mir_toolkit/*)

We need to be able to install the server process toolkit headers. (shared/mir_toolkit/* + server/mir_toolkit/*)

We need to be able to install for building against the library headers. (shared/mir/* + server/mir/* + shared/mir_toolkit/common.h)

I don't have a better proposal to hand, but something needs fixing.

~~~~

As I've said before I don't think that surfaces::Surface needs configure(MirSurfaceAttrib attrib, int value) - AFICS attributes are a feature of shell::Surface.

But I guess we can delete it a few minutes after it lands.

review: Abstain

[Robert Ancell (robert-ancell)]

I'm unsure about why we have mircommon, but that can be solved outside of this.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/58/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/26/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-autolanding/59/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

I might be able to install "common.h" twice; for client and server. Thus eliminating the mircommon-dev package. If you want me to give it a go?.. But there's obviously redundancy doing that.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:565
http://jenkins.qa.ubuntu.com/job/mir-ci/124/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/29/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/125/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/125/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I think we need a discussion about the different user code targets we want to support and organizing headers to support them. But this MP isn't the place for it.

I'd still prefer not putting attributes that belong on shell::Surface onto surface::Surface.

[Daniel van Vugt (vanvugt)]

Work in progress again. As happens every day, there are conflicts from upstream I need to resolve.

I will also re-visit the shell::Surface discussion now that shell::Surface actually exists.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:573
http://jenkins.qa.ubuntu.com/job/mir-ci/139/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/51/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/140/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/140/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I'm not sure if these are consistent:

316 + // I assume the type can only change from the client side. Otherwise
317 + // we would have to send off a message to retrieve the latest...
...
619 + bool set_type(MirSurfaceType t);

This is a public member function - that suggest that it could, potentially, be called other than as the result of a call from a client process.

Also, given the expectation of using a graphics toolkit within the server process is "client side" the right terminology?

[Alan Griffiths (alan-griffiths)]

820 + EXPECT_EQ(surf.configure(mir_surface_attrib_type,
821 + mir_surface_type_popover),
822 + mir_surface_type_popover);

Expected and actual reversed

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alan's concerns addressed.

And if you want to be that pedantic about gtest syntax, see bug 1149157.

[Robert Ancell (robert-ancell)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:578
http://jenkins.qa.ubuntu.com/job/mir-ci/151/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/66
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/152
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/152/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> Alan's concerns addressed.
>
> And if you want to be that pedantic about gtest syntax, see bug 1149157.

My "concern" is about getting clear output from a test failure.

If I wanted to be pedantic I'd "Need Fixing" the tests that don't test a single thing described by the test name.

review: Approve

[Robert Carr (robertcarr)]

Looks good! excited to get surface attributes in.

Some minor style nits:

>>+/**
>>+ * Set the type (purpose) of a surface. This is not guaranteed to always work
>>+ * with some shell types (e.g. phone/tablet UIs). As such, you may have to
>>+ * wait on the function and check the result using mir_surface_get_type.
>>+ * \param [in] surface The surface to operate on
>>+ * \param [in] type The new type of the surface
>>+ * \return A wait handle that can be passed to mir_wait_for
>>+ */

Only nitting here because it's public header. I found this a little confusing to read (i.e. mention of phone/tablet is a little distracting). I would write it like this:

"Set the type (role) of a surface. Success of this operation is dependent on server policy. As such, you may have to wait on the function and check the result using mir_suface_get_type"

>>+ mir_surface_type_arraysize_
Do we do this elsewhere (trailing underscore), should we?

>> 427:
I find the continuous mention in comments and the variable attrib_cache of "cache" confusing. In this sense of the word cache, mir_toolkit:MirSurface itself is a cache of the latest Surface state returned from the server. Reading cache here makes me think there is some extra caching behavior going on, whereas really it's no different than surface geometry!

[Robert Carr (robertcarr)]

>> 477:
I can't find the test for this

review: Needs Information

[Robert Carr (robertcarr)]

>> 305:
Whitespace

>> 821:

This would be more in line with the rest of the code style if there were a few tests:

TEST_F(ShellSurface, default_surface_type_is_normal)
TEST_F(ShellSurface, takes_surface_type_from_configure)
TEST_F(ShellSurface, invalid_surface_type_throw_behavior)

This would improve the failure output significantly I think

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

racarr,

comment confusion: Yeah easy to fix, later.

mir_surface_type_arraysize_: This is similar to a convention I have used for a long time. There is no precedent in the mir source AFAIK. It is adapted from the original version I proposed 3 weeks ago: "MirSurfaceType_ARRAYSIZE" which is the style Google uses in protobuf.

427: Comments can be clarified. But not worth a reproposal.

477: The test for the whole configure_surface stack is covered by:
TEST_F(DefaultDisplayServerTestFixture, surface_types)

305: That indentation is no mistake. I moved it to stay within 80 columns.

821: I am adjusting my style to suit you guys. It won't happen over night but it is happening. I don't think this is a blocker.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'debian/control'
--- debian/control	2013-03-12 03:50:42 +0000
+++ debian/control	2013-03-21 04:05:24 +0000
@@ -58,11 +58,22 @@
  .
  Contains the shared library needed by server applications for Mir.
 
+Package: mircommon-dev
+Section: libdevel
+Architecture: any
+Depends: ${misc:Depends},
+Description: Display server for Ubuntu - development headers
+ Mir is a display server running on linux systems, with a focus on efficiency, 
+ robust operation and a well-defined driver model.
+ .
+ Contains header files required for server and/or client development.
+
 Package: libmirserver-dev
 Section: libdevel
 Architecture: any
 Depends: libmirserver0 (= ${binary:Version}),
          libmirprotobuf-dev (= ${source:Version}),
+         mircommon-dev (= ${source:Version}),
          ${misc:Depends},
 Description: Display server for Ubuntu - development headers
  Mir is a display server running on linux systems, with a focus on efficiency,
@@ -86,6 +97,7 @@
 Architecture: any
 Depends: libmirclient0 (= ${binary:Version}),
          libmirprotobuf-dev (= ${source:Version}),
+         mircommon-dev (= ${source:Version}),
          ${misc:Depends},
 Description: Display server for Ubuntu - development headers
  Mir is a display server running on linux systems, with a focus on efficiency,

=== added file 'debian/mircommon-dev.install'
--- debian/mircommon-dev.install	1970-01-01 00:00:00 +0000
+++ debian/mircommon-dev.install	2013-03-21 04:05:24 +0000
@@ -0,0 +1,1 @@
+usr/include/mir_toolkit/*.h

=== modified file 'include/client/mir_toolkit/mir_client_library.h'
--- include/client/mir_toolkit/mir_client_library.h	2013-03-20 10:26:48 +0000
+++ include/client/mir_toolkit/mir_client_library.h	2013-03-21 04:05:24 +0000
@@ -18,6 +18,8 @@
 #ifndef MIR_CLIENT_LIBRARY_H
 #define MIR_CLIENT_LIBRARY_H
 
+#include <mir_toolkit/common.h>
+
 #ifdef __cplusplus
 /** The C client API
  */
@@ -358,6 +360,23 @@
  */
 int mir_debug_surface_id(MirSurface *surface);
 
+/**
+ * Set the type (purpose) of a surface. This is not guaranteed to always work
+ * with some shell types (e.g. phone/tablet UIs). As such, you may have to
+ * wait on the function and check the result using mir_surface_get_type.
+ *   \param [in] surface  The surface to operate on
+ *   \param [in] type     The new type of the surface
+ *   \return              A wait handle that can be passed to mir_wait_for
+ */
+MirWaitHandle* mir_surface_set_type(MirSurface *surface, MirSurfaceType type);
+
+/**
+ * Get the type (purpose) of a surface.
+ *   \param [in] surface  The surface to query
+ *   \return              The type of the surface
+ */
+MirSurfaceType mir_surface_get_type(MirSurface *surface);
+
 #ifdef __cplusplus
 }
 }

=== modified file 'include/server/mir/frontend/session.h'
--- include/server/mir/frontend/session.h	2013-03-19 03:05:30 +0000
+++ include/server/mir/frontend/session.h	2013-03-21 04:05:24 +0000
@@ -19,6 +19,7 @@
 #ifndef MIR_FRONTEND_SESSION_H_
 #define MIR_FRONTEND_SESSION_H_
 
+#include "mir_toolkit/common.h"
 #include "mir/frontend/surface_id.h"
 
 #include <mutex>
@@ -49,6 +50,8 @@
     virtual void hide() = 0;
     virtual void show() = 0;
 
+    virtual int configure_surface(SurfaceId id, MirSurfaceAttrib attrib, int value) = 0;
+
 protected:
     Session() = default;
     Session(Session const&) = delete;

=== modified file 'include/server/mir/frontend/session_mediator.h'
--- include/server/mir/frontend/session_mediator.h	2013-03-19 03:05:30 +0000
+++ include/server/mir/frontend/session_mediator.h	2013-03-21 04:05:24 +0000
@@ -100,6 +100,11 @@
                         mir::protobuf::DRMAuthMagicStatus* response,
                         google::protobuf::Closure* done);
 
+    void configure_surface(google::protobuf::RpcController* controller,
+                           const mir::protobuf::SurfaceSetting*,
+                           mir::protobuf::SurfaceSetting*,
+                           google::protobuf::Closure* done);
+
 private:
     std::shared_ptr<Shell> const shell;
     std::shared_ptr<graphics::Platform> const graphics_platform;

=== modified file 'include/server/mir/frontend/session_mediator_report.h'
--- include/server/mir/frontend/session_mediator_report.h	2013-03-19 03:05:30 +0000
+++ include/server/mir/frontend/session_mediator_report.h	2013-03-21 04:05:24 +0000
@@ -42,6 +42,8 @@
 
     virtual void session_drm_auth_magic_called(std::string const& app_name) = 0;
 
+    virtual void session_configure_surface_called(std::string const& app_name) = 0;
+
     virtual void session_error(
         std::string const& app_name,
         char const* method,
@@ -63,6 +65,8 @@
 
     virtual void session_drm_auth_magic_called(std::string const& app_name);
 
+    virtual void session_configure_surface_called(std::string const& app_name);
+
     virtual void session_error(
         std::string const& app_name,
         char const* method,

=== modified file 'include/server/mir/frontend/surface.h'
--- include/server/mir/frontend/surface.h	2013-03-20 10:56:07 +0000
+++ include/server/mir/frontend/surface.h	2013-03-21 04:05:24 +0000
@@ -23,6 +23,7 @@
 #include "mir/geometry/pixel_format.h"
 #include "mir/geometry/point.h"
 #include "mir/geometry/size.h"
+#include "mir_toolkit/common.h"
 
 #include <memory>
 
@@ -60,6 +61,8 @@
     virtual bool supports_input() const = 0;
     virtual int client_input_fd() const = 0;
 
+    virtual int configure(MirSurfaceAttrib attrib, int value) = 0;
+
 protected:
     Surface() = default;
     Surface(Surface const&) = delete;

=== modified file 'include/server/mir/logging/session_mediator_report.h'
--- include/server/mir/logging/session_mediator_report.h	2013-03-19 03:05:30 +0000
+++ include/server/mir/logging/session_mediator_report.h	2013-03-21 04:05:24 +0000
@@ -47,6 +47,8 @@
 
     virtual void session_drm_auth_magic_called(std::string const& app_name);
 
+    virtual void session_configure_surface_called(std::string const& app_name);
+
     virtual void session_error(
         std::string const& app_name,
         char const* method,

=== modified file 'include/server/mir/shell/application_session.h'
--- include/server/mir/shell/application_session.h	2013-03-20 10:56:07 +0000
+++ include/server/mir/shell/application_session.h	2013-03-21 04:05:24 +0000
@@ -47,6 +47,8 @@
     void hide();
     void show();
 
+    int configure_surface(frontend::SurfaceId id, MirSurfaceAttrib attrib, int value);
+
 protected:
     ApplicationSession(ApplicationSession const&) = delete;
     ApplicationSession& operator=(ApplicationSession const&) = delete;

=== added file 'include/shared/mir_toolkit/common.h'
--- include/shared/mir_toolkit/common.h	1970-01-01 00:00:00 +0000
+++ include/shared/mir_toolkit/common.h	2013-03-21 04:05:24 +0000
@@ -0,0 +1,49 @@
+/*
+ * Simple definitions common to client and server.
+ *
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Daniel van Vugt <[e-mail]>
+ */
+
+#ifndef MIR_COMMON_H_
+#define MIR_COMMON_H_
+
+/* This is C code. Not C++. */
+
+/*
+ * Attributes of a surface that the client and server/shell may wish to
+ * get or set over the wire.
+ */
+typedef enum MirSurfaceAttrib
+{
+    mir_surface_attrib_type,
+    mir_surface_attrib_arraysize_
+} MirSurfaceAttrib;
+
+typedef enum MirSurfaceType
+{
+    mir_surface_type_normal,
+    mir_surface_type_utility,
+    mir_surface_type_dialog,
+    mir_surface_type_overlay,
+    mir_surface_type_freestyle,
+    mir_surface_type_popover,
+    mir_surface_type_arraysize_
+} MirSurfaceType;
+
+/* TODO: Surface states here */
+
+#endif

=== modified file 'include/test/mir_test_doubles/mock_session.h'
--- include/test/mir_test_doubles/mock_session.h	2013-03-19 03:05:30 +0000
+++ include/test/mir_test_doubles/mock_session.h	2013-03-21 04:05:24 +0000
@@ -41,6 +41,8 @@
     
     MOCK_METHOD0(hide, void());
     MOCK_METHOD0(show, void());
+
+    MOCK_METHOD3(configure_surface, int(frontend::SurfaceId, MirSurfaceAttrib, int));
 };
 
 }

=== modified file 'include/test/mir_test_doubles/mock_surface.h'
--- include/test/mir_test_doubles/mock_surface.h	2013-03-20 10:56:07 +0000
+++ include/test/mir_test_doubles/mock_surface.h	2013-03-21 04:05:24 +0000
@@ -49,6 +49,8 @@
     
     MOCK_CONST_METHOD0(supports_input, bool());
     MOCK_CONST_METHOD0(client_input_fd, int());
+
+    MOCK_METHOD2(configure, int(MirSurfaceAttrib, int));
 };
 
 }

=== modified file 'include/test/mir_test_doubles/stub_session.h'
--- include/test/mir_test_doubles/stub_session.h	2013-03-19 03:05:30 +0000
+++ include/test/mir_test_doubles/stub_session.h	2013-03-21 04:05:24 +0000
@@ -54,6 +54,10 @@
     void show()
     {
     }
+    int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int)
+    {
+        return 0;
+    }
 };
 
 }

=== modified file 'include/test/mir_test_doubles/stub_surface.h'
--- include/test/mir_test_doubles/stub_surface.h	2013-03-18 15:43:58 +0000
+++ include/test/mir_test_doubles/stub_surface.h	2013-03-21 04:05:24 +0000
@@ -53,6 +53,11 @@
     {
         return std::shared_ptr<compositor::Buffer>();
     }
+
+    virtual int configure(MirSurfaceAttrib, int)
+    {
+        return 0;
+    }
     
     virtual bool supports_input() const
     {

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-03-20 05:11:57 +0000
+++ src/client/mir_client_library.cpp	2013-03-21 04:05:24 +0000
@@ -272,3 +272,25 @@
 {
     // Ignore
 }
+
+mir_toolkit::MirWaitHandle* mir_toolkit::mir_surface_set_type(MirSurface *surf,
+                                                           MirSurfaceType type)
+{
+    return surf ? surf->configure(mir_surface_attrib_type, type) : NULL;
+}
+
+MirSurfaceType mir_toolkit::mir_surface_get_type(MirSurface *surf)
+{
+    MirSurfaceType type = mir_surface_type_normal;
+
+    if (surf)
+    {
+        // Only the client will ever change the type of a surface so it is
+        // safe to get the type from a local cache surf->attrib().
+
+        int t = surf->attrib(mir_surface_attrib_type);
+        type = static_cast<MirSurfaceType>(t);
+    }
+
+    return type;
+}

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2013-03-07 08:04:05 +0000
+++ src/client/mir_surface.cpp	2013-03-21 04:05:24 +0000
@@ -50,6 +50,10 @@
     message.set_buffer_usage(params.buffer_usage);
 
     server.create_surface(0, &message, &surface, gp::NewCallback(this, &MirSurface::created, callback, context));
+
+    for (int i = 0; i < mir_surface_attrib_arraysize_; i++)
+        attrib_cache[i] = -1;
+    attrib_cache[mir_surface_attrib_type] = mir_surface_type_normal;
 }
 
 mir_toolkit::MirSurface::~MirSurface()
@@ -226,3 +230,46 @@
 {
     return *accelerated_window;
 }
+
+MirWaitHandle* MirSurface::configure(MirSurfaceAttrib at, int value)
+{
+    mp::SurfaceSetting setting;
+    setting.mutable_surfaceid()->CopyFrom(surface.id());
+    setting.set_attrib(at);
+    setting.set_ivalue(value);
+
+    configure_wait_handle.expect_result();
+    server.configure_surface(0, &setting, &configure_result, 
+              google::protobuf::NewCallback(this, &MirSurface::on_configured));
+
+    return &configure_wait_handle;
+}
+
+void MirSurface::on_configured()
+{
+    if (configure_result.has_surfaceid() &&
+        configure_result.surfaceid().value() == surface.id().value() &&
+        configure_result.has_attrib())
+    {
+        switch (configure_result.attrib())
+        {
+        case mir_surface_attrib_type:
+            if (configure_result.has_ivalue())
+            {
+                int t = configure_result.ivalue();
+                attrib_cache[mir_surface_attrib_type] = t;
+            } // else error is probably set due to an unsupported attrib/value
+            break;
+        default:
+            assert(false);
+            break;
+        }
+
+        configure_wait_handle.result_received();
+    }
+}
+
+int MirSurface::attrib(MirSurfaceAttrib at) const
+{
+    return attrib_cache[at];
+}

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2013-03-15 07:40:59 +0000
+++ src/client/mir_surface.h	2013-03-21 04:05:24 +0000
@@ -23,6 +23,7 @@
 #include "mir/geometry/pixel_format.h"
 #include "mir/geometry/dimensions.h"
 #include "mir_toolkit/mir_client_library.h"
+#include "mir_toolkit/common.h"
 #include "client_buffer_depository.h"
 #include "mir_wait_handle.h"
 #include "mir_client_surface.h"
@@ -72,7 +73,11 @@
     void release_cpu_region();
     EGLNativeWindowType generate_native_window();
 
+    MirWaitHandle* configure(MirSurfaceAttrib a, int value);
+    int attrib(MirSurfaceAttrib a) const;
+
 private:
+    void on_configured();
     void process_incoming_buffer();
     void populate(MirBufferPackage& buffer_package);
     void created(mir_surface_lifecycle_callback callback, void * context);
@@ -88,6 +93,7 @@
     MirConnection *connection;
     MirWaitHandle create_wait_handle;
     MirWaitHandle next_buffer_wait_handle;
+    MirWaitHandle configure_wait_handle;
 
     int last_buffer_id;
 
@@ -96,6 +102,11 @@
 
     std::shared_ptr<mir::client::Logger> logger;
     std::shared_ptr<EGLNativeWindowType> accelerated_window;
+
+    mir::protobuf::SurfaceSetting configure_result;
+
+    // Cache of latest SurfaceSettings returned from the server
+    int attrib_cache[mir_surface_attrib_arraysize_];
 };
 
 #endif /* MIR_CLIENT_PRIVATE_MIR_WAIT_HANDLE_H_ */

=== modified file 'src/server/frontend/null_session_mediator_report.cpp'
--- src/server/frontend/null_session_mediator_report.cpp	2013-03-19 03:05:30 +0000
+++ src/server/frontend/null_session_mediator_report.cpp	2013-03-21 04:05:24 +0000
@@ -42,6 +42,10 @@
 {
 }
 
+void mir::frontend::NullSessionMediatorReport::session_configure_surface_called(std::string const&)
+{
+}
+
 void mir::frontend::NullSessionMediatorReport::session_error(
         std::string const&,
         char const* ,

=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2013-03-07 08:04:05 +0000
+++ src/server/frontend/protobuf_message_processor.cpp	2013-03-21 04:05:24 +0000
@@ -176,6 +176,10 @@
         {
             invoke(&protobuf::DisplayServer::select_focus_by_lightdm_id, invocation);
         }
+        else if ("configure_surface" == invocation.method_name())
+        {
+            invoke(&protobuf::DisplayServer::configure_surface, invocation);
+        }
         else if ("disconnect" == invocation.method_name())
         {
             invoke(&protobuf::DisplayServer::disconnect, invocation);

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-03-20 05:11:57 +0000
+++ src/server/frontend/session_mediator.cpp	2013-03-21 04:05:24 +0000
@@ -23,6 +23,7 @@
 #include "mir/frontend/surface.h"
 #include "mir/frontend/surface_creation_parameters.h"
 #include "mir/frontend/resource_cache.h"
+#include "mir_toolkit/common.h"
 #include "mir/compositor/buffer_ipc_package.h"
 #include "mir/compositor/buffer_id.h"
 #include "mir/compositor/buffer.h"
@@ -220,3 +221,29 @@
     done->Run();
 }
 
+void mir::frontend::SessionMediator::configure_surface(
+    google::protobuf::RpcController*, // controller,
+    const mir::protobuf::SurfaceSetting* request,
+    mir::protobuf::SurfaceSetting* response,
+    google::protobuf::Closure* done)
+{
+    MirSurfaceAttrib attrib = static_cast<MirSurfaceAttrib>(request->attrib());
+
+    // Required response fields:
+    response->mutable_surfaceid()->CopyFrom(request->surfaceid());
+    response->set_attrib(attrib);
+
+    if (session.get() == nullptr)
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+    report->session_configure_surface_called(session->name());
+
+    auto const id = frontend::SurfaceId(request->surfaceid().value());
+    int value = request->ivalue();
+    int newvalue = session->configure_surface(id, attrib, value);
+
+    response->set_ivalue(newvalue);
+
+    done->Run();
+}
+

=== modified file 'src/server/logging/session_mediator_report.cpp'
--- src/server/logging/session_mediator_report.cpp	2013-03-19 03:05:30 +0000
+++ src/server/logging/session_mediator_report.cpp	2013-03-21 04:05:24 +0000
@@ -63,6 +63,11 @@
     log->log<Logger::informational>("session_drm_auth_magic_called(\"" + app_name + "\")", component);
 }
 
+void ml::SessionMediatorReport::session_configure_surface_called(std::string const& app_name)
+{
+    log->log<Logger::informational>("session_configure_surface_called(\"" + app_name + "\")", component);
+}
+
 void ml::SessionMediatorReport::session_error(
         std::string const& app_name,
         char const* method,

=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-03-20 10:56:07 +0000
+++ src/server/shell/application_session.cpp	2013-03-21 04:05:24 +0000
@@ -122,3 +122,13 @@
         id_s.second->show();
     }
 }
+
+int msh::ApplicationSession::configure_surface(mf::SurfaceId id,
+                                               MirSurfaceAttrib attrib,
+                                               int value)
+{
+    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::shared_ptr<mf::Surface> surf(checked_find(id)->second);
+
+    return surf->configure(attrib, value);
+}

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-03-19 03:05:30 +0000
+++ src/server/shell/surface.cpp	2013-03-21 04:05:24 +0000
@@ -35,7 +35,8 @@
     std::shared_ptr<input::InputChannel> const& input_channel)
   : builder(builder),
     input_channel(input_channel),
-    surface(builder->create_surface(params))
+    surface(builder->create_surface(params)),
+    type_value(mir_surface_type_normal)
 {
 }
 
@@ -133,3 +134,46 @@
         BOOST_THROW_EXCEPTION(std::logic_error("Surface does not support input"));
     return input_channel->client_fd();
 }
+
+int msh::Surface::configure(MirSurfaceAttrib attrib, int value)
+{
+    int result = 0;
+
+    /*
+     * TODO: In future, query the shell implementation for the subset of
+     *       attributes/types it implements.
+     */
+    switch (attrib)
+    {
+    case mir_surface_attrib_type:
+        if (!set_type(static_cast<MirSurfaceType>(value)))
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
+                                                   "type."));
+        result = type();
+        break;
+    default:
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
+                                               "attribute."));
+        break;
+    }
+
+    return result;
+}
+
+MirSurfaceType msh::Surface::type() const
+{
+    return type_value;
+}
+
+bool msh::Surface::set_type(MirSurfaceType t)
+{
+    bool valid = false;
+
+    if (t >= 0 && t < mir_surface_type_arraysize_)
+    {
+        type_value = t;
+        valid = true;
+    }
+
+    return valid;
+}

=== modified file 'src/server/shell/surface.h'
--- src/server/shell/surface.h	2013-03-20 10:56:07 +0000
+++ src/server/shell/surface.h	2013-03-21 04:05:24 +0000
@@ -22,6 +22,7 @@
 
 #include "mir/frontend/surface.h"
 #include "mir/surfaces/surface.h"
+#include "mir_toolkit/common.h"
 
 namespace mir
 {
@@ -66,10 +67,17 @@
     virtual bool supports_input() const;
     virtual int client_input_fd() const;
 
+    virtual int configure(MirSurfaceAttrib attrib, int value);
+    virtual MirSurfaceType type() const;
+
 private:
+    bool set_type(MirSurfaceType t);  // Use configure() to make public changes
+
     std::shared_ptr<SurfaceBuilder> const builder;
     std::shared_ptr<mir::input::InputChannel> const input_channel;
     std::weak_ptr<mir::surfaces::Surface> const surface;
+
+    MirSurfaceType type_value;
 };
 }
 }

=== modified file 'src/shared/CMakeLists.txt'
--- src/shared/CMakeLists.txt	2013-03-13 17:54:59 +0000
+++ src/shared/CMakeLists.txt	2013-03-21 04:05:24 +0000
@@ -5,3 +5,13 @@
   MIR_GENERATED_INCLUDE_DIRECTORIES
   ${MIR_GENERATED_INCLUDE_DIRECTORIES}
   PARENT_SCOPE)
+
+# Some headers contain common definitions required for both client and
+# server development...
+set (MIR_COMMON_HEADERS
+  ${CMAKE_SOURCE_DIR}/include/shared/mir_toolkit/common.h
+)
+
+# TODO: Find a more elegant include/ structure. Ideally one in which everything
+#       lives under "include/mir/".
+install (FILES ${MIR_COMMON_HEADERS} DESTINATION "include/mir_toolkit/")

=== modified file 'src/shared/protobuf/mir_protobuf.proto'
--- src/shared/protobuf/mir_protobuf.proto	2013-03-19 03:05:30 +0000
+++ src/shared/protobuf/mir_protobuf.proto	2013-03-21 04:05:24 +0000
@@ -114,6 +114,14 @@
   optional string error = 127;
 }
 
+message SurfaceSetting {
+  optional SurfaceId surfaceid = 1;
+  optional int32     attrib = 2;
+  optional int32     ivalue = 3;
+  // optional string    svalue = 4;  // Expected for future use
+  optional string error = 127;
+}
+
 service DisplayServer {
   // Platform independent requests
   rpc connect(ConnectParameters) returns (Connection);
@@ -127,5 +135,7 @@
   rpc drm_auth_magic(DRMMagic) returns (DRMAuthMagicStatus);
 
   rpc test_file_descriptors(Void) returns (Buffer);
+
+  rpc configure_surface(SurfaceSetting) returns (SurfaceSetting);
 }
 

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-03-20 05:11:57 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-03-21 04:05:24 +0000
@@ -205,6 +205,80 @@
     launch_client_process(client_config);
 }
 
+TEST_F(DefaultDisplayServerTestFixture, surface_types)
+{
+    struct ClientConfig : ClientConfigCommon
+    {
+        void exec()
+        {
+
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+
+            ASSERT_TRUE(connection != NULL);
+            EXPECT_TRUE(mir_connection_is_valid(connection));
+            EXPECT_STREQ(mir_connection_get_error_message(connection), "");
+
+            MirSurfaceParameters const request_params =
+            {
+                __PRETTY_FUNCTION__,
+                640, 480,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_hardware
+            };
+
+            mir_wait_for(mir_surface_create(connection, &request_params, create_surface_callback, this));
+
+            ASSERT_TRUE(surface != NULL);
+            EXPECT_TRUE(mir_surface_is_valid(surface));
+            EXPECT_STREQ(mir_surface_get_error_message(surface), "");
+
+            EXPECT_EQ(mir_surface_type_normal,
+                      mir_surface_get_type(surface));
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                              mir_surface_type_freestyle));
+            EXPECT_EQ(mir_surface_type_freestyle,
+                      mir_surface_get_type(surface));
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                            static_cast<MirSurfaceType>(999)));
+            EXPECT_EQ(mir_surface_type_freestyle,
+                      mir_surface_get_type(surface));
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                              mir_surface_type_dialog));
+            EXPECT_EQ(mir_surface_type_dialog,
+                      mir_surface_get_type(surface));
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                            static_cast<MirSurfaceType>(888)));
+            EXPECT_EQ(mir_surface_type_dialog,
+                      mir_surface_get_type(surface));
+
+            // Stress-test synchronization logic with some flooding
+            for (int i = 0; i < 100; i++)
+            {
+                mir_surface_set_type(surface, mir_surface_type_normal);
+                mir_surface_set_type(surface, mir_surface_type_utility);
+                mir_surface_set_type(surface, mir_surface_type_dialog);
+                mir_surface_set_type(surface, mir_surface_type_overlay);
+                mir_surface_set_type(surface, mir_surface_type_freestyle);
+                mir_wait_for(mir_surface_set_type(surface,
+                                                  mir_surface_type_popover));
+                ASSERT_EQ(mir_surface_type_popover,
+                          mir_surface_get_type(surface));
+            }
+
+            mir_wait_for(mir_surface_release(surface, release_surface_callback,
+                                             this));
+
+            mir_connection_release(connection);
+        }
+    } client_config;
+
+    launch_client_process(client_config);
+}
+
 TEST_F(DefaultDisplayServerTestFixture, client_library_creates_multiple_surfaces)
 {
     int const n_surfaces = 13;

=== modified file 'tests/integration-tests/cucumber/test_session_management_context.cpp'
--- tests/integration-tests/cucumber/test_session_management_context.cpp	2013-03-20 05:11:57 +0000
+++ tests/integration-tests/cucumber/test_session_management_context.cpp	2013-03-21 04:05:24 +0000
@@ -64,6 +64,8 @@
     MOCK_CONST_METHOD0(pixel_format, mir::geometry::PixelFormat ());
     MOCK_CONST_METHOD0(client_buffer, std::shared_ptr<mc::Buffer> ());
 
+    MOCK_METHOD2(configure, int(MirSurfaceAttrib, int));
+
     MOCK_CONST_METHOD0(supports_input, bool());
     MOCK_CONST_METHOD0(client_input_fd, int());
 };

=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-07 08:04:05 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-21 04:05:24 +0000
@@ -376,3 +376,23 @@
     EXPECT_EQ(pf, geom::PixelFormat::abgr_8888);
 }
 
+TEST_F(MirClientSurfaceTest, default_surface_type)
+{
+    using namespace testing;
+    using namespace mir::protobuf;
+
+    EXPECT_CALL(*mock_depository, deposit_package_rv(_,_,_,_))
+        .Times(1);
+
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_depository,
+                                                 params,
+                                                 &empty_callback,
+                                                 (void*) NULL);
+    surface->get_create_wait_handle()->wait_for_result();
+
+    EXPECT_EQ(mir_surface_type_normal,
+              surface->attrib(mir_surface_attrib_type));
+}

=== modified file 'tests/unit-tests/shell/test_surface.cpp'
--- tests/unit-tests/shell/test_surface.cpp	2013-03-20 05:11:57 +0000
+++ tests/unit-tests/shell/test_surface.cpp	2013-03-21 04:05:24 +0000
@@ -307,3 +307,51 @@
     }, std::logic_error);
 }
 
+TEST_F(ShellSurface, attributes)
+{
+    using namespace testing;
+
+    msh::Surface surf(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
+
+    EXPECT_THROW({
+        surf.configure(static_cast<MirSurfaceAttrib>(111), 222);
+    }, std::logic_error);
+}
+
+TEST_F(ShellSurface, types)
+{
+    using namespace testing;
+
+    msh::Surface surf(
+            mt::fake_shared(surface_builder),
+            mf::a_surface(),
+            null_input_channel);
+
+    EXPECT_EQ(mir_surface_type_normal, surf.type());
+
+    EXPECT_EQ(mir_surface_type_utility,
+              surf.configure(mir_surface_attrib_type,
+                             mir_surface_type_utility));
+    EXPECT_EQ(mir_surface_type_utility, surf.type());
+
+    EXPECT_THROW({
+        surf.configure(mir_surface_attrib_type, 999);
+    }, std::logic_error);
+    EXPECT_THROW({
+        surf.configure(mir_surface_attrib_type, -1);
+    }, std::logic_error);
+    EXPECT_EQ(mir_surface_type_utility, surf.type());
+
+    EXPECT_EQ(mir_surface_type_dialog,
+              surf.configure(mir_surface_attrib_type,
+                             mir_surface_type_dialog));
+    EXPECT_EQ(mir_surface_type_dialog, surf.type());
+
+    EXPECT_EQ(mir_surface_type_freestyle,
+              surf.configure(mir_surface_attrib_type,
+                             mir_surface_type_freestyle));
+    EXPECT_EQ(mir_surface_type_freestyle, surf.type());
+}