Mir

Merge lp:~mir-team/mir/improve-attribute-protocol-and-normalize-getters into lp:mir

improve-attribute-protocol-and-normalize-getters
Merge into development-branch

Proposed by Robert Carr on 2014-07-07

Status:

Merged

Approved by:

Alexandros Frantzis on 2014-07-18

Approved revision:

no longer in the source branch.

Merged at revision:

1778

Proposed branch:

lp:~mir-team/mir/improve-attribute-protocol-and-normalize-getters

Merge into:

lp:mir

Diff against target:

933 lines (+383/-171)

16 files modified

include/client/mir_toolkit/mir_surface.h (+14/-0)
include/server/mir/frontend/surface.h (+1/-0)
include/shared/mir_toolkit/common.h (+4/-0)
include/test/mir_test_doubles/mock_frontend_surface.h (+1/-0)
include/test/mir_test_doubles/stub_scene_surface.h (+1/-0)
src/client/mir_surface.cpp (+6/-3)
src/client/mir_surface.h (+3/-0)
src/client/mir_surface_api.cpp (+49/-10)
src/server/frontend/session_mediator.cpp (+9/-0)
src/server/scene/basic_surface.cpp (+125/-62)
src/server/scene/basic_surface.h (+9/-8)
src/shared/protobuf/mir_protobuf.proto (+2/-0)
tests/acceptance-tests/test_client_surface_visibility.cpp (+4/-1)
tests/unit-tests/client/test_client_mir_surface.cpp (+38/-26)
tests/unit-tests/scene/test_basic_surface.cpp (+117/-15)
tests/unit-tests/scene/test_surface_impl.cpp (+0/-46)

To merge this branch:

bzr merge lp:~mir-team/mir/improve-attribute-protocol-and-normalize-getters

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Gerry Boland (community)			Approve on 2014-07-16
Alexandros Frantzis (community)			Approve on 2014-07-14
Daniel van Vugt		2014-07-07	Abstain on 2014-07-11
Chris Halse Rogers			Approve on 2014-07-10
PS Jenkins bot (community)	continuous-integration		Approve on 2014-07-09
Review via email: mp+225912@code.launchpad.net

Commit message

Improve surface attribute protocol and add missing getters.

Description of the change

The motivation for this branch begins with attempting to address bug:

https://bugs.launchpad.net/mir/+bug/1336553

Seems simple enough at first but on investigation noticed a few irregularities in the surface attributes:

1. Some like focus, have no getter/persistent state.
2. Some like visibility have no getter.
3. Some like DPI have a getter on the client side, and thus support querying via the use of a "-1" value.
4. Some irregular observer behavior (missing observer calls, irregular behavior when values don't change).
5. Implementing client side getters requires use of a -1 value type trick, and leads to potentially blocking getters.

This branch sets out to clear up some of these abnormalities. You can see a series of tests instantiated for each attribute in test_basic_surface. There are a few small observer fixes, etc as mentioned. In order to make DPI behave normally (i.e. throw on invalid value rather than interpret as query), I've had to change surface attributes to be sent at surface creation time. This way the client library never has to query, it always has a value for each attribute.

Focus and visibility getters are added to the client library.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-07-08:

Just some quick comments:

(1) This should be 0..N:
+ /* Do not specify values...code relies on 1...N ordering. */

(2) The suffix "_state" is unnecessary and potentially confusing with the attribute called "state":
15 +MirSurfaceFocusState mir_surface_get_focus_state(MirSurface *surface);
23 +MirSurfaceVisibility mir_surface_get_visibility_state(MirSurface *surface);

(3) Returning an integer value outside the range of an enum is a violation of the implicit contract that a well designed API will only return values of those enums:
13 + * -1 if the surface is invalid.
21 + * -1 if the surface is invalid.
I think a more sensible fallback for invalid surfaces would be to return mir_surface_unfocused and mir_surface_visibility_occluded.

(4) src/shared/protobuf/mir_protobuf.proto: Protocol ABI change!
Please either avoid protocol changes, or record the change explicitly with a bump of libmirprotobuf0 to libmirprotobuf1. Not ideal.

review: Needs Fixing

Revision history for this message

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

FAILED: Continuous integration, rev:1741
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2082/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/850/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/856/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/849/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/603/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/601/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2688/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-07-09:

Yay! Surface attributes populated on construction! I'm broadly in favour of this :)

Your API docs don't match the behaviour; they say you'll return -1 for invalid surfaces.

While we're at it, I thought that, waaaaay back in London, we were going to abort() when passed an invalid surface? I'm very much against silently pretending that everything's OK. Likewise, I cringe at all the

try { <something> }
catch(...)
{
//la di da! The client doesn't need to know about this...
}

in there (even though much of it is pre-existing).

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-07-09:

370 + if (!set_focus_state(static_cast<MirSurfaceFocusState>(value)))
371 + BOOST_THROW_EXCEPTION(std::logic_error("Invalid focus state."));
372 + result = value;
373 break;
374 case mir_surface_attrib_swapinterval:
375 - allow_dropping = (value == 0);
376 - allow_framedropping(allow_dropping);
377 + if (!set_swap_interval(value))
378 + BOOST_THROW_EXCEPTION(std::logic_error("Invalid swapinterval"));
379 result = value;
380 break;
381 case mir_surface_attrib_dpi:
382 - if (value >= 0 && !set_dpi(value)) // Negative means query only
383 + if (!set_dpi(value))
384 BOOST_THROW_EXCEPTION(std::logic_error("Invalid DPI value"));

configure() is the only user of the set_* functions and it doesn't really need the return value. We might as well throw from the set_* functions instead of checking the return value and throwing in configure.

394 + switch (attrib)
395 + {
396 + case mir_surface_attrib_type:
397 + return type_value;
398 + case mir_surface_attrib_state:
399 + return state_value;
...

It seems like we storing the attributes in an array indexed by the attribute instead of separate variables would make life easier for BasicSurface.

review: Needs Fixing

Revision history for this message

Robert Carr (robertcarr) wrote on 2014-07-09:

Refactored the set_ functions

Added attribute array (caught several more missing locks in the meantime).

Removed misleading comment.

Lets do something about client library errors...but not in this branch...was our plan in london written down? I'll see what I can remember over lunch...I don't remember each detail.

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-07-10:

Re: client errors - in one of those enthusiastic London days we decided that API constructors would never return NULL, but instead return an error object. Because this makes it very easy to write programs that fail in unexpected ways, we further decided that passing an error object into any API call that isn't explicitly a check for error status would die, so that developers get immediate and useful corefile-based feedback.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-07-10:

Bah. I was going to suggest std::array<int, > for the attrib array on the basis that operator[] would do nice bounds checking for us, but it doesn't because of silliness.

review: Approve

Revision history for this message

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

(4) Hmm, the protocol change uses "repeated" which actually appears to be backward compatible, so no ABI bump for libmirprotobuf is required after all;
https://developers.google.com/protocol-buffers/docs/proto#updating

(5) This single-use function doesn't need to exist. If you initialize your variables in the constructor then it's more obvious locking isn't required either...
ms::BasicSurface::initialize_attributes()

(6) I think the TODO comment is still valid and should not be deleted:
395 - /*
396 - * TODO: In future, query the shell implementation for the subset of
397 - * attributes/types it implements.
398 - */

Needs^H^H^H^H^HWants fixing, somewhat. I haven't gone back through in enough detail to approve yet but won't block any more.

review: Abstain

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-07-11:

The protocol change is the sort of thing that we won't be able to get away with when used on the desktop, but because the phone reboots to do any package update it's ok (as long as libmirclient updates in lockstep with libmirserver!)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-07-14:

Looks good.

Nit:
260 + std::lock_guard<std::mutex> lg(guard);

As mentioned by Daniel, locking is not currently needed.

review: Approve

Revision history for this message

Gerry Boland (gerboland) wrote on 2014-07-16:

+ MirSurfaceType set_type(MirSurfaceType t); // Use configure() to make public changes
+ MirSurfaceState set_state(MirSurfaceState s);
+ int set_dpi(int);
+ MirSurfaceVisibility set_visibility(MirSurfaceVisibility v);
+ int set_swap_interval(int);
+ MirSurfaceFocusState set_focus_state(MirSurfaceFocusState f);

these look weird to me - why returning the value set? Could a different value be set than what I asked for? I'd be suspicious of it, but it's private so meh.
LGTM otherwise

review: Approve

Revision history for this message

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

Gerry:
We return the value applied because it might be different to the requested value, in some restricted shells. That's the TODO I was referring to above in (6).

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:

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'include/client/mir_toolkit/mir_surface.h'
 --- include/client/mir_toolkit/mir_surface.h	2014-06-19 16:15:42 +0000
 +++ include/client/mir_toolkit/mir_surface.h	2014-07-09 21:43:55 +0000
@@ -253,6 +253,20 @@
   *   \return  The DPI of the surface, or zero if unknown.
   */
  int mir_surface_get_dpi(MirSurface* surface);
++
++/**
++ * Query the focus state for a surface.
++ *   \param [in] surface The surface to operate on
++ *   \return             The focus state of said surface
++ */
++MirSurfaceFocusState mir_surface_get_focus(MirSurface *surface);
++
++/**
++ * Query the visibility state for a surface.
++ *   \param [in] surface The surface to operate on
++ *   \return             The visibility state of said surface
++ */
++MirSurfaceVisibility mir_surface_get_visibility(MirSurface *surface);
  /**
   * Choose the cursor state for a surface: whether a cursor is shown,
 === modified file 'include/server/mir/frontend/surface.h'
 --- include/server/mir/frontend/surface.h	2014-06-09 17:35:20 +0000
 +++ include/server/mir/frontend/surface.h	2014-07-09 21:43:55 +0000
@@ -53,6 +53,7 @@
      virtual int client_input_fd() const = 0;
      virtual int configure(MirSurfaceAttrib attrib, int value) = 0;
++    virtual int query(MirSurfaceAttrib attrib) = 0;
      virtual void set_cursor_image(std::shared_ptr<graphics::CursorImage> const& image) = 0;
 === modified file 'include/shared/mir_toolkit/common.h'
 --- include/shared/mir_toolkit/common.h	2014-06-23 02:57:39 +0000
 +++ include/shared/mir_toolkit/common.h	2014-07-09 21:43:55 +0000
@@ -33,12 +33,14 @@
   */
  typedef enum MirSurfaceAttrib
+ {
++    /* Do not specify values...code relies on 0...N ordering. */
      mir_surface_attrib_type,
      mir_surface_attrib_state,
      mir_surface_attrib_swapinterval,
      mir_surface_attrib_focus,
      mir_surface_attrib_dpi,
      mir_surface_attrib_visibility,
++    /* Must be last */
      mir_surface_attribs
  } MirSurfaceAttrib;
@@ -68,6 +70,8 @@
      mir_surface_states
  } MirSurfaceState;
++/* TODO: MirSurfaceFocusState MirSurfaceVisibility and MirLifecycleState use an inconsistent
++   naming convention. */
  typedef enum MirSurfaceFocusState
+ {
      mir_surface_unfocused = 0,
 === modified file 'include/test/mir_test_doubles/mock_frontend_surface.h'
 --- include/test/mir_test_doubles/mock_frontend_surface.h	2014-06-03 17:00:23 +0000
 +++ include/test/mir_test_doubles/mock_frontend_surface.h	2014-07-09 21:43:55 +0000
@@ -50,6 +50,7 @@
      MOCK_METHOD1(set_cursor_image, void(std::shared_ptr<graphics::CursorImage> const&));
      MOCK_METHOD2(configure, int(MirSurfaceAttrib, int));
++    MOCK_METHOD1(query, int(MirSurfaceAttrib));
  };
+ }
+ }
 === modified file 'include/test/mir_test_doubles/stub_scene_surface.h'
 --- include/test/mir_test_doubles/stub_scene_surface.h	2014-06-23 22:25:17 +0000
 +++ include/test/mir_test_doubles/stub_scene_surface.h	2014-07-09 21:43:55 +0000
@@ -96,6 +96,7 @@
      bool supports_input() const override { return true;}
      int client_input_fd() const override { return fd;}
      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 'src/client/mir_surface.cpp'
 --- src/client/mir_surface.cpp	2014-06-23 02:57:39 +0000
 +++ src/client/mir_surface.cpp	2014-07-09 21:43:55 +0000
@@ -67,9 +67,6 @@
      for (int i = 0; i < mir_surface_attribs; i++)
          attrib_cache[i] = -1;
--    attrib_cache[mir_surface_attrib_type] = mir_surface_type_normal;
--    attrib_cache[mir_surface_attrib_state] = mir_surface_state_unknown;
--    attrib_cache[mir_surface_attrib_swapinterval] = 1;
      std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
      valid_surfaces.insert(this);
@@ -228,6 +225,12 @@
          process_incoming_buffer();
          accelerated_window = platform->create_egl_native_window(this);
++
++        for(int i = 0; i < surface.attributes_size(); i++)
++        {
++            auto const& attrib = surface.attributes(i);
++            attrib_cache[attrib.attrib()] = attrib.ivalue();
++        }
+     }
      connection->on_surface_created(id(), this);
 === modified file 'src/client/mir_surface.h'
 --- src/client/mir_surface.h	2014-06-23 02:57:39 +0000
 +++ src/client/mir_surface.h	2014-07-09 21:43:55 +0000
@@ -86,7 +86,10 @@
      EGLNativeWindowType generate_native_window();
      MirWaitHandle* configure(MirSurfaceAttrib a, int value);
++
++    // Non-blocking
      int attrib(MirSurfaceAttrib a) const;
++
      MirOrientation get_orientation() const;
      MirWaitHandle* configure_cursor(MirCursorConfiguration const* cursor);
 === modified file 'src/client/mir_surface_api.cpp'
 --- src/client/mir_surface_api.cpp	2014-06-19 16:15:42 +0000
 +++ src/client/mir_surface_api.cpp	2014-07-09 21:43:55 +0000
@@ -247,34 +247,73 @@
  int mir_surface_get_swapinterval(MirSurface* surf)
+ {
--    return surf ? surf->attrib(mir_surface_attrib_swapinterval) : -1;
++    int swap_interval = -1;
++
++    try
++    {
++        swap_interval = surf ? surf->attrib(mir_surface_attrib_swapinterval) : -1;
++    }
++    catch (...)
++    {
++    }
++
++    return swap_interval;
+ }
  int mir_surface_get_dpi(MirSurface* surf)
+ {
--    int dpi = 0;
++    int dpi = -1;
      try
+     {
          if (surf)
+         {
              dpi = surf->attrib(mir_surface_attrib_dpi);
--            if (dpi < 0)
--            {
--                // Officially we don't support setting DPI from the client.
--                // But this is a convenient way to query it on startup...
--                surf->configure(mir_surface_attrib_dpi, -1)->wait_for_all();
--                dpi = surf->attrib(mir_surface_attrib_dpi);
--            }
+         }
+     }
--    catch (std::exception const&)
++    catch (...)
+     {
+     }
      return dpi;
+ }
++MirSurfaceFocusState mir_surface_get_focus(MirSurface* surf)
++{
++    MirSurfaceFocusState state = mir_surface_unfocused;
++
++    try
++    {
++        if (surf)
++        {
++            state = static_cast<MirSurfaceFocusState>(surf->attrib(mir_surface_attrib_focus));
++        }
++    }
++    catch (...)
++    {
++    }
++
++    return state;
++}
++
++MirSurfaceVisibility mir_surface_get_visibility(MirSurface* surf)
++{
++    MirSurfaceVisibility state = static_cast<MirSurfaceVisibility>(mir_surface_visibility_occluded);
++
++    try
++    {
++        if (surf)
++        {
++            state = static_cast<MirSurfaceVisibility>(surf->attrib(mir_surface_attrib_visibility));
++        }
++    }
++    catch (...)
++    {
++    }
++
++    return state;
++}
++
  MirWaitHandle* mir_surface_configure_cursor(MirSurface* surface, MirCursorConfiguration const* cursor)
+ {
      MirWaitHandle *result = nullptr;
 === modified file 'src/server/frontend/session_mediator.cpp'
 --- src/server/frontend/session_mediator.cpp	2014-07-09 10:48:47 +0000
 +++ src/server/frontend/session_mediator.cpp	2014-07-09 21:43:55 +0000
@@ -196,6 +196,15 @@
      if (surface->supports_input())
          response->add_fd(surface->client_input_fd());
++
++    for (unsigned int i = 0; i < mir_surface_attribs; i++)
++    {
++        auto setting = response->add_attributes();
++
++        setting->mutable_surfaceid()->set_value(surf_id.as_value());
++        setting->set_attrib(i);
++        setting->set_ivalue(surface->query(static_cast<MirSurfaceAttrib>(i)));
++    }
      advance_buffer(surf_id, *surface,
          [lock, this, response, done, session]
 === modified file 'src/server/scene/basic_surface.cpp'
 --- src/server/scene/basic_surface.cpp	2014-07-09 07:54:29 +0000
 +++ src/server/scene/basic_surface.cpp	2014-07-09 21:43:55 +0000
@@ -159,15 +159,24 @@
      input_sender(input_sender),
      configurator(configurator),
      cursor_image_(cursor_image),
--    report(report),
--    type_value(mir_surface_type_normal),
--    state_value(mir_surface_state_restored),
--    visibility_value(mir_surface_visibility_exposed),
--    dpi_value(0)
++    report(report)
+ {
++    initialize_attributes();
      report->surface_created(this, surface_name);
+ }
++void ms::BasicSurface::initialize_attributes()
++{
++    std::lock_guard<std::mutex> lg(guard);
++
++    attrib_values[mir_surface_attrib_type] = mir_surface_type_normal;
++    attrib_values[mir_surface_attrib_state] = mir_surface_state_restored;
++    attrib_values[mir_surface_attrib_swapinterval] = 1;
++    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;
++}
++
  void ms::BasicSurface::force_requests_to_complete()
+ {
      surface_buffer_stream->force_requests_to_complete();
@@ -404,42 +413,94 @@
  MirSurfaceType ms::BasicSurface::type() const
--{
--    return type_value;
++{
++    std::unique_lock<std::mutex> lg(guard);
++    return static_cast<MirSurfaceType>(attrib_values[mir_surface_attrib_type]);
+ }
--bool ms::BasicSurface::set_type(MirSurfaceType t)
++MirSurfaceType ms::BasicSurface::set_type(MirSurfaceType t)
+ {
--    bool valid = false;
--
--    if (t >= 0 && t < mir_surface_types)
--    {
--        type_value = t;
--        valid = true;
--    }
--
--    return valid;
++    std::unique_lock<std::mutex> lg(guard);
++
++    if (t < 0 || t > mir_surface_types)
++    {
++        BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
++            "type."));
++    }
++
++    if (attrib_values[mir_surface_attrib_type] != t)
++    {
++        attrib_values[mir_surface_attrib_type] = t;
++        lg.unlock();
++
++        observers.attrib_changed(mir_surface_attrib_type, attrib_values[mir_surface_attrib_type]);
++    }
++
++    return t;
+ }
  MirSurfaceState ms::BasicSurface::state() const
+ {
--    return state_value;
++    std::unique_lock<std::mutex> lg(guard);
++    return static_cast<MirSurfaceState>(attrib_values[mir_surface_attrib_state]);
+ }
--bool ms::BasicSurface::set_state(MirSurfaceState s)
++MirSurfaceState ms::BasicSurface::set_state(MirSurfaceState s)
+ {
--    bool valid = false;
++    if (s < mir_surface_state_unknown || s > mir_surface_states)
++        BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface state."));
--    if (s > mir_surface_state_unknown &&
--        s < mir_surface_states)
++    std::unique_lock<std::mutex> lg(guard);
++    if (attrib_values[mir_surface_attrib_state] != s)
+     {
--        state_value = s;
--        valid = true;
--
++        attrib_values[mir_surface_attrib_state] = s;
++        lg.unlock();
++
          observers.attrib_changed(mir_surface_attrib_state, s);
+     }
--    return valid;
++    return s;
++}
++
++int ms::BasicSurface::set_swap_interval(int interval)
++{
++    if (interval < 0)
++    {
++        BOOST_THROW_EXCEPTION(std::logic_error("Invalid swapinterval"));
++    }
++
++    std::unique_lock<std::mutex> lg(guard);
++    if (attrib_values[mir_surface_attrib_swapinterval] != interval)
++    {
++        attrib_values[mir_surface_attrib_swapinterval] = interval;
++        bool allow_dropping = (interval == 0);
++        allow_framedropping(allow_dropping);
++
++        lg.unlock();
++        observers.attrib_changed(mir_surface_attrib_swapinterval, interval);
++    }
++
++    return interval;
++}
++
++MirSurfaceFocusState ms::BasicSurface::set_focus_state(MirSurfaceFocusState new_state)
++{
++    if (new_state != mir_surface_focused &&
++        new_state != mir_surface_unfocused)
++    {
++        BOOST_THROW_EXCEPTION(std::logic_error("Invalid focus state."));
++    }
++
++    std::unique_lock<std::mutex> lg(guard);
++    if (attrib_values[mir_surface_attrib_focus] != new_state)
++    {
++        attrib_values[mir_surface_attrib_focus] = new_state;
++
++        lg.unlock();
++        observers.attrib_changed(mir_surface_attrib_focus, new_state);
++    }
++
++    return new_state;
+ }
  void ms::BasicSurface::take_input_focus(std::shared_ptr<msh::InputTargeter> const& targeter)
@@ -449,43 +510,26 @@
  int ms::BasicSurface::configure(MirSurfaceAttrib attrib, int value)
+ {
--    bool allow_dropping = false;
--
--    /*
--     * TODO: In future, query the shell implementation for the subset of
--     *       attributes/types it implements.
--     */
--    int result = value = configurator->select_attribute_value(*this, attrib, value);
++    int result = configurator->select_attribute_value(*this, attrib, value);
      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();
++        result = set_type(static_cast<MirSurfaceType>(result));
          break;
      case mir_surface_attrib_state:
--        if (value != mir_surface_state_unknown &&
--            !set_state(static_cast<MirSurfaceState>(value)))
--            BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface state."));
--        result = state();
++        result = set_state(static_cast<MirSurfaceState>(result));
          break;
      case mir_surface_attrib_focus:
--        observers.attrib_changed(attrib, value);
++        result = set_focus_state(static_cast<MirSurfaceFocusState>(result));
          break;
      case mir_surface_attrib_swapinterval:
--        allow_dropping = (value == 0);
--        allow_framedropping(allow_dropping);
--        result = value;
++        result = set_swap_interval(result);
          break;
      case mir_surface_attrib_dpi:
--        if (value >= 0 && !set_dpi(value))  // Negative means query only
--            BOOST_THROW_EXCEPTION(std::logic_error("Invalid DPI value"));
--        result = dpi();
++        result = set_dpi(result);
          break;
      case mir_surface_attrib_visibility:
--        set_visibility(static_cast<MirSurfaceVisibility>(value));
--        result = visibility_value;
++        result = set_visibility(static_cast<MirSurfaceVisibility>(result));
          break;
      default:
          BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
@@ -498,6 +542,16 @@
      return result;
+ }
++int ms::BasicSurface::query(MirSurfaceAttrib attrib)
++{
++    if (attrib < 0 || attrib > mir_surface_attribs)
++        BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
++                                               "attribute."));
++
++    std::unique_lock<std::mutex> lg(guard);
++    return attrib_values[attrib];
++}
++
  void ms::BasicSurface::hide()
+ {
      set_hidden(true);
@@ -528,24 +582,29 @@
  int ms::BasicSurface::dpi() const
+ {
--    return dpi_value;
++    std::unique_lock<std::mutex> lock(guard);
++    return attrib_values[mir_surface_attrib_dpi];
+ }
--bool ms::BasicSurface::set_dpi(int new_dpi)
++int ms::BasicSurface::set_dpi(int new_dpi)
+ {
--    bool valid = false;
++    if (new_dpi < 0)
++    {
++        BOOST_THROW_EXCEPTION(std::logic_error("Invalid DPI value"));
++    }
--    if (new_dpi >= 0)
++    std::unique_lock<std::mutex> lg(guard);
++    if (attrib_values[mir_surface_attrib_dpi] != new_dpi)
+     {
--        dpi_value = new_dpi;
--        valid = true;
--        observers.attrib_changed(mir_surface_attrib_dpi, dpi_value);
++        attrib_values[mir_surface_attrib_dpi] = new_dpi;
++        lg.unlock();
++        observers.attrib_changed(mir_surface_attrib_dpi, new_dpi);
+     }
--    return valid;
++    return new_dpi;
+ }
--void ms::BasicSurface::set_visibility(MirSurfaceVisibility new_visibility)
++MirSurfaceVisibility ms::BasicSurface::set_visibility(MirSurfaceVisibility new_visibility)
+ {
      if (new_visibility != mir_surface_visibility_occluded &&
          new_visibility != mir_surface_visibility_exposed)
@@ -553,11 +612,15 @@
          BOOST_THROW_EXCEPTION(std::logic_error("Invalid visibility value"));
+     }
--    if (visibility_value != new_visibility)
++    std::unique_lock<std::mutex> lg(guard);
++    if (attrib_values[mir_surface_attrib_visibility] != new_visibility)
+     {
--        visibility_value = new_visibility;
--        observers.attrib_changed(mir_surface_attrib_visibility, visibility_value);
++        attrib_values[mir_surface_attrib_visibility] = new_visibility;
++        lg.unlock();
++        observers.attrib_changed(mir_surface_attrib_visibility, attrib_values[mir_surface_attrib_visibility]);
+     }
++
++    return new_visibility;
+ }
  void ms::BasicSurface::add_observer(std::shared_ptr<SurfaceObserver> const& observer)
 === modified file 'src/server/scene/basic_surface.h'
 --- src/server/scene/basic_surface.h	2014-06-27 13:40:34 +0000
 +++ src/server/scene/basic_surface.h	2014-07-09 21:43:55 +0000
@@ -139,6 +139,7 @@
      MirSurfaceState state() const override;
      void take_input_focus(std::shared_ptr<shell::InputTargeter> const& targeter) override;
      int configure(MirSurfaceAttrib attrib, int value) override;
++    int query(MirSurfaceAttrib attrib) override;
      void hide() override;
      void show() override;
@@ -152,10 +153,12 @@
  private:
      bool visible(std::unique_lock<std::mutex>&) const;
--    bool set_type(MirSurfaceType t);  // Use configure() to make public changes
--    bool set_state(MirSurfaceState s);
--    bool set_dpi(int);
--    void set_visibility(MirSurfaceVisibility v);
++    MirSurfaceType set_type(MirSurfaceType t);  // Use configure() to make public changes
++    MirSurfaceState set_state(MirSurfaceState s);
++    int set_dpi(int);
++    MirSurfaceVisibility set_visibility(MirSurfaceVisibility v);
++    int set_swap_interval(int);
++    MirSurfaceFocusState set_focus_state(MirSurfaceFocusState f);
      SurfaceObservers observers;
      std::mutex mutable guard;
@@ -175,10 +178,8 @@
      std::shared_ptr<graphics::CursorImage> cursor_image_;
      std::shared_ptr<SceneReport> const report;
--    MirSurfaceType type_value;
--    MirSurfaceState state_value;
--    MirSurfaceVisibility visibility_value;
--    int dpi_value;
++    void initialize_attributes();
++    int attrib_values[mir_surface_attribs];
  };
+ }
 === modified file 'src/shared/protobuf/mir_protobuf.proto'
 --- src/shared/protobuf/mir_protobuf.proto	2014-07-09 07:54:29 +0000
 +++ src/shared/protobuf/mir_protobuf.proto	2014-07-09 21:43:55 +0000
@@ -103,6 +103,8 @@
    repeated sint32 fd = 7;
    optional int32 fds_on_side_channel = 8;
++
++  repeated SurfaceSetting attributes = 9;
    optional string error = 127;
+ }
 === modified file 'tests/acceptance-tests/test_client_surface_visibility.cpp'
 --- tests/acceptance-tests/test_client_surface_visibility.cpp	2014-06-23 17:19:22 +0000
 +++ tests/acceptance-tests/test_client_surface_visibility.cpp	2014-07-09 21:43:55 +0000
@@ -293,7 +293,10 @@
          mt::WaitCondition event_received;
          EXPECT_CALL(mock_visibility_callback, handle(surface, visibility))
--            .WillOnce(mt::WakeUp(&event_received));
++            .WillOnce(DoAll(Invoke([&visibility](MirSurface *s, MirSurfaceVisibility)
++                {
++                    EXPECT_EQ(visibility, mir_surface_get_visibility(s));
++                }), mt::WakeUp(&event_received)));
          action();
 === modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
 --- tests/unit-tests/client/test_client_mir_surface.cpp	2014-06-24 12:55:16 +0000
 +++ tests/unit-tests/client/test_client_mir_surface.cpp	2014-07-09 21:43:55 +0000
@@ -36,11 +36,12 @@
  #include "mir_test/gmock_fixes.h"
  #include "mir_test/fake_shared.h"
--#include <cstring>
--
  #include <gtest/gtest.h>
  #include <gmock/gmock.h>
++#include <cstring>
++#include <map>
++
  #include <fcntl.h>
  namespace mcl = mir::client;
@@ -96,6 +97,8 @@
      int pf_sent;
      int stride_sent;
++    static std::map<int, int> sent_surface_attributes;
++
  private:
      void generate_unique_buffer()
+     {
@@ -143,16 +146,26 @@
          response->set_width(server_package.width);
          response->set_height(server_package.height);
+     }
--
++
      void create_surface_response(mir::protobuf::Surface* response)
+     {
++        unsigned int const id = 2;
++
          response->set_fds_on_side_channel(1);
--        response->mutable_id()->set_value(2);
++        response->mutable_id()->set_value(id);
          response->set_width(width_sent);
          response->set_height(height_sent);
          response->set_pixel_format(pf_sent);
          response->add_fd(input_fd);
++
++        for (auto const& kv : sent_surface_attributes)
++        {
++            auto setting = response->add_attributes();
++            setting->mutable_surfaceid()->set_value(id);
++            setting->set_attrib(kv.first);
++            setting->set_ivalue(kv.second);
++        }
          create_buffer_response(response->mutable_buffer());
+     }
@@ -167,6 +180,15 @@
      int global_buffer_id;
  };
++std::map<int, int> MockServerPackageGenerator::sent_surface_attributes = {
++    { mir_surface_attrib_type, mir_surface_type_normal },
++    { mir_surface_attrib_state, mir_surface_state_restored },
++    { 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 }
++};
++
  struct MockBuffer : public mcl::ClientBuffer
+ {
      MockBuffer()
@@ -469,6 +491,18 @@
      create_and_wait_for_surface_with(*client_comm_channel, mock_buffer_factory);
+ }
++TEST_F(MirClientSurfaceTest, attributes_set_on_surface_creation)
++{
++    using namespace testing;
++
++    auto surface = create_and_wait_for_surface_with(*client_comm_channel, mock_buffer_factory);
++
++    for (int i = 0; i < mir_surface_attribs; i++)
++    {
++        EXPECT_EQ(MockServerPackageGenerator::sent_surface_attributes[i], surface->attrib(static_cast<MirSurfaceAttrib>(i)));
++    }
++}
++
  TEST_F(MirClientSurfaceTest, create_wait_handle_really_blocks)
+ {
      using namespace testing;
@@ -630,28 +664,6 @@
      EXPECT_THAT(after.height, Eq(new_height));
+ }
--TEST_F(MirClientSurfaceTest, default_surface_type)
--{
--    using namespace testing;
--
--    auto const surface = create_and_wait_for_surface_with(*client_comm_channel, stub_buffer_factory);
--
--    EXPECT_THAT(surface->attrib(mir_surface_attrib_type),
--                Eq(mir_surface_type_normal));
--}
--
--TEST_F(MirClientSurfaceTest, default_surface_state)
--{
--    using namespace testing;
--
--    auto const surface = create_and_wait_for_surface_with(*client_comm_channel, stub_buffer_factory);
--
--    // Test the default cached state value. It is always unknown until we
--    // get a real answer from the server.
--    EXPECT_THAT(surface->attrib(mir_surface_attrib_state),
--                Eq(mir_surface_state_unknown));
--}
--
  TEST_F(MirClientSurfaceTest, get_cpu_region_returns_correct_data)
+ {
      using namespace testing;
 === modified file 'tests/unit-tests/scene/test_basic_surface.cpp'
 --- tests/unit-tests/scene/test_basic_surface.cpp	2014-07-09 07:54:29 +0000
 +++ tests/unit-tests/scene/test_basic_surface.cpp	2014-07-09 21:43:55 +0000
@@ -468,48 +468,150 @@
      EXPECT_EQ(mi::InputReceptionMode::receives_all_input, surface.reception_mode());
+ }
--TEST_F(BasicSurfaceTest, notifies_about_visibility_attrib_changes)
++namespace
++{
++
++struct AttributeTestParameters
++{
++    MirSurfaceAttrib attribute;
++    int default_value;
++    int a_valid_value;
++    int an_invalid_value;
++};
++
++struct BasicSurfaceAttributeTest : public BasicSurfaceTest,
++    public ::testing::WithParamInterface<AttributeTestParameters>
++{
++};
++
++AttributeTestParameters const surface_visibility_test_parameters{
++    mir_surface_attrib_visibility,
++    mir_surface_visibility_exposed,
++    mir_surface_visibility_occluded,
++    -1
++};
++
++AttributeTestParameters const surface_type_test_parameters{
++    mir_surface_attrib_type,
++    mir_surface_type_normal,
++    mir_surface_type_freestyle,
++    -1
++};
++
++AttributeTestParameters const surface_state_test_parameters{
++    mir_surface_attrib_state,
++    mir_surface_state_restored,
++    mir_surface_state_fullscreen,
++    1178312
++};
++
++AttributeTestParameters const surface_swapinterval_test_parameters{
++    mir_surface_attrib_swapinterval,
++    1,
++    0,
++    -1
++};
++
++AttributeTestParameters const surface_dpi_test_parameters{
++    mir_surface_attrib_dpi,
++    0,
++    90,
++    -1
++};
++
++AttributeTestParameters const surface_focus_test_parameters{
++    mir_surface_attrib_focus,
++    mir_surface_unfocused,
++    mir_surface_focused,
++    -1
++};
++
++}
++
++TEST_P(BasicSurfaceAttributeTest, default_value)
++{
++    auto const& params = GetParam();
++    auto const& attribute = params.attribute;
++    auto const& default_value = params.default_value;
++
++    EXPECT_EQ(default_value, surface.query(attribute));
++}
++
++TEST_P(BasicSurfaceAttributeTest, notifies_about_attrib_changes)
+ {
      using namespace testing;
++
++    auto const& params = GetParam();
++    auto const& attribute = params.attribute;
++    auto const& value1 = params.a_valid_value;
++    auto const& value2 = params.default_value;
++
      MockSurfaceAttribObserver mock_surface_observer;
      InSequence s;
--    EXPECT_CALL(mock_surface_observer, attrib_changed(mir_surface_attrib_visibility, mir_surface_visibility_occluded))
++    EXPECT_CALL(mock_surface_observer, attrib_changed(attribute, value1))
          .Times(1);
--    EXPECT_CALL(mock_surface_observer, attrib_changed(mir_surface_attrib_visibility, mir_surface_visibility_exposed))
++    EXPECT_CALL(mock_surface_observer, attrib_changed(attribute, value2))
          .Times(1);
      surface.add_observer(mt::fake_shared(mock_surface_observer));
--    surface.configure(mir_surface_attrib_visibility, mir_surface_visibility_occluded);
--    surface.configure(mir_surface_attrib_visibility, mir_surface_visibility_exposed);
++    surface.configure(attribute, value1);
++    surface.configure(attribute, value2);
+ }
--TEST_F(BasicSurfaceTest, does_not_notify_if_visibility_attrib_is_unchanged)
++TEST_P(BasicSurfaceAttributeTest, does_not_notify_if_attrib_is_unchanged)
+ {
      using namespace testing;
++
++    auto const& params = GetParam();
++    auto const& attribute = params.attribute;
++    auto const& default_value = params.default_value;
++    auto const& another_value = params.a_valid_value;
++
      MockSurfaceAttribObserver mock_surface_observer;
--    EXPECT_CALL(mock_surface_observer, attrib_changed(mir_surface_attrib_visibility, mir_surface_visibility_occluded))
++    EXPECT_CALL(mock_surface_observer, attrib_changed(attribute, another_value))
          .Times(1);
      surface.add_observer(mt::fake_shared(mock_surface_observer));
--    surface.configure(mir_surface_attrib_visibility, mir_surface_visibility_exposed);
--    surface.configure(mir_surface_attrib_visibility, mir_surface_visibility_occluded);
--    surface.configure(mir_surface_attrib_visibility, mir_surface_visibility_occluded);
++    surface.configure(attribute, default_value);
++    surface.configure(attribute, another_value);
++    surface.configure(attribute, another_value);
+ }
--TEST_F(BasicSurfaceTest, throws_on_invalid_visibility_attrib_value)
++TEST_P(BasicSurfaceAttributeTest, throws_on_invalid_value)
+ {
      using namespace testing;
--
++
++    auto const& params = GetParam();
++    auto const& attribute = params.attribute;
++    auto const& invalid_value = params.an_invalid_value;
++
      EXPECT_THROW({
--        surface.configure(mir_surface_attrib_visibility,
--                          static_cast<int>(mir_surface_visibility_exposed) + 1);
--    }, std::logic_error);
++            surface.configure(attribute, invalid_value);
++        }, std::logic_error);
+ }
++INSTANTIATE_TEST_CASE_P(SurfaceTypeAttributeTest, BasicSurfaceAttributeTest,
++   ::testing::Values(surface_type_test_parameters));
++
++INSTANTIATE_TEST_CASE_P(SurfaceVisibilityAttributeTest, BasicSurfaceAttributeTest,
++   ::testing::Values(surface_visibility_test_parameters));
++
++INSTANTIATE_TEST_CASE_P(SurfaceStateAttributeTest, BasicSurfaceAttributeTest,
++   ::testing::Values(surface_state_test_parameters));
++
++INSTANTIATE_TEST_CASE_P(SurfaceSwapintervalAttributeTest, BasicSurfaceAttributeTest,
++   ::testing::Values(surface_swapinterval_test_parameters));
++
++INSTANTIATE_TEST_CASE_P(SurfaceDPIAttributeTest, BasicSurfaceAttributeTest,
++   ::testing::Values(surface_dpi_test_parameters));
++
++INSTANTIATE_TEST_CASE_P(SurfaceFocusAttributeTest, BasicSurfaceAttributeTest,
++   ::testing::Values(surface_focus_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-06-23 22:25:17 +0000
 +++ tests/unit-tests/scene/test_surface_impl.cpp	2014-07-09 21:43:55 +0000
@@ -186,52 +186,6 @@
      EXPECT_EQ(mir_surface_state_fullscreen, surf.state());
+ }
--TEST_F(Surface, dpi_is_initialized)
--{
--    using namespace testing;
--
--    ms::BasicSurface surf(
--        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_EQ(0, surf.dpi()); // The current default. It will change.
--}
--
--TEST_F(Surface, dpi_changes)
--{
--    using namespace testing;
--
--    ms::BasicSurface surf(
--        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_EQ(123, surf.configure(mir_surface_attrib_dpi, 123));
--    EXPECT_EQ(123, surf.dpi());
--
--    EXPECT_EQ(456, surf.configure(mir_surface_attrib_dpi, 456));
--    EXPECT_EQ(456, surf.dpi());
--
--    surf.configure(mir_surface_attrib_dpi, -1);
--    EXPECT_EQ(456, surf.dpi());
--
--    EXPECT_EQ(789, surf.configure(mir_surface_attrib_dpi, 789));
--    EXPECT_EQ(789, surf.dpi());
--}
--
  bool operator==(MirEvent const& a, MirEvent const& b)
+ {
      // We will always fill unused bytes with zero, so memcmp is accurate...