> 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}

Done

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

This is using the same format as the other examples.

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

Done

> 
> 567 + virtual ~TrustSession() {}
> 1129    + TrustSession() {}
> 1130    + virtual ~TrustSession() {}
> 
> Use '= default' instead;

Done

> 
> 744 +#include <string>
> 
> Not needed.

Done

> 
> 1239    +/**************************
> 1240    + * Trust session specific functions *
> 1241    + **************************/
> 
> This can be made prettier :)

Removed with separation of client api source files

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

Must have picked it up on some other project. Switched to single line

> 
> 1430    + std::lock_guard<std::recursive_mutex> lock(mutex);
> 
> Why do we need a recursive mutex?

Changed already

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

Done

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

Done

> 
> 1814    +mcl::TrustSessionControl::~TrustSessionControl()
> 1815    +{
> 1816    +}
> 
> Is this needed?

No. Removed (=default)

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

Done