Code review comment for lp:~kdub/mir/multiple-bufferstream-api

10+ MirBufferStream * stream;

'*' with type

26+/** Set the streams associated with the spec.
27+ * streams[0] is the bottom-most stream, and streams[size-1] is the topmost.

Space before this line.

28+ * On application of the spec, A stream that is present in the surface,

Space before this line (different topic started).
'a stream'

30+ * On application of the spec, A stream that is not present in the surface,
31+ * but is in the list will be associated with the surface.

'<some other connecting phrase>, a stream that is not present' ...

32+ * Streams set a displacement from the top-left corner of the surface.

Space before this line (change of topic), or perhaps move this information to the documentation of the MirBufferStreamInfo structure?

57+#include <list>
69+ virtual void configure_streams(Surface& surface, std::list<shell::StreamSpecification> const& config) = 0;

Somewhat pre-existing: Is a vector<> not sufficient for this use case? I see that Surface::set_streams() uses a list<> which I don't think offers any advantages over vector<> for our usage patterns and is a probably a pessimization ("use vector<> by default").

91+ for(auto
110+ for(auto
... and elsewhere

'for ('

517+ primary_stream = std::unique_ptr<Stream>(
525+ streams.emplace_back(std::unique_ptr<Stream>(

make_unique

517+ primary_stream = std::unique_ptr<Stream>(
549+ infos[i++] = MirBufferStreamInfo{

I doesn't seem that we need to treat the primary stream specially. Treating it as yet another stream simplifies the code.

553+ for(auto &stream

'auto const&'

442+ bool ensure_last_ordering_is_consistent_with(443+ std::vector<geom::Displacement> const& arrangement)
444+ {

... return arrangment == displacement;

Or even better, change:

574+ EXPECT_TRUE(ordering->ensure_last_ordering_is_consistent_with(displacements))

to EXPECT_THAT(ordering->last_arrangement(), ContainerEq(displacements)) to get more detailed information in case of failure.