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

2919 + * Copyright © 2013 Canonical Ltd.

Surely 2014 deserves a mention?

~~~~

534 === renamed file 'src/server/scene/session_container.h' => 'include/server/mir/scene/session_container.h'

Why is this interface being made public? It is internal to the Mir scene implementation and should be access via SessionManager (which has public interfaces frontend::Shell and shell::FocusController)

Oh, I see: it has an entirely new use with shell::Session::get_children(). That opens a whole new debate about procedural vs OO design:

In this instance, for example, in TrustSession::stop() we should be telling the trusted_helper that the session has stopped, not asking for this collection and manipulating it.

This isn't the only part of the design that makes use of manipulating state directly instead of tell what is going on. A quick search for [sg]et_ turns up more.

~~~~

There's a misuse of smart pointers - shared_ptr is about "ownership" and this needs to be a directed *acyclic* graph to correctly manage memory. Having the parent and child both "own" each other is wrong. At least one of these smart pointers should be a raw pointer.