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

A first pass...

Since mir_client_library.cpp has started to become unwieldly I propose we follow the convention mir_screencast is using, i.e.:

1. Public header file goes in mir_toolkit/mir_trust_session.h
2. C API implementation goes in src/client/mir_trust_session_api.cpp
3. Internal classes as usual, src/client/mir_trust_session.{cpp,h}

148 +///\page trust_session.c trust_session.c: A mir client which starts a trust session and trusted client app.
149 +/// mir_demo_client_trust_session shows the use of mir trust session API.
150 +/// This program opens a mir connection and creates a trust session.
...

519 + virtual std::shared_ptr<TrustSession> start_trust_session_for(std::string& error,
520 + std::shared_ptr<Session> const& session,

Since this is a C file, the C(/Javadoc) format is preferred, like in the public files.

519 + virtual std::shared_ptr<TrustSession> start_trust_session_for(std::string& error,
520 + std::shared_ptr<Session> const& session,
...

Either use a 4-space indentation or align with char after '('.

567 + virtual ~TrustSession() {}
1129 + TrustSession() {}
1130 + virtual ~TrustSession() {}

Use '= default' instead;

744 +#include <string>

Not needed.

1239 +/**************************
1240 + * Trust session specific functions *
1241 + **************************/

This can be made prettier :)

1249 + pid_t pid)

1255 + mir_trust_session_callback callback,
1256 + void* context)

1272 + reinterpret_cast<mir_trust_session_callback>
1273 + (assign_result),
1274 + NULL));

1279 + mir_trust_session_callback callback,
1280 + void* context)

1296 + reinterpret_cast<mir_trust_session_callback>
1297 + (assign_result),
1298 + NULL));

1308 + mir_trust_session_event_callback callback,
1309 + void* context)

Either use a 4-space indentation or align with char after '(' on previous line.

1427 + [this]
1428 + (MirTrustSessionState new_state)

1501 + [this, callback, context]
1502 + (MirTrustSessionState state)

2771 + [this]
2772 + (std::shared_ptr<msh::Session> const& child_session)

We tend to prefer to keep the parameter list on the same line as the capture list:
[this] (MirTrustSessionState new_state), but that's not a hard and fast rule AFAIK.

1430 + std::lock_guard<std::recursive_mutex> lock(mutex);

Why do we need a recursive mutex?

1595 + MirTrustSession(mir::protobuf::DisplayServer::Stub & server,
1600 + MirTrustSession(MirTrustSession const &) = delete;
1601 + MirTrustSession& operator=(MirTrustSession const &) = delete;
... and others in the class

* and & go with the type (or 'const' if there is one).

1820 + std::unique_lock<std::mutex> lk(guard);

Since the lock is not used with condition variables or moved, we could use the simpler std::lock_guard<>.

1814 +mcl::TrustSessionControl::~TrustSessionControl()
1815 +{
1816 +}

Is this needed?

2737 + TrustSession* impl = new TrustSession(trusted_helper, parameters, sink);
2738 + std::shared_ptr<msh::TrustSession> ptr(impl);

auto const trust_session = std::make_shared<TrustSession>(trusted_helper, parameters, sink);