> Overall, very nice. The size of the proposal however means the little issues
> are adding up to quite a list...
> 
> (1) 397 +MirTrustSession* mir_connection_trust_session_create(MirConnection*
> connection);
> should be renamed to follow the existing convention:
> MirTrustSession* mir_connection_create_trust_session(MirConnection*
> connection);

My bad again. Done

> 
> (2) Please expand on the commit message/description further. It's still too
> brief for a large enhancement.

Done

> 
> (3) Please remove "handle_trust_session_event()". EventSink is only meant to
> have one method "handle_event" which handles any and all event types. The
> other methods of EventSink are mistakes we need to revert already. Sorry it's
> misleading.

Hmm. bit of an interesting one about this one. The only consumer for MirEvents are the surfaces.
Which means we'll need another consumer to push the events to clients without a surface.
I've removed the TrustSessionControl class, and added a mcl::EventDistributor class to the client library which does this.

> 
> (4) ABI bumps: The server ABI seems to have changed, but the client ABI is not
> broken by this proposal. Please revent the MIR_CLIENT_ABI bump.

Done

> 
> (5) trust_session.c: The program prints messages referring to itself as
> "demo_client_trusted_helper" and "demo_client_app2". However the binary name
> is "mir_demo_client_trust_session". Maybe it should be printing that or just
> argv[0] to avoid confusion.

Made references/stdout of each process more explicit.

> 
> (6) trust_session.c: Forks into two processes, which is unusual for a client.
> Is that the expected/normal use case?

We need 2 processes for a trust session, the "trusted helper", and the "trusted app".
It would probably be more normal for the trusted helper to start an external process, but I it 
better to enclose all behaviour in the single example.

> 
> (7) read(STDIN_FILENO, &buff, 1) - Why not getchar()? It's simpler more
> portable too.

Done

> 
> (8) Long lines - I prefer code wrapped before column 80 and so do plenty of
> people. Not just because that's what a terminal defaults to, but it's easier
> for the brain to follow line continuation when you avoid them getting too
> long.
> 
> (9) frontend/trust_session.h: Is std::vector necessary to maintain an ordered
> list? If not then the more appropriate container is std::unordered_set

Needs to be ordered for the shell. (last session is on top)

> 
> (10) frontend/trust_session.h: a stop() method but no start() method? Is that
> a good idea? Is the start implicit in construction?

It's not ideal, but there is a static start_for function on the src/server/scene/trust_session.cpp
Which essentially is implicit start on creation. Can't do what we need from a member start.

> 
> (11) MirTrustSessionAddTrustResult: The enum values are arguably too long.
> IMHO.

Now shorter (kind of). Though there are some pretty long ones about still.

> 
> (12) Identical descriptions for different function types?
> 784     + * Callback member of MirTrustSession for handling of trust sessions.
> 791     + * Callback member of MirTrustSession for handling of trust sessions.
> 

Fixed.

> (13) We should avoid landing new code with known race conditions:
> 1618    +    /* todo: race condition. protobuf does not guarantee that
> callbacks will be synchronized. potential
> 1619    +             race in session, last_buffer_id */

Removed comment. Copied over from src/client/mir_surface.h, which has similar usage.
Original comment was [now] incorrect as per alan_g/kdub in any case.

> 
> (14) Superficially it's confusing as to why these with equivalent names should
> be separate classes:
> 1592    +struct MirTrustSession : public mir::client::TrustSession
> But now I notice how simple mir::client::TrustSession is, they're clearly
> quite different things. So the class names should clarify the differences
> between them.

Removed abstract class. No idea why I abstracted it in the first place.

> 
> (15) What's a "Control"?
> 1885    +class TrustSessionControl
> I'm not familiar with that abstraction. Even though there seems to be another
> "Control" class there already :S

This is essentially just a marshalling object. It receives the state events from rpc and
calls handlers the registered client TrustSessions.
It's been renamed and repurposed as the generic EventDistributor object (to which the TrustSession registers for events)

> 
> (16) test_protobuf_client.cpp: A busy wait loop seems like a lazy approach to
> this (albeit a lazy approach we've already used in other tests). It would be
> cleaner to use a std::condition_variable with a std::mutex and bool. That
> would also avoid the expected Helgrind complaints about std::atomic being
> racy.

Done with existing WaitCondition class. I haven't changed the rest of the uses in the file though.

> 
> (17) test_trust_session.cpp: Wrong copyright year range.

Done.