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}

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