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

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

[Already discussed] above this interface should not be published. It is internal to the Mir scene implementation.

~~~~

424 +
425 + // TODO - remove these in favour of using handle_event

What has this to do with trust sessions?

~~~~

437 + virtual void session_start_trust_session_called(std::string const& app_name) = 0;

Is the name of the current session enough information to be useful? Would some information about what is trusted be appropriate?

~~~~

1902 + // only surface events have e.surface.id . Other events will throw exception.
1903 + try
1904 + {
1905 + surface_map->with_surface_do(e.surface.id,
1906 + [&e](MirSurface* surface)
1907 + {
1908 + surface->handle_event(e);
1909 + });
1910 + }
1911 + catch (std::exception const& x)
1912 + {
1913 + rpc_report->event_parsing_failed(event);
1914 + // Eat this exception as it doesn't affect other events
1915 + }

This looks suspiciously like using exceptions in the normal control path. We're not even logging information from x.

~~~~

2016 + BOOST_THROW_EXCEPTION(std::logic_error("Cannot start another trust session"));

If I read the surrounding code correctly, the client has requested a trust session while the server already has one associated with the client? That doesn't sound like a logic_error (in the Mir code), more like a runtime_error as the client code hasn't acted in the expected manner.