Merge lp:~raof/mir/prebump-abi-for-lifecycle-cookie into lp:~mir-team/mir/trunk

[Chris Halse Rogers]

Pre-bump SONAME for future lifecycle cookie changes

We want to do this now, since the libmirclient3 ABI bump hasn't yet
been transitioned to, so we save work.

[Robert Ancell (robert-ancell)]

Looks good to me

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1057
http://jenkins.qa.ubuntu.com/job/mir-ci/1459/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1968
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1853
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/697
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/697/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[lp:~raof/mir/prebump-abi-for-lifecycle-cookie]

1058. By Chris Halse Rogers on 2013-09-10

Switch from opaque pointers to uint64_t for event cookies.

It's highly likely that we'll want to reuse MirEventCookie for input event tagging
as well - for things like focus stealing prevention - and that will want to be portable
cross-process.

[Daniel van Vugt (vanvugt)]

I have no idea what the context is here. But tere has to be a good reason to further increase coupling like this. Can you provide more details in the description/commit message?

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1058
http://jenkins.qa.ubuntu.com/job/mir-ci/1461/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1972
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1857
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/699/console

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

not sure what we're planning to do with the new parameter

review: Needs Information

[Chris Halse Rogers (raof)]

The idea here is that sometimes clients will need to refer to a past event. They will do this with a MirEventCookie. This is generally useful - for example, we could use it in the modesetting code to verify that the configuration the client is trying to set is newer than the last hardware hotplug event, and if we tag each input event with a cookie it's useful for properly handing off focus to new surfaces/clients.

In this particular case, I'll later add a mir_connection_lifecycle_save_complete(MirConn, MirEventCookie), so that clients can tell Mir when they've finished handling the mir_lifecycle_state_may_suspend event.

I just wanted to land this while we hadn't yet transitioned to libmirclient3, to save some work.

[Daniel van Vugt (vanvugt)]

Oh. If that's what a "cookie" is then I'd suggest "sequence" or "serial" are better more meaningful names. Just make "int serial" (or whatever) a required field in each Mir*Event type.

review: Needs Fixing

[Chris Halse Rogers (raof)]

I don't want to use serial or sequence for this; particularly, we *don't* want the case, as in X, that the sequence/serial/timestamps are trivially guessable.

‘Serial’ or ‘sequence’ imply some visible ordering that I think we should avoid.

I think that ‘cookie’ has a well-understood meaning along the lines of ‘here's an opaque token you'll want to use later’, which is the API I think we want.

[Daniel van Vugt (vanvugt)]

Fair point, but "cookie" means you can't test if a>b. Only a != b. So you're saying you will never need to test if one cookie is specifically older or newer than another.

Also, the word "cookie" does not have any obvious meaning to most readers (IMHO). Hence my confusion. Even if you keep it opaque, I think a better name than "cookie" is desirable.

[Chris Halse Rogers (raof)]

*Client code* can't test if a > b, and I can't think of any particular reason why it would want to.

Suggestions for better naming welcome! :)

[Robert Ancell (robert-ancell)]

I agree we don't want ordering in the protocol, then would would have to manage wrapping around of sequence numbers etc. The server can quite easily keep a mapping of cookies to sequence if it needs it.

cookie seem acceptable to me, but other options are:
- id (though this might imply uniqueness forever)
- token

[Robert Ancell (robert-ancell)]

Note we have reverted back from libmirclient3 to libmirclient2 in trunk [1] so the pressure to land this change is reduced - we should land it at the same time as we re-apply the DPMS branch.

[1] https://lists.ubuntu.com/archives/mir-devel/2013-September/000395.html

[Daniel van Vugt (vanvugt)]

I like the "id" suggestion. Though arguably the word "sequence" could still represent an opaque value too. It's all about ensuring order, even if you can't compare a>b.

More importantly right now:
Text conflict in debian/control
Path conflict: debian/libmirclient2.install / debian/libmirclient4.install
Text conflict in debian/libmirclient4.install
Text conflict in src/client/CMakeLists.txt
4 conflicts encountered.

review: Needs Fixing

Unmerged revisions

1058. By Chris Halse Rogers on 2013-09-10

Switch from opaque pointers to uint64_t for event cookies.

It's highly likely that we'll want to reuse MirEventCookie for input event tagging
as well - for things like focus stealing prevention - and that will want to be portable
cross-process.

1057. By Chris Halse Rogers on 2013-09-10

Bump SONAME for ABI break

1056. By Chris Halse Rogers on 2013-09-10

Add a (as yet unset) opaque MirEventCookie to lifecycle callback.

Doing this now so that I can break the ABI while libmirclient3 still hasn't been
transitioned to

1055. By Alan Griffiths on 2013-09-09

input: Separate the code for dispatching input from that reading it.

Approved by PS Jenkins bot, Alexandros Frantzis.

1054. By Robert Carr on 2013-09-06

Add DPMS configuration API. Fixes: https://bugs.launchpad.net/bugs/1193222.

Approved by PS Jenkins bot, Alan Griffiths, Robert Ancell.

1053. By Eleni Maria Stea on 2013-09-05

graphics: Pull in Eleni's changes to get the DRM fd to init GBM from the host Mir instance.

Approved by PS Jenkins bot, Alexandros Frantzis, Kevin DuBois, Robert Ancell.

1052. By PS Jenkins bot on 2013-09-04

Releasing 0.0.10+13.10.20130904-0ubuntu1 (revision 1051 from lp:mir).

Approved by PS Jenkins bot.

1051. By Kevin DuBois on 2013-09-04

fix lp:1220441 (a test for android display ID was not updated). Fixes: https://bugs.launchpad.net/bugs/1220441.

Approved by Daniel van Vugt, PS Jenkins bot, Robert Ancell.

1050. By PS Jenkins bot on 2013-09-03

Releasing 0.0.10+13.10.20130903-0ubuntu1 (revision 1049 from lp:mir).

Approved by PS Jenkins bot.

1049. By Daniel van Vugt on 2013-09-03

SwitchingBundle: Simplify and clarify guarantees that compositor_acquire
always has a buffer to return without blocking or throwing an exception.

The trade-off is that to enforce the guarantee we need to permanently
reserve one buffer for compositing. This reduces the flexibility of
SwitchingBundle a little, such that minimum nbuffers is now 2.

This was originally requested by Alexandros, as the potential throw concerned
him. Although, it was logically guaranteed to never happen for other reasons.

The second reason for doing this is to eliminate recycling logic, which
while safe and correct, was quite confusing. So this change further proves
that that logic (now removed) is not to blame for frame ordering bugs.
.

Approved by Robert Ancell, Alan Griffiths, PS Jenkins bot.

=== modified file 'debian/control'
--- debian/control	2013-09-04 18:19:53 +0000
+++ debian/control	2013-09-10 05:58:22 +0000
@@ -127,7 +127,7 @@
  .
  Contains header files required to build Mir servers.
 
-Package: libmirclient3
+Package: libmirclient4
 Section: libs
 Architecture: i386 amd64 armhf arm64
 Multi-Arch: same
@@ -145,7 +145,7 @@
 Architecture: i386 amd64 armhf arm64
 Multi-Arch: same
 Pre-Depends: ${misc:Pre-Depends}
-Depends: libmirclient3 (= ${binary:Version}),
+Depends: libmirclient4 (= ${binary:Version}),
          libmirprotobuf-dev (= ${binary:Version}),
          mircommon-dev (= ${binary:Version}),
          ${misc:Depends},

=== renamed file 'debian/libmirclient3.install' => 'debian/libmirclient4.install'
--- debian/libmirclient3.install	2013-08-23 17:52:27 +0000
+++ debian/libmirclient4.install	2013-09-10 05:58:22 +0000
@@ -1,1 +1,1 @@
-usr/lib/*/libmirclient.so.3
+usr/lib/*/libmirclient.so.4

=== modified file 'include/shared/mir_toolkit/client_types.h'
--- include/shared/mir_toolkit/client_types.h	2013-09-06 03:46:37 +0000
+++ include/shared/mir_toolkit/client_types.h	2013-09-10 05:58:22 +0000
@@ -81,11 +81,12 @@
  * from the running server.
  *   \param [in] connection     The connection associated with the lifecycle event
  *   \param [in] cb             The callback requested
+ *   \param [in] cookie         The cookie associated with this event
  *   \param [in,out] context    The context provided by the client
  */
 
 typedef void (*mir_lifecycle_event_callback)(
-    MirConnection* connection, MirLifecycleState state, void* context);
+    MirConnection* connection, MirLifecycleState state, MirEventCookie cookie, void* context);
 
 /**
  * Callback called when a display config change has occurred

=== modified file 'include/shared/mir_toolkit/event.h'
--- include/shared/mir_toolkit/event.h	2013-08-28 03:41:48 +0000
+++ include/shared/mir_toolkit/event.h	2013-09-10 05:58:22 +0000
@@ -182,6 +182,10 @@
     MirSurfaceEvent surface;
 } MirEvent;
 
+/** Opaque cookie used by clients to refer to past events */
+typedef uint64_t MirEventCookie;
+
+
 #ifdef __cplusplus
 }
 /**@}*/

=== modified file 'src/client/CMakeLists.txt'
--- src/client/CMakeLists.txt	2013-09-04 18:19:53 +0000
+++ src/client/CMakeLists.txt	2013-09-10 05:58:22 +0000
@@ -68,7 +68,7 @@
   ${CLIENT_SOURCES}
 )
 
-set(MIRCLIENT_ABI 3)
+set(MIRCLIENT_ABI 4)
 
 set_target_properties(
   mirclient

=== modified file 'src/client/lifecycle_control.cpp'
--- src/client/lifecycle_control.cpp	2013-08-23 12:23:47 +0000
+++ src/client/lifecycle_control.cpp	2013-09-10 05:58:22 +0000
@@ -23,7 +23,7 @@
 namespace mcl = mir::client;
 
 mcl::LifecycleControl::LifecycleControl()
-    : handle_lifecycle_event([](MirLifecycleState){})
+    : handle_lifecycle_event([](MirLifecycleState, MirEventCookie){})
 {
 }
 
@@ -31,7 +31,7 @@
 {
 }
 
-void mcl::LifecycleControl::set_lifecycle_event_handler(std::function<void(MirLifecycleState)> const& fn)
+void mcl::LifecycleControl::set_lifecycle_event_handler(std::function<void(MirLifecycleState, MirEventCookie)> const& fn)
 {
     std::unique_lock<std::mutex> lk(guard);
 
@@ -42,5 +42,5 @@
 {
     std::unique_lock<std::mutex> lk(guard);
 
-    handle_lifecycle_event(static_cast<MirLifecycleState>(state));
+    handle_lifecycle_event(static_cast<MirLifecycleState>(state), 0);
 }

=== modified file 'src/client/lifecycle_control.h'
--- src/client/lifecycle_control.h	2013-08-23 12:23:47 +0000
+++ src/client/lifecycle_control.h	2013-09-10 05:58:22 +0000
@@ -20,6 +20,7 @@
 #define MIR_LIFECYCLE_CONTROL_H_
 
 #include "mir_toolkit/common.h"
+#include "mir_toolkit/event.h"
 
 #include <functional>
 #include <mutex>
@@ -34,12 +35,12 @@
     LifecycleControl();
     ~LifecycleControl();
 
-    void set_lifecycle_event_handler(std::function<void(MirLifecycleState)> const&);
+    void set_lifecycle_event_handler(std::function<void(MirLifecycleState, MirEventCookie)> const&);
     void call_lifecycle_event_handler(uint32_t state);
 
 private:
     std::mutex mutable guard;
-    std::function<void(MirLifecycleState)> handle_lifecycle_event;
+    std::function<void(MirLifecycleState, MirEventCookie)> handle_lifecycle_event;
 };
 }
 }

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-09-06 03:46:37 +0000
+++ src/client/mir_connection.cpp	2013-09-10 05:58:22 +0000
@@ -371,7 +371,11 @@
 
 void MirConnection::register_lifecycle_event_callback(mir_lifecycle_event_callback callback, void* context)
 {
-    lifecycle_control->set_lifecycle_event_handler(std::bind(callback, this, std::placeholders::_1, context));
+    lifecycle_control->set_lifecycle_event_handler(std::bind(callback,
+                                                             this,
+                                                             std::placeholders::_1,
+                                                             std::placeholders::_2,
+                                                             context));
 }
 
 void MirConnection::register_display_change_callback(mir_display_config_callback callback, void* context)

=== modified file 'tests/integration-tests/shell/test_session_lifecycle_event.cpp'
--- tests/integration-tests/shell/test_session_lifecycle_event.cpp	2013-08-23 12:59:23 +0000
+++ tests/integration-tests/shell/test_session_lifecycle_event.cpp	2013-09-10 05:58:22 +0000
@@ -71,7 +71,10 @@
 
     struct ClientConfig : TestingClientConfiguration
     {
-        static void lifecycle_callback(MirConnection* /*connection*/, MirLifecycleState state, void* context)
+        static void lifecycle_callback(MirConnection* /*connection*/,
+                                       MirLifecycleState state,
+                                       MirEventCookie /*cookie*/,
+                                       void* context)
         {
             auto config = static_cast<ClientConfig*>(context);
             config->handler->state_changed(state);