Code review comment for lp:~andreas-pokorny/mir/allow-transparent-server-buffers

Do we actually need CascadedDisplayConfigurationPolicy? Or could TranslucentOutputs simply decorate the default policy?

~~~~

49 +CascadedDisplayConfigurationPolicy::CascadedDisplayConfigurationPolicy(std::shared_ptr<graphics::DisplayConfigurationPolicy> const& l,
50 + std::shared_ptr<graphics::DisplayConfigurationPolicy> const& r)

vs

102 + CascadedDisplayConfigurationPolicy(std::shared_ptr<graphics::DisplayConfigurationPolicy> const& left,
103 + std::shared_ptr<graphics::DisplayConfigurationPolicy> const& right);

The naming should be consistent between declaration and definition.
http://unity.ubuntu.com/mir/cppguide/index.html?showone=Function_Declarations_and_Definitions#Function_Declarations_and_Definitions

Also, I don't understand the significance of left/right (or the alternative l/r) - are these names meaningful? What I can see in the code is better represented as "first/second".

~~~~

106 + std::shared_ptr<DisplayConfigurationPolicy> left, right;

These dependencies should be const.

Also, while it isn't in the style guide, we have evolved a strong tendency towards single declarations in a statement. Vis:

std::shared_ptr<DisplayConfigurationPolicy> const left;
std::shared_ptr<DisplayConfigurationPolicy> const right;

~~~~

438 - void configure_output(DisplayConfigurationOutputId, bool, geometry::Point, size_t, MirPowerMode power_mode);
439 + void configure_output(DisplayConfigurationOutputId, bool, geometry::Point, size_t, size_t, MirPowerMode power_mode);

OK, not introduced by this MP, but the declaration doesn't:

1. Name the parameters
2. mention "virtual" or "override"