Code review comment for lp:~robertcarr/mir/enable-inprocess-egl

76 + auto client = new me::InprocessEGLClient(graphics_platform, surface_factory);
77 + client->start();

Looks like a leak - at the very least it needs a comment explaining why it doesn't leak.

80 + std::shared_ptr<msh::SurfaceFactory> const surface_factory;
81 + std::shared_ptr<mg::Platform> const graphics_platform;

should be const.

Actually, loose this class and rewrite main():

int main(int argc, char const* argv[])
try
{
    mir::DefaultServerConfiguration config(argc, argv);

    std::shared_ptr<me::InprocessEGLClient> client;

    mir::run_mir(config, [&](mir::DisplayServer&)
    {
         client = std::make_shared<me::InprocessEGLClient>(
              config.the_surface_factory(),
              config.the_graphics_platform());
         client->start();
    });
}
catch (std::exception const& error)
...

~~~~

516 === modified file 'include/server/mir/server_configuration.h'
657 === modified file 'src/server/display_server.cpp'
1020 === modified file 'tests/integration-tests/cucumber/test_session_management_context.cpp'

Unnecessary changes to these files. (None are are needed.)

~~~~

375 + virtual ~InprocessEGLClient() {}

Why written explicitly? Why not "default"? Why virtual?

~~~~

402 -

Unnecessary

414 -
415 +

Wrong

~~~~

427 -/// All things Mir

It is a feeble doc comment, but please don't just remove it - replace it with something better!

~~~~

637 - display->context = connection;
638 + display->context = static_cast<void*>(connection);

Unnecessary?

~~~~

650 + display_server.cpp
651 run_mir.cpp
652 - display_server.cpp

Unnecessary.