Merge lp:~raof/mir/persistent-surface-id into lp:mir

[Chris Halse Rogers]

Add mir_surface_request_persistent_id() API.

This requests an ID for a surface which can be referenced by other clients and is stable over time.

API for actually using these IDs will come as a followup.

First - slightly useless - half of persistent surface reference support.

Adds the ability for a client to get a MirSurfaceId, which *currently* does nothing, but in a follow-up will be convertible to a string, sent cross-process, and deserialised into a MirSurfaceId that can be set in an input method spec to do cross-process surface-relative positioning.

It will also grow into support for restoring surface position/state across application restarts.

[Daniel van Vugt (vanvugt)]

Using UUIDs only makes sense if "persistent" also means persistent between runs, and persistent across all machines. Obviously a unique ID for the lifetime of the server process could be achieved with an int...

(1) So if you need a persistent ID across machines and server instances, what happens if you have multiple instances of a client running? They'd have the same IDs, which would be a problem right?

(2) What about simple apps/toolkits that don't support the UUID? For example X apps. Can we live with some windows not having IDs? In the past I've suggested the fallback should be some string that just describes the window title and default attributes, which would at least be consistent between runs. So that's "mir_surface_id_from_string"? But there's no good guarantee of uniqueness.

(3) Given we can't have a 100% water-tight guarantee that this new MirSurfaceId is unique (per (2)), it would need to co-exist with and never replace our internal integer IDs. So in that case it's not really the "surface ID" but should be called something else like "MirSurfaceSignature". So at least someone looking at the Mir source code knows the surface ID is not the surface signature.

(1) and (2) need info. (3) I think needs fixing.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2506
http://jenkins.qa.ubuntu.com/job/mir-ci/3656/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2235/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2234/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2185/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1653/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2185/console

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

(4) Consider using the client executable binary path instead of UUID:
    readlink("/proc/self/exe", ...)

That would give a sufficiently unique string with the added bonus of completely solving issue (2) as no apps or toolkits would need modification at all. All clients would support it for free.

[Chris Halse Rogers (raof)]

(1) If you have multiple instances of a client running, their surfaces get different IDs.

(2) If an app doesn't need to do anything that requires an ID it doesn't get one. Simple apps won't do anything requiring an ID. We punt on trying to restore surface state for apps that don't explicitly opt-in (this actually isn't any different from X).

IM frameworks are an interesting case. For native Mir clients, it's fine - the IM framework has an in-process plugin that can request the surface ID. For XMir clients we need to do something special anyway - the IM framework doesn't have any useful knowlege in that case. I was thinking of having XMir ask for a ID for all its surfaces, then setting a _X_CANONICAL_MIR_ID property that the IM framework can read out.

We'll have to do something like that regardless of how we identify surfaces (at least for sensible ways of identifying surfaces), unless we integrate XMir more closely into Mir - at which point XID → MirSurface lookups become possible, and this still works.

(3) We can guarantee this MirSurfaceId is unique. I don't want to replace our internal integer IDs with it; they serve different purposes. But we don't expose those integer IDs to the client, so I think MirSurfaceId is a reasonable name for it, client-side. I'm happy to change it, although it really is a client-side ID, so I might wait until others weigh in.

(4) /proc/self/exe is one of many bad options. For example, all Java applications have the same /proc/self/exe (that of the JVM binary). This applies to Mono, Python, shell scripts, …

[Robert Carr (robertcarr)]

Looks basically ok to me though client API requires docs.

I have some questions about how we are going to implement persistence across shell restarts and if the client API is really sufficient though, lets talk on IRC perhaps?

[Robert Carr (robertcarr)]

Understand how the persistence model will work now:

16:32 < RAOF> racarr: Yo!
16:34 < racarr> YO
16:34 < RAOF> You wanted to talk about persistence across shell restarts and such?
16:35 < racarr> RAOF: Ah yes...uh...so how is that going to work? Wont the API have to be like
16:35 < racarr> well involve the client having to provide some sort of cookie
16:35 < racarr> otherwise yeah how to differentiate between multiple java apps etc
16:35 < RAOF> Yeah, this would be more obvious if I'd actually documented the client API, wouldn't it :)
16:36 < RAOF> And also not included bits that should be in a subsequent MP.
16:36 < racarr> ?
16:36 < RAOF> Well, for example, mir_surface_spec_set_id() doesn't do anything, nor does id_from_string() or as_string().
16:37 < RAOF> The reason being that I removed the acceptance tests which used them in order to do the minimal *get* an ID thing.
16:38 < RAOF> Anyway; my high-level idea of this is that any client which wishes to restore its surface location/state across starts needs to
              first acquire an ID for the relevant surface and save it's serialised representation somewhere.
16:39 < RAOF> Then, when it's next starting up, before it creates the relevant window it deserialises the ID it got and adds the ID to the
              surface spec.
16:39 < RAOF> If there's currently no surface with that ID active, the PersistentSurfaceStore will return the relevant dodads, and the new
              surface will reclaim the old ID.
16:40 < racarr> oh ok you add the ID to the surface spec
16:40 < RAOF> 43I+bool mir_surface_spec_set_id(MirSurfaceSpec* spec, MirSurfaceId* id);
16:40 < RAOF> :)
16:40 < RAOF> It's not hooked up to anything, but it's (accidentally!) there :)
16:40 < racarr> yeah I guess I had a different mental model so I
16:40 < racarr> filtered that out

[Daniel van Vugt (vanvugt)]

Already confused by whether you mean internal integer ID by "ID" or MirSurfaceId. Please fix (3) ASAP. Having two broadly unrelated meanings of "surface ID" is going to keep biting us.

(1) What I mean is consider if you have only one app installed and do this:
  * Open one instance A. Close instance A.
  * Open two instances B and C. Close instances B and C.
  * Open one instance D.
Now exactly which instances have the same MirSurfaceId's? They can't all be different as that would defeat the purpose of the exercise.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2508
http://jenkins.qa.ubuntu.com/job/mir-ci/3671/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2258/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2257/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2208/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1668/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2208/console

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

On Fri, May 1, 2015 at 12:41 PM, Daniel van Vugt
<email address hidden> wrote:
> Already confused by whether you mean internal integer ID by "ID" or
> MirSurfaceId. Please fix (3) ASAP. Having two broadly unrelated
> meanings of "surface ID" is going to keep biting us.

In this we'll assume that this app wants to do surface position/state
restore, and so is calling mir_surface_request_persistent_id...

>
> (1) What I mean is consider if you have only one app installed and do
> this:
> * Open one instance A. Close instance A.

Instance A opens surface A' and requests an ID A*, which it serialises
to disc.

> * Open two instances B and C. Close instances B and C.

App instance B loads the serialised MirSurfaceId A* from disc, sets it
on the MirSurfaceSpec, and creates surface B'. Since there is no
current surface with Id A*, the MirSurfaceId of B' is A* and it gets
the state restoration treatment. App instance B calls
request_persistent_id, gets A* back and serialises it to disc.

App instance C loads the serialised MirSurfaceId A* from disc, sets it
on the MirSurfaceSpec, and creates surface C'. Since there *is* a
current surface with Id A*, C' gets a new MirSurfaceId C* and does not
get the state restoration treatment. App instance C calls
request_persistent_id, gets back C*, and serialises it to disc.

>
> * Open one instance D.
> Now exactly which instances have the same MirSurfaceId's? They can't
> all be different as that would defeat the purpose of the exercise.

App instance D loads the serialised MirSurfaceId C* from disc, sets it
on the MirSurfaceSpec, and creates surface D'. Since there is no
current surface with Id C*, D' gets Id C* and has C's state restored.

Note that more sophisticated clients could do more interesting things.
For example, if the application is a document editor then it can store
the serialised MirSurfaceId alongside the document and restore that
when appropriate. In that case, if instances A, C, D were opening the
same document and instance B opened a different document, A', C', and
D' would share ID and state, and B' would get a different ID and state.

[Chris Halse Rogers (raof)]

Oh, and FWIW - I've had some discussions with the GNOME Wayland guys, and got a vague “yeah, that seems sensible” about sticking the relevant client API into GTK.

[Daniel van Vugt (vanvugt)]

(0) I'm concerned that we might have missed initial analysis/requirements. Last time I discussed all this with sabdfl I convinced him at the time that remembering things was a bad idea (based on complaints from users in Compiz bugs that windows opened on other desktops they had forgotten about). So the whole exercise of remembering anything is questionable, but that's not a blocking issue if "remembering" is an option you can turn off easily.

(1) OK so indeed there is only one MirSurfaceId being shared by the multiple instances of an app, as expected. Just confused because that's the opposite of your first reply to (1) depending on the meaning of "ID" (see (3)).

(2) Opt-in concerns me still. Is it really OK that only some toolkits/apps will have a nice unique UUID?

(3) I still vote for s/MirSurfaceId/MirSurfaceSignature/ as this discussion is suffering from the confusion of the current name already. Or s/Signature/Fingerprint/.

(4) You could solve the /proc/self/exe problem by using "/proc/self/exe + /proc/self/cmdline" instead. That would be different between Java apps, but also consistent and stable between shell-launched app instances.

(5) Mention of the word "disc" and files is also a concern. Last time those words came up, the consensus was that Mir should never touch disc/files because the context in which Mir is used will vary between shells and we don't want a shell having settings spread between multiple repositories. A more elegant solution is for Mir to specify just the serialization format (ie. a string) and let the shell choose the storage location (e.g. QSettings/GSettings/file) if any.

review: Needs Fixing

[Chris Halse Rogers (raof)]

On Fri, May 1, 2015 at 3:02 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Needs Fixing
>
> (0) I'm concerned that we might have missed initial
> analysis/requirements. Last time I discussed all this with sabdfl I
> convinced him at the time that remembering things was a bad idea
> (based on complaints from users in Compiz bugs that windows opened on
> other desktops they had forgotten about). So the whole exercise of
> remembering anything is questionable, but that's not a blocking issue
> if "remembering" is an option you can turn off easily.

Relevant bit of the Mir Surface spec:

https://docs.google.com/document/d/1L85DdfDd3lDbvchYbgQ45C_lJ1IeTMG4uc7Nuq_XdAE/edit#heading=h.c76tva1odjo

> (1) OK so indeed there is only one MirSurfaceId being shared by the
> multiple instances of an app, as expected. Just confused because
> that's the opposite of your first reply to (1) depending on the
> meaning of "ID" (see (3)).

I didn't understand the question in the same way you did. I thought it
was talking about multiple *concurrent* instances of an application; in
this case they do indeed get different IDs.

Multiple sequential instances of an application may share a surface ID,
as indeed that's one of the use-cases.

> (2) Opt-in concerns me still. Is it really OK that only some
> toolkits/apps will have a nice unique UUID?

Yes. This is no worse than it is today in X. And should be better,
because we'll actually be able to *use* the information the client
opts-into.

> (3) I still vote for s/MirSurfaceId/MirSurfaceSignature/ as this
> discussion is suffering from the confusion of the current name
> already. Or s/Signature/Fingerprint/.

I still don't particularly like either Signature or Fingerprint, but am
willing to be outvoted.

>
> (4) You could solve the /proc/self/exe problem by using
> "/proc/self/exe + /proc/self/cmdline" instead. That would be
> different between Java apps, but also consistent and stable between
> shell-launched app instances.

It is not stable between shell-launched app instances, at least if you
include content-hub as a part of the shell.

A substantial number of applications are not launched directly by the
shell, and it'd be particularly weird if, for example, your email app
consistently opened in a certain place when launched from the launcher
but in a different place when launched from a notification.

>
> (5) Mention of the word "disc" and files is also a concern. Last time
> those words came up, the consensus was that Mir should never touch
> disc/files because the context in which Mir is used will vary between
> shells and we don't want a shell having settings spread between
> multiple repositories. A more elegant solution is for Mir to specify
> just the serialization format (ie. a string) and let the shell choose
> the storage location (e.g. QSettings/GSettings/file) if any.

Every mention of “disc” in the example was a *client* writing to
disc.

In the current implementation there's (a) no write to disc shell-side,
and (b) a shell is perfectly able to implement the whole
PersistentSurfaceStore and its associated ID with whatever it likes; if
it w...

[Chris Halse Rogers (raof)]

Hm. How about MirReference, rather than MirSurfaceId?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2511
http://jenkins.qa.ubuntu.com/job/mir-ci/3679/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2271/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2270
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2219/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1676/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2219/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2512
http://jenkins.qa.ubuntu.com/job/mir-ci/3680/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2272/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2271
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2220
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1677
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1677/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2220
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2220/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5126
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20154

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2513
http://jenkins.qa.ubuntu.com/job/mir-ci/3686/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2280/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2279
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2228
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1683
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1683/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2228
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2228/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5133
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20166

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

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

 >>string, stored or sent to another process, and then later deserialised to refer to the same
 >> MirSurface.

Or rather can be used to indicate that a new MirSurface refers to the same application window?

[Robert Carr (robertcarr)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2514
http://jenkins.qa.ubuntu.com/job/mir-ci/3702/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2298
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2297
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2246
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1699
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1699/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2246
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2246/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5146
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20181

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Needs Fixing (nit):

222+MirReference::MirReference(std::string const& string_id)
223+ : string_id{string_id}
...
910- explicit FrontendShell(std::shared_ptr<shell::Shell> const& wrapped) :
911- wrapped{wrapped} {}
912+ explicit FrontendShell(std::shared_ptr<shell::Shell> const& wrapped,
913+ std::shared_ptr<shell::PersistentSurfaceStore> const& surface_store)
914+ : wrapped{wrapped},
915+ surface_store{surface_store}

etc.

Not the approved layout:

http://unity.ubuntu.com/mir/cppguide/index.html?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists

~~~~

Needs clarification:

86+/**
87+ * \brief Free a MirReference
88+ * \param [in] id The MirReference to free
89+ */
90+void mir_reference_release(MirReference* id);

This is just freeing the local resource? It doesn't purge knowledge of the reference from the server?

~~~~

[non blocking opinion] I'm not convinced that "Reference" is as good a name as "ID".

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> Not the approved layout:
>
> http://unity.ubuntu.com/mir/cppguide/index.html?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists

Our code base contains some of the approved style and much more of the traditional style (i.e. what is used in this MP). I personally like the traditionally style better because with it a multi-line constructor parameter list is visually distinct from the initializer list [1]. Perhaps we should revisit the decision.

[1] http://paste.ubuntu.com/10990296/

[Chris Halse Rogers (raof)]

If you'd like to *ensure* that the style used is consistent, there's always https://code.launchpad.net/~raof/mir/clang-formatting-test/+merge/258219 :)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2518
http://jenkins.qa.ubuntu.com/job/mir-ci/3727/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2326
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2325
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2274
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1724
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1724/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2274
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2274/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5167
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20210

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

(3) I agree MirSurfaceId probably needs to begin with "MirSurface" but the "Id" part is easily confused with surface IDs we already have in the server code and even visible in the client debug API. Still think MirSurfaceSignature or MirSurfaceFingerprint is better. Although come to think of it, any generic persistent identifier doesn't have to be just used for surfaces so maybe "Surface" should not be mentioned; just "MirSignature" or "MirFingerprint".

(4) "/proc/self/exe + /proc/self/cmdline + (subwindow ID?)" instead of UUID would not only work, but it would avoid the need to generate UUIDs somewhere and would solve problem (2) instead of declaring it unsolvable. The issue of where an app was spawned from changing its ID is not an issue. The vast majority of the time a user will launch an app in the same way yielding the same ID. Arguably it's even a feature for different launchers to yield different restore positions. Using UUIDs prevents us from being able to have state memories work for 100% of apps (issue (2) is never solved). At least as a fallback from UUIDs (apps/toolkits that don't implement them) it would be a good idea.

review: Needs Fixing

[Chris Halse Rogers (raof)]

On Wed, May 6, 2015 at 4:24 PM, Daniel van Vugt
<email address hidden> wrote:
> (4) "/proc/self/exe + /proc/self/cmdline + (subwindow ID?)" instead
> of UUID would not only work, but it would avoid the need to generate
> UUIDs somewhere and would solve problem (2) instead of declaring it
> unsolvable. The issue of where an app was spawned from changing its
> ID is not an issue. The vast majority of the time a user will launch
> an app in the same way yielding the same ID.

Actually, I think that for all but one of the applications I have open
*right now* /proc/self/cmdline will give the wrong result - Chrome has
been opened from xdg-open <some link>, which is not stable, QtCreator
has been opened with a file, but probably should not change positions
based on the initially opened file, geary was launched with arguments
from the indicator, …

One of the reasons why no major X window manager restores window state
is that the heuristics available are wrong too often to be useful.

I think that the foundation of your argument - that the user will
almost always launch an app with the same /proc/self/cmdline - is false.

[Andreas Pokorny (andreas-pokorny)]

looks good
nit:two nl in frontend_shell.cpp 876 and 891

review: Approve

[Daniel van Vugt (vanvugt)]

(3) Specifically you now have these two types:
   mir::frontend::SurfaceId
   MirSurfaceId
which while they sound similar are vastly different. So a different name is required to avoid any confusion. Even in a semi-public client API we already have something called surface ID:
   int mir_debug_surface_id(MirSurface *surface);

(4) That's a reasonable argument. Also given item (0) people such as myself will disable the feature anyway so will have nothing to complain about. And so long as MirSurfaceId remains an opaque string the implementation is flexible.

[Alan Griffiths (alan-griffiths)]

I approve of the design. A few nits:

217+#include <uuid/uuid.h>
...
289+#include <uuid/uuid.h>

unused includes

~~~~

205+ ${UUID_LDFLAGS} ${UUID_LIBRARIES}

Dependency in the wrong place - it is libmirserver that uses uuid_*

~~~~

1063+ msh::DefaultPersistentSurfaceStore map;

"map" seems to reflect the implementation, not the usage. Would "store" or "pss" be better?

~~~~

225+MirSurfaceId::MirSurfaceId(std::string const& string_id)226+ : string_id{string_id}

Not the approved layout:

http://unity.ubuntu.com/mir/cppguide/index.html?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists

review: Needs Fixing

[Kevin DuBois (kdub)]

lgtm. I kept thinking that MirSurfaceTag might be another good name, but I'm okay with Id as well.
652+ UUID(UUID const& copy_from);
could have assignment defined too.

review: Approve

[Alberto Aguirre (albaguirre)]

s/serialise/serialize?
s/serialise/serialize

+ what Alan said.

review: Needs Fixing

[Chris Halse Rogers (raof)]

Client type naming poll: I'm ok with MirSurfaceTag. I prefer the newly-thought-of MirSurfaceTicket.

Also ok with MirTag or MirTicket if we wanted the ability to be lazy in future and use these as generic identifiers; I'm weakly opposed to this on meaningful-type grounds.

[Alan Griffiths (alan-griffiths)]

> Client type naming poll: I'm ok with MirSurfaceTag. I prefer the newly-
> thought-of MirSurfaceTicket.
>
> Also ok with MirTag or MirTicket if we wanted the ability to be lazy in future
> and use these as generic identifiers; I'm weakly opposed to this on
> meaningful-type grounds.

I'm happy with MirSurfaceID.

If we actually think there is a chance of confusion between the useful-to-the-client MirSurfaceID and the mostly-internal mir::frontend::SurfaceId I think the latter should be rethought.

[Alexandros Frantzis (afrantzis)]

My preference is for MirSurfaceSignature or MirSurfaceFingerprint or similar. In this context, I don't think the overly general term "ID" properly stresses the most important characheristic of the value, which is its persistence. If I saw the term "surface id" in some foreign code, a persistent, stable-between-invocations value would not be high on the list of concepts I would expect that term to represent.

[Alberto Aguirre (albaguirre)]

MirPersistentID?
MirCookie?

But I'm mostly ok with MirSurfaceID. MirSurfaceTag would be fine too.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2522
http://jenkins.qa.ubuntu.com/job/mir-ci/3790/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2417
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2416
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2365
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1787
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1787/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2365
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2365/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5245
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20348

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Also against the name "MirSurfaceId" is the widespread knowledge people have about windowing systems where window instances usually have some sort of integer ID (as they do in Mir too). But that's definitely not what this is about.

Preferences (in order):
  MirSurfaceSignature
  MirSurfaceFingerprint
  MirSurfaceT{ag,icket}

[Alan Griffiths (alan-griffiths)]

145+class PersistentSurfaceStore

Should disable CopyAssign

~~~~

168+ /**
169+ * \brief Lookup Surface by ID.
170+ * \param [in] id ID of surface to lookup
171+ * \return The surface with ID \arg id.
172+ */
173+ virtual std::shared_ptr<scene::Surface> surface_for_id(Id const& id) const = 0;

What happens if, for example, the surface has been closed since the ID was created? I'd guess a nullptr result but it needs to be documented.

~~~~

181+ * \throw std::out_of_range if \arg buffer contains a valid serialized ID, but
182+ * the PersistentSurfaceStore has no Surface with that ID.

Is this actually a post-condition failure? It seems more like a possible result: AFAICS this depends on the state of external actions (e.g. the client innocently has an ID from a store that was wiped between server sessions, or for a surface that has been closed).

Perhaps the problem is that I'm not seeing the use(s) of this feature the same way that you are: surely, for the user, the point is whether the store can provide the linked data (either a surface or some surface properties) not whether the ID is recognized.

In that model is seems more reasonable for the deserialize_id() to succeed and the subsequent surface_for_id() to return nullptr. (And if that is right then maybe Ids should be a value type that validates on construction without needing to be constructed by the store?)

~~~~

Something that doesn't appear to be addressed is the destruction of IDs. Is there a mechanism for this? I realize that not invalidating IDs avoids invalid references being passed, but it seems wrong for a "persistent store" to lack such a mechanism.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

148+ /**
149+ * An opaque identifier tied to this PersistentSurfaceStore
150+ */
151+ class Id
152+ {
153+ public:
154+ virtual ~Id() = default;
155+ virtual bool operator==(Id const& rhs) const = 0;
156+ };
...
167+ virtual Id const& id_for_surface(std::shared_ptr<scene::Surface> const& surface) = 0;
...
173+ virtual std::shared_ptr<scene::Surface> surface_for_id(Id const& id) const = 0;

I guess it is techincally OK (as we can't create sliced objects because of the pure member) but use of an interface combined passing references and allowing CopyAssign seems like a confusion of value and instance semantics. (I'd much rather see either a pointer or a value type being passed.)

[Daniel van Vugt (vanvugt)]

Yet another pre-existing definition of SurfaceId:
    mir_protobuf.proto:message SurfaceId {

Best not to keep using the same name for different unrelated things.

[Alan Griffiths (alan-griffiths)]

For the moment just a critique of the class Id semantics. Shall we sort those first?

160 + /**
161 + * \brief Acquire ID for a Surface
162 + * \param [in] surface to query or generate an ID for
163 + * \return A reference to the ID of this surface. This reference is stable
164 + * for the lifetime of the PersistentSurfaceStore.
165 + * \note If \arg surface has not yet had an ID generated, this generates its ID.
166 + */
167 + virtual Id const& id_for_surface(std::shared_ptr<scene::Surface> const& surface) = 0;

Requiring the reference to remain valid is reasonable for the user, but it requires that every ID ever referenced remains in memory. For a persistant surface store that persists to disk this could be inconvenient.

The API is also error prone:

   auto const id = ps->id_for_surface(surface); // Captures by value
   ps->surface_for_id(id); // undefined behaviour

(UB because, e.g. the default implementation casts the passed reference to a UUID.)

We could break this example by giving Id a private destructor, but I think it would be better to return a value.

Making Id a value type admittedly doesn't allow the store to chose its own ID implementation, but I think is a better trade-off.

~~~~

168 + /**
169 + * \brief Lookup Surface by ID.
170 + * \param [in] id ID of surface to lookup
171 + * \return The surface with ID \arg id.
172 + */
173 + virtual std::shared_ptr<scene::Surface> surface_for_id(Id const& id) const = 0;

What happens if, for example, the surface has been closed since the ID was created? I'd guess a nullptr result but it needs to be documented.

~~~~

184 + virtual Id const& deserialize_id(std::vector<uint8_t> const& buffer) const = 0;
...
191 + virtual std::vector<uint8_t> serialize_id(Id const& id) const = 0;

Do these need to be on the PersistentSurfaceStore interface? Or could they be functions on Id?

review: Needs Fixing

[Chris Halse Rogers (raof)]

On Wed, May 20, 2015 at 9:46 AM, Alan Griffiths <email address hidden>
wrote:
> Review: Needs Fixing
>
> For the moment just a critique of the class Id semantics. Shall we
> sort those first?
>
> 160 + /**
> 161 + * \brief Acquire ID for a Surface
> 162 + * \param [in] surface to query or generate an ID for
> 163 + * \return A reference to the ID of this surface.
> This reference is stable
> 164 + * for the lifetime of the
> PersistentSurfaceStore.
> 165 + * \note If \arg surface has not yet had an ID generated,
> this generates its ID.
> 166 + */
> 167 + virtual Id const&
> id_for_surface(std::shared_ptr<scene::Surface> const& surface) = 0;
>
> Requiring the reference to remain valid is reasonable for the user,
> but it requires that every ID ever referenced remains in memory. For
> a persistant surface store that persists to disk this could be
> inconvenient.
>
> The API is also error prone:
>
> auto const id = ps->id_for_surface(surface); // Captures by value
> ps->surface_for_id(id); // undefined
> behaviour
>
> (UB because, e.g. the default implementation casts the passed
> reference to a UUID.)
>
> We could break this example by giving Id a private destructor, but I
> think it would be better to return a value.
>
> Making Id a value type admittedly doesn't allow the store to chose
> its own ID implementation, but I think is a better trade-off.

Yeah, I'm part-way through making Id a concrete value type backed by a
uuid_t. Stores will need to deal with it.

>
>
> ~~~~
>
> 168 + /**
> 169 + * \brief Lookup Surface by ID.
> 170 + * \param [in] id ID of surface to lookup
> 171 + * \return The surface with ID \arg id.
> 172 + */
> 173 + virtual std::shared_ptr<scene::Surface> surface_for_id(Id
> const& id) const = 0;
>
> What happens if, for example, the surface has been closed since the
> ID was created? I'd guess a nullptr result but it needs to be
> documented.

Either that or throwing; I guess returning nullptr is more idiomatic.

Will document.
>
> ~~~~
>
> 184 + virtual Id const& deserialize_id(std::vector<uint8_t> const&
> buffer) const = 0;
> ...
> 191 + virtual std::vector<uint8_t> serialize_id(Id const& id)
> const = 0;
>
> Do these need to be on the PersistentSurfaceStore interface? Or could
> they be functions on Id?

Yeah, now that Id is concrete they're moving to it.

[Chris Halse Rogers (raof)]

It is resolved!

MirPersistentId is to be the name.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2529
http://jenkins.qa.ubuntu.com/job/mir-ci/3943/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2627
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2575
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/99
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/99/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2575
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2575/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5425
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20833

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

A mix *Needs Discussion* and Nits:

If users are to customize the persistent id store then this needs to be configurable through mir::Server. I presume that (and corresponding acceptance) happens in the second half of persistent surface reference support?

~~~~~

168 + * \throws std::out_of_range if there is no surface with \arg id.

Is this really a postcondition failure?

I would have expected it to be "normal" not "exceptional" for an ID key supplied by another process to have no matching surface. (E.g. because the surface closed since the ID was generated.) AFAICS there is no way to "validate" the ID before making this call.

~~~~

162 + virtual Id id_for_surface(std::shared_ptr<scene::Surface> const& surface) = 0;
163 + /**

Could probably do with another newline.

~~~~

196 + std::vector<uint8_t> serialize_id() const;
197 + static Id deserialize_id(std::vector<uint8_t> const& buffer);

These are members of Id, so "_id" is unnecessary repetition.

~~~~

202 + Id(std::array<char, 37> const& buffer);

Using std::array<char, ...> in the private section seems odd when the public interface is std::vector<uint8_t>. This leads to this odd copying:

919 + std::array<char, 37> null_terminated_buffer;
920 + std::copy(buffer.begin(), buffer.end(), null_terminated_buffer.data());
921 + null_terminated_buffer[36] = '\0';

I'm not sure why we don't just have a public constructor with the same parameter and semantics as deserialize_id()? (Nor why we use char internally and uint8_t publicly. Indeed, isn't std::string a better fit?)

~~~~

261 +MirWaitHandle* MirSurface::request_persistent_id(mir_surface_id_callback callback, void* context)
...
278 + surface.set_error(std::string{"Error invoking create surface: "} +
279 + boost::diagnostic_information(ex));

"Error invoking create surface" is misleading.

~~~~

793 +

Spurious whitespace

review: Needs Information

[Chris Halse Rogers (raof)]

On Mon, Jun 1, 2015 at 7:47 PM, Alan Griffiths <email address hidden>
wrote:
> Review: Needs Information
>
> A mix *Needs Discussion* and Nits:
>
> If users are to customize the persistent id store then this needs to
> be configurable through mir::Server. I presume that (and
> corresponding acceptance) happens in the second half of persistent
> surface reference support?

I think it'll actually happen later; there's no reason for shells to
want to configure this until there's a client API that uses
across-shell-restart persistence. The immediate follow up branch -
input-methods-can-specify-foreign-parents - has no need for shell
customisation of the store.

>
> ~~~~~
>
> 168 + * \throws std::out_of_range if there is no surface with
> \arg id.
>
> Is this really a postcondition failure?
>
> I would have expected it to be "normal" not "exceptional" for an ID
> key supplied by another process to have no matching surface. (E.g.
> because the surface closed since the ID was generated.) AFAICS there
> is no way to "validate" the ID before making this call.

In the case of a closed surface the method is documented as returning
nullptr.

Generally, I think it's worth being able to distinguish between “this
client sent you garbage” and “this client was the victim of an
unavoidable race condition”. It's reasonable to kill the client in
the former, but not the latter.

>
> 202 + Id(std::array<char, 37> const& buffer);
>
> Using std::array<char, ...> in the private section seems odd when the
> public interface is std::vector<uint8_t>. This leads to this odd
> copying:
>
> 919 + std::array<char, 37> null_terminated_buffer;
> 920 + std::copy(buffer.begin(), buffer.end(),
> null_terminated_buffer.data());
> 921 + null_terminated_buffer[36] = '\0';
>
> I'm not sure why we don't just have a public constructor with the
> same parameter and semantics as deserialize_id()? (Nor why we use
> char internally and uint8_t publicly. Indeed, isn't std::string a
> better fit?)

I don't think std::string is a good fit for the
serialisation/deserialisation API.

I like vector<uint8_t> as it makes it clear that we're dealing with
blobs of data, not strings.

Hm. That said, serialising to a string rather than bytestream is a
perfectly natural thing to do, and is how we currently use these. I'll
rename to serialize_to_string() and return a std::string, and take a
std::string for deserialisation.

[Daniel van Vugt (vanvugt)]

Seems like my primary concern is addressed. Haven't re-reviewed yet but I don't need to block this.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2538
http://jenkins.qa.ubuntu.com/job/mir-ci/3968/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2656
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/167
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2604
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/124
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/124/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2604
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2604/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5453
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20882

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> > I would have expected it to be "normal" not "exceptional" for an ID
> > key supplied by another process to have no matching surface. (E.g.
> > because the surface closed since the ID was generated.) AFAICS there
> > is no way to "validate" the ID before making this call.
>
> In the case of a closed surface the method is documented as returning
> nullptr.
>
> Generally, I think it's worth being able to distinguish between “this
> client sent you garbage” and “this client was the victim of an
> unavoidable race condition”. It's reasonable to kill the client in
> the former, but not the latter.

It sounds as though I've been confused by the term "persistent" - I was thinking the ID might be used across sequential executions of the same client[1]. In which case the client can't know if the server happens to have been restarted (and lost its list of closed surface IDs).

Are these actually serializable IDs rather than persistent ones?

[1] I know that identifying surfaces doesn't make sense across different executions of a client, but we do have use cases (the WM storing defaults for a surface) where a persistent ID is needed. Are we going to have a separate "persistent surface properties ID" for that?

review: Needs Information

[Chris Halse Rogers (raof)]

No, I don't think you've been confused, but perhaps the comment is a little confusing. Or maybe the partially completed code is a bit confusing.

The PersistentSurfaceStore will need to grow a separate method for the different-executions-of-a-client case - surface_for_id can't return a std::shared_ptr<Surface> for the get-last-used-defaults case, because the whole point is that there isn't a currently active Surface with that id.

At the moment they are indeed serializable IDs; when we implement the persistent bit they'll become persistent :).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2539
http://jenkins.qa.ubuntu.com/job/mir-ci/3982/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2676/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/188/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2624/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/138/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2624/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2540
http://jenkins.qa.ubuntu.com/job/mir-ci/3983/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2677
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/189
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2625/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/139
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/139/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2625
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2625/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5472/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20915

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2540
http://jenkins.qa.ubuntu.com/job/mir-ci/3984/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2678
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/190
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2626/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/140
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/140/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2626
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2626/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5473/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20916

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

So, six minutes to start glmark and then timed out after an hour. Probably unrelated to these changes.

[timestamp] Start time : 2015-06-03T08:47:27+0000
...
[timestamp] Start : mir_performance_tests 2015-06-03T08:53:52+0000
+ grep -q mir_demo_client_
+ grep -q mir_privileged_tests
+ phablet-test-run -x -s 00693fd555c9186a -v umockdev-run -- mir_performance_tests
running umockdev-run -- mir_performance_tests
Running main() from command_line_server_configuration.cpp
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from GLMark2Test
[ RUN ] GLMark2Test.benchmark_fullscreen_default
[1433321633.435408] mirplatform: Found graphics driver: dummy
[1433321633.438794] mirplatform: Found graphics driver: android
[1433321633.439770] mirplatform: Found graphics driver: mesa
Build timed out (after 60 minutes). Marking the build as failed.
Build was aborted

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2540
http://jenkins.qa.ubuntu.com/job/mir-ci/4002/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2703
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/216
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2651/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/158
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/158/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2651
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2651/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5496/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20953

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

What the hell, mako?

/me fires up his mako to test locally.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2540
http://jenkins.qa.ubuntu.com/job/mir-ci/4019/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2730
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/243
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2678/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/175
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/175/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2678
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2678/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5521/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20983

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

Ok. This works fine on *my* mako. Let's just try prodding CI again.

[Chris Halse Rogers (raof)]

...!

Ok. So, on more detailed inspection this *does* fail on my mako. glmark2-es2-mir, and as far as I've tested, *only* glmark2-es2-mir hangs in a syscall before displaying anything. mir_demo_client_multiwin works, as does mir_demo_clent_eglplasma and eglflash.

Um...

[Chris Halse Rogers (raof)]

Oh, that's fun.

So, this is libmirprotobuf0 breakage. Because I've added an RPC call this has re-arranged the protobuf::DisplayServer vtable so when libmirclient8 calls what it thinks is server->exchange_buffer it's actually calling server->stop_prompt_session.

On desktop it just happens to work.

Now, I've moved the request_persistent_id to the end of the DisplayServer .proto definition. Hopefully that'll get protoc to generate a class with request_persistent_id at the end. Although we have derived classes of DisplayServer, they're defined in libmirprotobuf0, so this might work!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2542
http://jenkins.qa.ubuntu.com/job/mir-ci/4040/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2761
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/274
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2709/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/196
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/196/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2709
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2709/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5549/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21029

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

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

^--Looks like device setup failure - not related to this MP. Retriggering.

[Alberto Aguirre (albaguirre)]

LGTM.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2542
http://jenkins.qa.ubuntu.com/job/mir-ci/4050/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2774
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2722
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/206
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/206/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2722
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2722/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5562
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21054

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

review: Approve (continuous-integration)

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-06-02 19:34:13 +0000
+++ CMakeLists.txt	2015-06-09 07:21:31 +0000
@@ -173,6 +173,7 @@
 find_package(LTTngUST REQUIRED)
 pkg_check_modules(UDEV REQUIRED libudev)
 pkg_check_modules(GLIB REQUIRED glib-2.0)
+pkg_check_modules(UUID REQUIRED uuid)
 
 include_directories (${GLESv2_INCLUDE_DIRS})
 include_directories (${EGL_INCLUDE_DIRS})

=== modified file 'debian/control'
--- debian/control	2015-06-08 08:48:29 +0000
+++ debian/control	2015-06-09 07:21:31 +0000
@@ -40,6 +40,7 @@
                libfreetype6-dev,
                abi-compliance-checker,
                libevdev-dev,
+               uuid-dev,
 Standards-Version: 3.9.4
 Homepage: https://launchpad.net/mir
 # If you aren't a member of ~mir-team but need to upload packaging changes,

=== modified file 'include/client/mir_toolkit/client_types.h'
--- include/client/mir_toolkit/client_types.h	2015-05-26 22:56:02 +0000
+++ include/client/mir_toolkit/client_types.h	2015-06-09 07:21:31 +0000
@@ -43,6 +43,7 @@
 typedef struct MirScreencast MirScreencast;
 typedef struct MirPromptSession MirPromptSession;
 typedef struct MirBufferStream MirBufferStream;
+typedef struct MirPersistentId MirPersistentId;
 
 /**
  * Returned by asynchronous functions. Must not be free'd by
@@ -123,6 +124,10 @@
 typedef void (*mir_client_fd_callback)(
     MirPromptSession *prompt_session, size_t count, int const* fds, void* context);
 
+
+typedef void (*mir_surface_id_callback)(
+    MirSurface* surface, MirPersistentId* id, void* context);
+
 /**
  * MirBufferUsage specifies how a surface can and will be used. A "hardware"
  * surface can be used for OpenGL accelerated rendering. A "software" surface

=== modified file 'include/client/mir_toolkit/mir_surface.h'
--- include/client/mir_toolkit/mir_surface.h	2015-05-27 11:37:38 +0000
+++ include/client/mir_toolkit/mir_surface.h	2015-06-09 07:21:31 +0000
@@ -582,6 +582,46 @@
  */
 void mir_surface_apply_spec(MirSurface* surface, MirSurfaceSpec* spec);
 
+/**
+ * \brief Request an ID for the surface that can be shared cross-process and
+ *        across restarts.
+ *
+ * This call acquires a MirPersistentId for this MirSurface. This MirPersistentId
+ * can be serialized to a string, stored or sent to another process, and then
+ * later deserialized to refer to the same surface.
+ *
+ * \param [in]     surface   The surface to acquire a persistent reference to.
+ * \param [in]     callback  Callback to invoke when the request completes.
+ * \param [in,out] context   User data passed to completion callback.
+ * \return A MirWaitHandle that can be used in mir_wait_for to await completion.
+ */
+MirWaitHandle* mir_surface_request_persistent_id(MirSurface* surface, mir_surface_id_callback callback, void* context);
+
+/**
+ * \brief Request a persistent ID for a surface and wait for the result
+ * \param [in] surface  The surface to acquire a persistent ID for.
+ * \return A MirPersistentId. This MirPersistentId is owned by the calling code, and must
+ *         be freed with a call to mir_persistent_id_release()
+ */
+MirPersistentId* mir_surface_request_persistent_id_sync(MirSurface *surface);
+
+/**
+ * \brief Check the validity of a MirPersistentId
+ * \param [in] id  The MirPersistentId
+ * \return True iff the MirPersistentId contains a valid ID value.
+ *
+ * \note This does not guarantee that the ID refers to a currently valid object.
+ */
+bool mir_persistent_id_is_valid(MirPersistentId* id);
+
+/**
+ * \brief Free a MirPersistentId
+ * \param [in] id  The MirPersistentId to free
+ * \note This frees only the client-side representation; it has no effect on the
+ *       object referred to by \arg id.
+ */
+void mir_persistent_id_release(MirPersistentId* id);
+
 #ifdef __cplusplus
 }
 /**@}*/

=== added file 'include/server/mir/shell/persistent_surface_store.h'
--- include/server/mir/shell/persistent_surface_store.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/shell/persistent_surface_store.h	2015-06-09 07:21:31 +0000
@@ -0,0 +1,131 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#ifndef MIR_SHELL_PERSISTENT_SURFACE_STORE_H_
+#define MIR_SHELL_PERSISTENT_SURFACE_STORE_H_
+
+#include <memory>
+#include <vector>
+#include <array>
+#include <uuid/uuid.h>
+
+
+namespace mir
+{
+namespace scene
+{
+class Surface;
+}
+
+namespace shell
+{
+/**
+ * A store for Surface information divorced from the lifetime of any given Session
+ *
+ * This provides the backing for persistent references to Surface objects,
+ * both across client Sessions and across shell restarts, for features such as
+ * surface position/size restoration.
+ *
+ * \todo Persistence across shell restarts is as-yet unimplemented.
+ */
+class PersistentSurfaceStore
+{
+public:
+    class Id;
+
+    virtual ~PersistentSurfaceStore() = default;
+
+    /**
+     * \brief Acquire ID for a Surface
+     * \param [in] surface Surface to query or generate an ID for
+     * \return             The ID for this surface.
+     * \note If \arg surface has not yet had an ID generated, this generates its ID.
+     * \note This does not extend the lifetime of \arg surface.
+     */
+    virtual Id id_for_surface(std::shared_ptr<scene::Surface> const& surface) = 0;
+
+    /**
+     * \brief Lookup Surface by ID.
+     * \param [in] id    ID of surface to lookup
+     * \return           The surface with ID \arg id. If this surface has been destroyed,
+     *                   but the store retains a reference, returns nullptr.
+     * \throws std::out_of_range if the store has no reference for a surface with \arg id.
+     */
+    virtual std::shared_ptr<scene::Surface> surface_for_id(Id const& id) const = 0;
+};
+}
+}
+
+namespace std
+{
+template<>
+struct hash<mir::shell::PersistentSurfaceStore::Id>;
+}
+
+namespace mir
+{
+namespace shell
+{
+
+class PersistentSurfaceStore::Id final
+{
+public:
+    /**
+     * \brief Generate a new, unique Id.
+     */
+    Id();
+
+    /**
+     * \brief Construct an Id from its serialized string form
+     * \param serialized_form [in] The previously-serialized Id
+     * \throw std::invalid_argument if \arg serialized_form is not parseable as an Id.
+     */
+    Id(std::string const& serialized_form);
+
+    Id(Id const& rhs);
+    Id& operator=(Id const& rhs);
+
+    bool operator==(Id const& rhs) const;
+
+    /**
+     * \brief Serialize to a UTF-8 string
+     * \return A string representation of the Id; this is guaranteed to be valid UTF-8
+     */
+    std::string serialize_to_string() const;
+private:
+    friend struct std::hash<Id>;
+
+    uuid_t uuid;
+};
+
+}
+}
+
+namespace std
+{
+template<>
+struct hash<mir::shell::PersistentSurfaceStore::Id>
+{
+    typedef mir::shell::PersistentSurfaceStore::Id argument_type;
+    typedef std::size_t result_type;
+
+    result_type operator()(argument_type const& uuid) const;
+};
+}
+
+#endif // MIR_SHELL_PERSISTENT_SURFACE_STORE_H_

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2015-05-21 19:25:51 +0000
+++ src/client/mir_surface.cpp	2015-06-09 07:21:31 +0000
@@ -127,6 +127,16 @@
     return message;
 }
 
+MirPersistentId::MirPersistentId(std::string const& string_id)
+    : string_id{string_id}
+{
+}
+
+std::string const&MirPersistentId::as_string()
+{
+    return string_id;
+}
+
 MirSurface::MirSurface(std::string const& error)
 {
     surface.set_error(error);
@@ -220,6 +230,42 @@
     return false;
 }
 
+void MirSurface::acquired_persistent_id(mir_surface_id_callback callback, void* context)
+{
+    if (!persistent_id.has_error())
+    {
+        callback(this, new MirPersistentId{persistent_id.value()}, context);
+    }
+    else
+    {
+        callback(this, nullptr, context);
+    }
+    persistent_id_wait_handle.result_received();
+}
+
+MirWaitHandle* MirSurface::request_persistent_id(mir_surface_id_callback callback, void* context)
+{
+    std::lock_guard<decltype(mutex)> lock{mutex};
+
+    if (persistent_id.has_value())
+    {
+        callback(this, new MirPersistentId{persistent_id.value()}, context);
+        return nullptr;
+    }
+
+    persistent_id_wait_handle.expect_result();
+    try
+    {
+        server->request_persistent_surface_id(0, &surface.id(), &persistent_id, gp::NewCallback(this, &MirSurface::acquired_persistent_id, callback, context));
+    }
+    catch (std::exception const& ex)
+    {
+        surface.set_error(std::string{"Failed to acquire a persistent ID from the server: "} +
+                          boost::diagnostic_information(ex));
+    }
+    return &persistent_id_wait_handle;
+}
+
 MirWaitHandle* MirSurface::get_create_wait_handle()
 {
     return &create_wait_handle;

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2015-05-21 19:25:51 +0000
+++ src/client/mir_surface.h	2015-06-09 07:21:31 +0000
@@ -99,6 +99,17 @@
     mir::optional_value<AspectRatio> max_aspect;
 };
 
+struct MirPersistentId
+{
+public:
+    MirPersistentId(std::string const& string_id);
+
+    std::string const& as_string();
+
+private:
+    std::string const string_id;
+};
+
 struct MirSurface
 {
 public:
@@ -153,18 +164,22 @@
     MirWaitHandle* modify(MirSurfaceSpec const& changes);
 
     static bool is_valid(MirSurface* query);
+
+    MirWaitHandle* request_persistent_id(mir_surface_id_callback callback, void* context);
 private:
     mutable std::mutex mutex; // Protects all members of *this
 
     void on_configured();
     void on_cursor_configured();
     void created(mir_surface_callback callback, void* context);
+    void acquired_persistent_id(mir_surface_id_callback callback, void* context);
     MirPixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf) const;
 
     mir::protobuf::DisplayServer::Stub* server{nullptr};
     mir::protobuf::Debug::Stub* debug{nullptr};
     mir::protobuf::Surface surface;
     mir::protobuf::BufferRequest buffer_request;
+    mir::protobuf::PersistentSurfaceId persistent_id;
     std::string error_message;
     std::string name;
     mir::protobuf::Void void_response;
@@ -178,6 +193,7 @@
     MirWaitHandle create_wait_handle;
     MirWaitHandle configure_wait_handle;
     MirWaitHandle configure_cursor_wait_handle;
+    MirWaitHandle persistent_id_wait_handle;
 
     std::shared_ptr<mir::client::ClientBufferStreamFactory> const buffer_stream_factory;
     std::shared_ptr<mir::client::ClientBufferStream> buffer_stream;

=== modified file 'src/client/mir_surface_api.cpp'
--- src/client/mir_surface_api.cpp	2015-05-27 11:37:38 +0000
+++ src/client/mir_surface_api.cpp	2015-06-09 07:21:31 +0000
@@ -562,3 +562,40 @@
     MIR_LOG_UNCAUGHT_EXCEPTION(ex);
     return false;
 }
+
+MirWaitHandle* mir_surface_request_persistent_id(MirSurface* surface, mir_surface_id_callback callback, void* context)
+{
+    mir::require(mir_surface_is_valid(surface));
+
+    return surface->request_persistent_id(callback, context);
+}
+
+namespace
+{
+void assign_surface_id_result(MirSurface*, MirPersistentId* id, void* context)
+{
+    void** result_ptr = reinterpret_cast<void**>(context);
+    *result_ptr = id;
+}
+}
+
+MirPersistentId* mir_surface_request_persistent_id_sync(MirSurface *surface)
+{
+    mir::require(mir_surface_is_valid(surface));
+
+    MirPersistentId* result = nullptr;
+    mir_wait_for(mir_surface_request_persistent_id(surface,
+                                                   &assign_surface_id_result,
+                                                   &result));
+    return result;
+}
+
+bool mir_persistent_id_is_valid(MirPersistentId* id)
+{
+    return id != nullptr;
+}
+
+void mir_persistent_id_release(MirPersistentId* id)
+{
+    delete id;
+}

=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2015-05-27 11:44:54 +0000
+++ src/client/symbols.map	2015-06-09 07:21:31 +0000
@@ -76,6 +76,8 @@
     mir_omnidirectional_resize_cursor_name;
     mir_open_hand_cursor_name;
     mir_orientation_event_get_direction;
+    mir_persistent_id_is_valid;
+    mir_persistent_id_release;
     mir_platform_message_create;
     mir_platform_message_get_data;
     mir_platform_message_get_fds;
@@ -118,6 +120,8 @@
     mir_surface_is_valid;
     mir_surface_release;
     mir_surface_release_sync;
+    mir_surface_request_persistent_id;
+    mir_surface_request_persistent_id_sync;
     mir_surface_set_event_handler;
     mir_surface_set_preferred_orientation;
     mir_surface_set_state;

=== modified file 'src/include/server/mir/default_server_configuration.h'
--- src/include/server/mir/default_server_configuration.h	2015-05-21 19:25:51 +0000
+++ src/include/server/mir/default_server_configuration.h	2015-06-09 07:21:31 +0000
@@ -83,6 +83,7 @@
 class DisplayLayout;
 class HostLifecycleEventListener;
 class Shell;
+class PersistentSurfaceStore;
 namespace detail { class FrontendShell; }
 }
 namespace time
@@ -268,6 +269,7 @@
     virtual std::shared_ptr<scene::PromptSessionListener> the_prompt_session_listener();
     virtual std::shared_ptr<scene::PromptSessionManager>  the_prompt_session_manager();
     virtual std::shared_ptr<shell::HostLifecycleEventListener> the_host_lifecycle_event_listener();
+    virtual std::shared_ptr<shell::PersistentSurfaceStore> the_persistent_surface_store();
 
     /** @} */
 
@@ -440,6 +442,7 @@
     CachedPtr<scene::CoordinateTranslator> coordinate_translator;
     CachedPtr<EmergencyCleanup> emergency_cleanup;
     CachedPtr<shell::HostLifecycleEventListener> host_lifecycle_event_listener;
+    CachedPtr<shell::PersistentSurfaceStore> surface_store;
     CachedPtr<SharedLibraryProberReport> shared_library_prober_report;
     CachedPtr<shell::Shell> shell;
 

=== modified file 'src/include/server/mir/frontend/shell.h'
--- src/include/server/mir/frontend/shell.h	2015-04-08 07:51:14 +0000
+++ src/include/server/mir/frontend/shell.h	2015-06-09 07:21:31 +0000
@@ -25,6 +25,7 @@
 #include <sys/types.h>
 
 #include <memory>
+#include <vector>
 
 namespace mir
 {
@@ -62,6 +63,7 @@
     virtual SurfaceId create_surface(std::shared_ptr<Session> const& session, scene::SurfaceCreationParameters const& params) = 0;
     virtual void modify_surface(std::shared_ptr<Session> const& session, SurfaceId surface, shell::SurfaceSpecification const& modifications) = 0;
     virtual void destroy_surface(std::shared_ptr<Session> const& session, SurfaceId surface) = 0;
+    virtual std::string persistent_id_for(std::shared_ptr<Session> const& session, SurfaceId surface) = 0;
 
     virtual int set_surface_attribute(
         std::shared_ptr<Session> const& session,

=== modified file 'src/protobuf/mir_protobuf.proto'
--- src/protobuf/mir_protobuf.proto	2015-06-05 06:35:11 +0000
+++ src/protobuf/mir_protobuf.proto	2015-06-09 07:21:31 +0000
@@ -85,6 +85,12 @@
   required int32 value = 1;
 };
 
+message PersistentSurfaceId {
+  optional string value = 1;
+
+  optional string error = 127;
+};
+
 message BufferStreamId {
   required int32 value = 1;
 };
@@ -321,7 +327,10 @@
   rpc start_prompt_session(PromptSessionParameters) returns (Void);
   rpc stop_prompt_session(Void) returns (Void);
   rpc exchange_buffer(BufferRequest) returns (Buffer);
+
   rpc submit_buffer(BufferRequest) returns (Void);
+
+  rpc request_persistent_surface_id(SurfaceId) returns (PersistentSurfaceId);
 }
 
 message CoordinateTranslationRequest {

=== modified file 'src/server/CMakeLists.txt'
--- src/server/CMakeLists.txt	2015-05-19 21:34:34 +0000
+++ src/server/CMakeLists.txt	2015-06-09 07:21:31 +0000
@@ -76,6 +76,7 @@
   ${GLESv2_LDFLAGS} ${GLESv2_LIBRARIES}
   ${UDEV_LDFLAGS} ${UDEV_LIBRARIES}
   ${GLIB_LDFLAGS} ${GLIB_LIBRARIES}
+  ${UUID_LDFLAGS} ${UUID_LIBRARIES}
 )
 
 set(MIR_SERVER_OBJECTS ${MIR_SERVER_OBJECTS} PARENT_SCOPE)
@@ -101,6 +102,7 @@
   ${GLESv2_LDFLAGS} ${GLESv2_LIBRARIES}
   ${UDEV_LDFLAGS} ${UDEV_LIBRARIES}
   ${GLIB_LDFLAGS} ${GLIB_LIBRARIES}
+  ${UUID_LDFLAGS} ${UUID_LIBRARIES}
 )
 
 install(TARGETS mirserver

=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2015-06-08 13:43:27 +0000
+++ src/server/frontend/protobuf_message_processor.cpp	2015-06-09 07:21:31 +0000
@@ -312,6 +312,10 @@
                 report->exception_handled(display_server.get(), invocation.id(), err);
             }
         }
+        else if ("request_persistent_surface_id" == invocation.method_name())
+        {
+            invoke(this, display_server.get(), &protobuf::DisplayServer::request_persistent_surface_id, invocation);
+        }
         else
         {
             report->unknown_method(display_server.get(), invocation.id(), invocation.method_name());

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2015-06-05 08:29:23 +0000
+++ src/server/frontend/session_mediator.cpp	2015-06-09 07:21:31 +0000
@@ -878,6 +878,24 @@
     done->Run();
 }
 
+void mf::SessionMediator::request_persistent_surface_id(
+    ::google::protobuf::RpcController*,
+    mir::protobuf::SurfaceId const* request,
+    mir::protobuf::PersistentSurfaceId* response,
+    google::protobuf::Closure* done)
+{
+    auto const session = weak_session.lock();
+
+    if (!session)
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+    auto buffer = shell->persistent_id_for(session, mf::SurfaceId{request->value()});
+
+    *response->mutable_value() = std::string{buffer.begin(), buffer.end()};
+
+    done->Run();
+}
+
 void mf::SessionMediator::pack_protobuf_buffer(
     protobuf::Buffer& protobuf_buffer,
     graphics::Buffer* graphics_buffer,

=== modified file 'src/server/frontend/session_mediator.h'
--- src/server/frontend/session_mediator.h	2015-06-01 14:37:00 +0000
+++ src/server/frontend/session_mediator.h	2015-06-09 07:21:31 +0000
@@ -214,6 +214,12 @@
         ::mir::protobuf::CoordinateTranslationResponse* response,
         ::google::protobuf::Closure *done) override;
 
+    void request_persistent_surface_id(
+        ::google::protobuf::RpcController* controller,
+        ::mir::protobuf::SurfaceId const* request,
+        ::mir::protobuf::PersistentSurfaceId* response,
+        ::google::protobuf::Closure* done) override;
+
 private:
     void pack_protobuf_buffer(protobuf::Buffer& protobuf_buffer,
                               graphics::Buffer* graphics_buffer,

=== modified file 'src/server/frontend/shell_wrapper.cpp'
--- src/server/frontend/shell_wrapper.cpp	2015-04-08 07:51:14 +0000
+++ src/server/frontend/shell_wrapper.cpp	2015-06-09 07:21:31 +0000
@@ -68,6 +68,11 @@
     wrapped->destroy_surface(session, surface);
 }
 
+std::string mf::ShellWrapper::persistent_id_for(std::shared_ptr<Session> const& session, mf::SurfaceId surface)
+{
+    return wrapped->persistent_id_for(session, surface);
+}
+
 int mf::ShellWrapper::set_surface_attribute(
     std::shared_ptr<Session> const& session,
     SurfaceId surface_id,

=== modified file 'src/server/frontend/shell_wrapper.h'
--- src/server/frontend/shell_wrapper.h	2015-04-08 07:51:14 +0000
+++ src/server/frontend/shell_wrapper.h	2015-06-09 07:21:31 +0000
@@ -57,6 +57,8 @@
 
     void destroy_surface(std::shared_ptr<Session> const& session, SurfaceId surface) override;
 
+    std::string persistent_id_for(std::shared_ptr<Session> const& session, SurfaceId surface) override;
+
     int set_surface_attribute(
         std::shared_ptr<Session> const& session,
         SurfaceId surface_id,

=== modified file 'src/server/shell/CMakeLists.txt'
--- src/server/shell/CMakeLists.txt	2015-05-19 21:42:19 +0000
+++ src/server/shell/CMakeLists.txt	2015-06-09 07:21:31 +0000
@@ -10,6 +10,8 @@
   default_window_manager.cpp
   shell_wrapper.cpp
   surface_ready_observer.cpp
+  default_persistent_surface_store.cpp
+  persistent_surface_store.cpp
 )
 
 add_library(

=== modified file 'src/server/shell/default_configuration.cpp'
--- src/server/shell/default_configuration.cpp	2015-05-19 21:34:34 +0000
+++ src/server/shell/default_configuration.cpp	2015-06-09 07:21:31 +0000
@@ -24,6 +24,7 @@
 #include "mir/shell/canonical_window_manager.h"
 #include "mir/input/composite_event_filter.h"
 #include "mir/shell/abstract_shell.h"
+#include "default_persistent_surface_store.h"
 #include "frontend_shell.h"
 #include "graphics_display_layout.h"
 
@@ -62,12 +63,22 @@
     return wrapped;
 }
 
+std::shared_ptr<msh::PersistentSurfaceStore>
+mir::DefaultServerConfiguration::the_persistent_surface_store()
+{
+    return surface_store([]()
+    {
+        return std::make_shared<msh::DefaultPersistentSurfaceStore>();
+    });
+}
+
 std::shared_ptr<mf::Shell>
 mir::DefaultServerConfiguration::the_frontend_shell()
 {
     return frontend_shell([this]
         {
-            return std::make_shared<msh::detail::FrontendShell>(the_shell());
+            return std::make_shared<msh::detail::FrontendShell>(the_shell(),
+                                                                the_persistent_surface_store());
         });
 }
 

=== added file 'src/server/shell/default_persistent_surface_store.cpp'
--- src/server/shell/default_persistent_surface_store.cpp	1970-01-01 00:00:00 +0000
+++ src/server/shell/default_persistent_surface_store.cpp	2015-06-09 07:21:31 +0000
@@ -0,0 +1,91 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#include "default_persistent_surface_store.h"
+#include <uuid/uuid.h>
+#include <algorithm>
+#include <unordered_map>
+#include <boost/throw_exception.hpp>
+
+namespace msh = mir::shell;
+namespace ms = mir::scene;
+
+class msh::DefaultPersistentSurfaceStore::SurfaceIdBimap
+{
+public:
+    using Id = PersistentSurfaceStore::Id;
+
+    Id const& insert_or_retrieve(std::shared_ptr<scene::Surface> const& surface);
+
+    std::shared_ptr<scene::Surface> operator[](Id const& id) const;
+    Id const& operator[](std::shared_ptr<scene::Surface> const& surface) const;
+
+private:
+    std::unordered_map<Id, std::weak_ptr<scene::Surface>> id_to_surface;
+    std::unordered_map<scene::Surface const*, Id const*> surface_to_id;
+};
+
+auto msh::DefaultPersistentSurfaceStore::SurfaceIdBimap::insert_or_retrieve(std::shared_ptr<scene::Surface> const& surface)
+    -> Id const&
+{
+    auto element = surface_to_id.insert(std::make_pair(surface.get(), nullptr));
+    if (!element.second)
+    {
+        // Surface already exists in our map, return it.
+        return *element.first->second;
+    }
+    else
+    {
+        auto new_element = id_to_surface.insert(std::make_pair(Id{}, surface));
+        // Update the surface_to_id map element from nullptr to our new Id
+        element.first->second = &new_element.first->first;
+        return *element.first->second;
+    }
+}
+
+auto msh::DefaultPersistentSurfaceStore::SurfaceIdBimap::operator[](std::shared_ptr<scene::Surface> const& surface) const
+    -> Id const&
+{
+    return *surface_to_id.at(surface.get());
+}
+
+auto msh::DefaultPersistentSurfaceStore::SurfaceIdBimap::operator[](Id const& id) const
+    -> std::shared_ptr<scene::Surface>
+{
+    return id_to_surface.at(id).lock();
+}
+
+msh::DefaultPersistentSurfaceStore::DefaultPersistentSurfaceStore()
+    : store{std::make_unique<SurfaceIdBimap>()}
+{
+}
+
+msh::DefaultPersistentSurfaceStore::~DefaultPersistentSurfaceStore()
+{
+}
+
+auto msh::DefaultPersistentSurfaceStore::id_for_surface(std::shared_ptr<scene::Surface> const& surface)
+    -> Id
+{
+    return store->insert_or_retrieve(surface);
+}
+
+std::shared_ptr<ms::Surface> msh::DefaultPersistentSurfaceStore::surface_for_id(Id const& id) const
+{
+    return (*store)[id];
+}

=== added file 'src/server/shell/default_persistent_surface_store.h'
--- src/server/shell/default_persistent_surface_store.h	1970-01-01 00:00:00 +0000
+++ src/server/shell/default_persistent_surface_store.h	2015-06-09 07:21:31 +0000
@@ -0,0 +1,44 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#ifndef MIR_SHELL_DEFAULT_PERSISTENT_SURFACE_STORE_H_
+#define MIR_SHELL_DEFAULT_PERSISTENT_SURFACE_STORE_H_
+
+#include "mir/shell/persistent_surface_store.h"
+
+namespace mir
+{
+namespace shell
+{
+class DefaultPersistentSurfaceStore : public PersistentSurfaceStore
+{
+public:
+    DefaultPersistentSurfaceStore();
+    ~DefaultPersistentSurfaceStore() override;
+
+    Id id_for_surface(std::shared_ptr<scene::Surface> const& surface) override;
+    std::shared_ptr<scene::Surface> surface_for_id(Id const& id) const override;
+
+private:
+    class SurfaceIdBimap;
+    std::unique_ptr<SurfaceIdBimap> const store;
+};
+}
+}
+
+#endif // MIR_SHELL_DEFAULT_PERSISTENT_SURFACE_STORE_H_

=== modified file 'src/server/shell/frontend_shell.cpp'
--- src/server/shell/frontend_shell.cpp	2015-04-08 07:51:14 +0000
+++ src/server/shell/frontend_shell.cpp	2015-06-09 07:21:31 +0000
@@ -17,6 +17,7 @@
  */
 
 #include "frontend_shell.h"
+#include "mir/shell/persistent_surface_store.h"
 #include "mir/shell/shell.h"
 
 #include "mir/scene/session.h"
@@ -93,6 +94,14 @@
     wrapped->destroy_surface(scene_session, surface);
 }
 
+std::string msh::FrontendShell::persistent_id_for(std::shared_ptr<mf::Session> const& session, mf::SurfaceId surface_id)
+{
+    auto const scene_session = std::dynamic_pointer_cast<ms::Session>(session);
+    auto const surface = scene_session->surface(surface_id);
+
+    return surface_store->id_for_surface(surface).serialize_to_string();
+}
+
 int msh::FrontendShell::set_surface_attribute(
     std::shared_ptr<mf::Session> const& session,
     mf::SurfaceId surface_id,

=== modified file 'src/server/shell/frontend_shell.h'
--- src/server/shell/frontend_shell.h	2015-04-08 07:51:14 +0000
+++ src/server/shell/frontend_shell.h	2015-06-09 07:21:31 +0000
@@ -29,6 +29,7 @@
 namespace shell
 {
 class Shell;
+class PersistentSurfaceStore;
 
 namespace detail
 {
@@ -36,9 +37,14 @@
 struct FrontendShell : mf::Shell
 {
     std::shared_ptr<shell::Shell> const wrapped;
+    std::shared_ptr<shell::PersistentSurfaceStore> const surface_store;
 
-    explicit FrontendShell(std::shared_ptr<shell::Shell> const& wrapped) :
-        wrapped{wrapped} {}
+    explicit FrontendShell(std::shared_ptr<shell::Shell> const& wrapped,
+                           std::shared_ptr<shell::PersistentSurfaceStore> const& surface_store)
+        : wrapped{wrapped},
+          surface_store{surface_store}
+    {
+    }
 
     std::shared_ptr<mf::Session> open_session(
         pid_t client_pid,
@@ -63,6 +69,8 @@
 
     void destroy_surface(std::shared_ptr<mf::Session> const& session, mf::SurfaceId surface) override;
 
+    std::string persistent_id_for(std::shared_ptr<mf::Session> const& session, mf::SurfaceId surface) override;
+
     int set_surface_attribute(
         std::shared_ptr<mf::Session> const& session,
         mf::SurfaceId surface_id,

=== added file 'src/server/shell/persistent_surface_store.cpp'
--- src/server/shell/persistent_surface_store.cpp	1970-01-01 00:00:00 +0000
+++ src/server/shell/persistent_surface_store.cpp	2015-06-09 07:21:31 +0000
@@ -0,0 +1,79 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#include "mir/shell/persistent_surface_store.h"
+
+#include <boost/throw_exception.hpp>
+
+namespace msh = mir::shell;
+using Id = mir::shell::PersistentSurfaceStore::Id;
+
+Id::Id()
+{
+    uuid_generate(uuid);
+}
+
+Id::Id(std::string const& serialized_form)
+{
+    using namespace std::literals::string_literals;
+    if (serialized_form.size() != 36)
+    {
+        BOOST_THROW_EXCEPTION((std::invalid_argument{"Failed to parse: "s + serialized_form +
+            " (has invalid length: "s + std::to_string(serialized_form.size()) +
+            " expected 36)"}));
+    }
+
+    if (uuid_parse(serialized_form.c_str(), uuid) != 0)
+    {
+        BOOST_THROW_EXCEPTION((std::invalid_argument{"Failed to parse: "s + serialized_form}));
+    }
+}
+
+Id::Id(Id const& rhs)
+{
+    std::copy(rhs.uuid, rhs.uuid + sizeof(rhs.uuid), uuid);
+}
+
+Id& Id::operator=(Id const& rhs)
+{
+    std::copy(rhs.uuid, rhs.uuid + sizeof(rhs.uuid), uuid);
+    return *this;
+}
+
+bool Id::operator==(Id const& rhs) const
+{
+    return uuid_compare(uuid, rhs.uuid) == 0;
+}
+
+std::string Id::serialize_to_string() const
+{
+    // uuid_unparse adds a trailing null; allocate enough memory for it...
+    char buffer[37];
+    uuid_unparse(uuid, buffer);
+
+    return buffer;
+}
+
+auto std::hash<Id>::operator()(argument_type const &uuid) const
+    -> result_type
+{
+    // uuid_t is defined in the header to be a char[16]; hash this as two 64-byte integers
+    // and mix the result with XOR.
+    return std::hash<uint64_t>()(*reinterpret_cast<uint64_t const*>(uuid.uuid)) ^
+           std::hash<uint64_t>()(*reinterpret_cast<uint64_t const*>(uuid.uuid + sizeof(uint64_t)));
+}

=== modified file 'src/server/symbols.map'
--- src/server/symbols.map	2015-06-02 13:41:40 +0000
+++ src/server/symbols.map	2015-06-09 07:21:31 +0000
@@ -581,6 +581,7 @@
     mir::DefaultServerConfiguration::the_mediating_display_changer*;
     mir::DefaultServerConfiguration::the_message_processor_report*;
     mir::DefaultServerConfiguration::the_options*;
+    mir::DefaultServerConfiguration::the_persistent_surface_store*;
     mir::DefaultServerConfiguration::the_pixel_buffer*;
     mir::DefaultServerConfiguration::the_placement_strategy*;
     mir::DefaultServerConfiguration::the_prompt_connection_creator*;

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2015-05-27 11:37:38 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2015-06-09 07:21:31 +0000
@@ -885,3 +885,23 @@
     mir_surface_release_sync(surface);
     mir_connection_release(connection);
 }
+
+TEST_F(ClientLibrary, can_get_persistent_surface_id)
+{
+    auto connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+
+    auto surface_spec = mir_connection_create_spec_for_normal_surface(connection,
+                                                                      800, 600,
+                                                                      mir_pixel_format_argb_8888);
+    auto surface = mir_surface_create_sync(surface_spec);
+    mir_surface_spec_release(surface_spec);
+
+    ASSERT_THAT(surface, IsValid());
+
+    auto surface_id = mir_surface_request_persistent_id_sync(surface);
+    EXPECT_TRUE(mir_persistent_id_is_valid(surface_id));
+
+    mir_surface_release_sync(surface);
+    mir_persistent_id_release(surface_id);
+    mir_connection_release(connection);
+}

=== modified file 'tests/include/mir_test_doubles/mock_shell.h'
--- tests/include/mir_test_doubles/mock_shell.h	2015-04-08 07:51:14 +0000
+++ tests/include/mir_test_doubles/mock_shell.h	2015-06-09 07:21:31 +0000
@@ -54,6 +54,8 @@
     MOCK_METHOD2(create_surface, frontend::SurfaceId(std::shared_ptr<frontend::Session> const&, scene::SurfaceCreationParameters const& params));
     MOCK_METHOD3(modify_surface, void(std::shared_ptr<frontend::Session> const&, frontend::SurfaceId, shell::SurfaceSpecification const&));
     MOCK_METHOD2(destroy_surface, void(std::shared_ptr<frontend::Session> const&, frontend::SurfaceId));
+    MOCK_METHOD2(persistent_id_for, std::string(std::shared_ptr<frontend::Session> const&, frontend::SurfaceId));
+
 
     MOCK_METHOD4(set_surface_attribute, int(
         std::shared_ptr<frontend::Session> const& session, frontend::SurfaceId surface_id,

=== modified file 'tests/unit-tests/shell/CMakeLists.txt'
--- tests/unit-tests/shell/CMakeLists.txt	2015-03-31 02:35:42 +0000
+++ tests/unit-tests/shell/CMakeLists.txt	2015-06-09 07:21:31 +0000
@@ -2,6 +2,8 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_default_placement_strategy.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_default_window_manager.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_graphics_display_layout.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_persistent_surface_store_id.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_default_persistent_surface_store.cpp
 )
 
 set(

=== added file 'tests/unit-tests/shell/test_default_persistent_surface_store.cpp'
--- tests/unit-tests/shell/test_default_persistent_surface_store.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/shell/test_default_persistent_surface_store.cpp	2015-06-09 07:21:31 +0000
@@ -0,0 +1,124 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#include "mir/shell/persistent_surface_store.h"
+
+#include "mir/scene/surface.h"
+
+#include "src/server/shell/default_persistent_surface_store.h"
+
+#include "mir_test_doubles/mock_surface.h"
+
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace ms = mir::scene;
+namespace msh = mir::shell;
+namespace mtd = mir::test::doubles;
+
+TEST(DefaultPersistentSurfaceStore, id_for_surface_is_idempotent)
+{
+    using namespace testing;
+
+    msh::DefaultPersistentSurfaceStore store;
+
+    auto surface = std::make_shared<NiceMock<mtd::MockSurface>>();
+
+    auto id_one = store.id_for_surface(surface);
+    auto id_two = store.id_for_surface(surface);
+
+    EXPECT_THAT(id_one, Eq(std::ref(id_two)));
+}
+
+TEST(DefaultPersistentSurfaceStore, id_is_stable_under_aliasing)
+{
+    using namespace testing;
+
+    msh::DefaultPersistentSurfaceStore store;
+
+    auto surface = std::make_shared<NiceMock<mtd::MockSurface>>();
+    auto surface_alias = surface;
+
+    auto id_one = store.id_for_surface(surface);
+    auto id_two = store.id_for_surface(surface_alias);
+
+    EXPECT_THAT(id_one, Eq(std::ref(id_two)));
+}
+
+TEST(DefaultPersistentSurfaceStore, can_lookup_surface_by_id)
+{
+    using namespace testing;
+
+    msh::DefaultPersistentSurfaceStore store;
+
+    auto surface = std::make_shared<NiceMock<mtd::MockSurface>>();
+
+    auto id = store.id_for_surface(surface);
+
+    auto looked_up_surface = store.surface_for_id(id);
+
+    EXPECT_THAT(looked_up_surface, Eq(surface));
+}
+
+TEST(DefaultPersistentSurfaceStore, retrieves_correct_surface)
+{
+    using namespace testing;
+
+    msh::DefaultPersistentSurfaceStore store;
+
+    auto surface_one = std::make_shared<NiceMock<mtd::MockSurface>>();
+    auto surface_two = std::make_shared<NiceMock<mtd::MockSurface>>();
+
+    auto id_one = store.id_for_surface(surface_one);
+    auto id_two = store.id_for_surface(surface_two);
+
+    auto looked_up_surface_one = store.surface_for_id(id_one);
+    auto looked_up_surface_two = store.surface_for_id(id_two);
+
+    EXPECT_THAT(looked_up_surface_one, Eq(surface_one));
+    EXPECT_THAT(looked_up_surface_two, Eq(surface_two));
+}
+
+TEST(DefaultPersistentSurfaceStore, looking_up_destroyed_surface_returns_nullptr)
+{
+    using namespace testing;
+
+    msh::DefaultPersistentSurfaceStore store;
+
+    auto surface = std::make_shared<NiceMock<mtd::MockSurface>>();
+
+    auto id = store.id_for_surface(surface);
+
+    surface.reset();
+
+    auto looked_up_surface = store.surface_for_id(id);
+
+    EXPECT_THAT(looked_up_surface, Eq(nullptr));
+}
+
+TEST(DefaultPersistentSurfaceStore, looking_up_nonexistent_surface_throws)
+{
+    using namespace testing;
+
+    msh::DefaultPersistentSurfaceStore store;
+
+    msh::PersistentSurfaceStore::Id id;
+
+    EXPECT_THROW(store.surface_for_id(id), std::out_of_range);
+}

=== added file 'tests/unit-tests/shell/test_persistent_surface_store_id.cpp'
--- tests/unit-tests/shell/test_persistent_surface_store_id.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/shell/test_persistent_surface_store_id.cpp	2015-06-09 07:21:31 +0000
@@ -0,0 +1,88 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Christopher James Halse Rogers <[e-mail]>
+ */
+
+#include "mir/shell/persistent_surface_store.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace ms = mir::scene;
+namespace msh = mir::shell;
+//namespace mtd = mir::test::doubles;
+
+using Id = msh::PersistentSurfaceStore::Id;
+
+TEST(PersistentSurfaceStoreId, deserialising_wildly_incorrect_buffer_raises_exception)
+{
+    EXPECT_THROW(Id{"bob"}, std::invalid_argument);
+}
+
+TEST(PersistentSurfaceStoreId, deserialising_invalid_buffer_raises_exception)
+{
+    // This is the right size, but isn't a UUID because it lacks the XX-XX-XX structure
+    EXPECT_THROW(Id{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, std::invalid_argument);
+}
+
+TEST(PersistentSurfaceStoreId, serialization_roundtrips_with_deserialization)
+{
+    using namespace testing;
+    Id first_id;
+
+    auto const buf = first_id.serialize_to_string();
+    Id const second_id{buf};
+
+    EXPECT_THAT(second_id, Eq(first_id));
+}
+
+TEST(PersistentSurfaceStoreId, ids_assigned_evaluate_equal)
+{
+    using namespace testing;
+
+    Id const first_id;
+
+    auto const second_id = first_id;
+
+    EXPECT_THAT(second_id, Eq(first_id));
+}
+
+TEST(PersistentSurfaceStoreId, equal_ids_hash_equally)
+{
+    using namespace testing;
+
+    auto const uuid_string = "0744caf3-c8d9-4483-a005-3375c1954287";
+
+    Id const first_id{uuid_string};
+    Id const second_id{uuid_string};
+
+    EXPECT_THAT(std::hash<Id>()(second_id), Eq(std::hash<Id>()(first_id)));
+}
+
+TEST(PersistentSurfaceStoreId, can_assign_ids)
+{
+    using namespace testing;
+
+    Id first_id;
+    Id second_id;
+
+    // Technically, there's a roughly 1-in-2^128 chance of a false fail here.
+    EXPECT_THAT(second_id, Not(Eq(first_id)));
+
+    second_id = first_id;
+
+    EXPECT_THAT(second_id, Eq(first_id));
+}