Code review comment for lp:~andreas-pokorny/mir/add-pixel-format-to-display-configuration

(1) I know I said I would drop the issue, but now I notice this, my original concerns seem justified. We all know that EGL won't and can't honour the exact format. But passing it as a parameter could give easily the reader a false impression that we expect the given pixel format to be honoured.
680 +EGLConfig mgn::detail::EGLDisplayHandle::choose_windowed_es_config(MirPixelFormat format) const
681 {
682 + EGLint const nested_egl_config_attribs[] =
683 + {
684 + EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
685 + EGL_RED_SIZE, mg::red_channel_depth(format),
686 + EGL_GREEN_SIZE, mg::green_channel_depth(format),
687 + EGL_BLUE_SIZE, mg::blue_channel_depth(format),
688 + EGL_ALPHA_SIZE, mg::alpha_channel_depth(format),
689 + EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
690 + EGL_NONE
691 + };

(2) Back on the subject of namespaces... Although I do endorse "using namespace", introducing it in existing source files can and does inflate the diff unnecessarily (like in real_kms_display_configuration.cpp). I'm not saying change it back, but just beware in future.

(4) The comment doesn't match the code:
63 /** The index in the 'pixel_format' vector of the current output pixel format. */
64 - size_t current_format_index;
65 + MirPixelFormat current_format;

(5) Unnecessary change in indentation:
88 - std::shared_ptr<DisplayConfigurationPolicy> const& initial_conf_policy) = 0;
89 + std::shared_ptr<DisplayConfigurationPolicy> const& initial_conf_policy) = 0;

(6) This is a client ABI break, so we need to bump MIRCLIENT_ABI and debian/* info:
101 - uint32_t current_output_format;
102 + MirPixelFormat current_output_format;

(7) Don't you mean "mode_index >= modes.size()" ?
521 + if (mode_index > modes.size())
522 + return 0;

(8) Please avoid the keyword "inline". The compiler is a better judge of that. Inlining unnecessarily just leads to binary bloat.