Code review comment for lp:~nick-dedekind/mir/trusted_sessions

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);

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

(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.

(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.

(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.

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

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

(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

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

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

(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.

(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 */

(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.

(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

(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.

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