Code review comment for lp:~cemil-azizoglu/mir/create-chain-from-render-surface

> (just repeating myself from email chain)
> The signature looks good to me. I think its more straightforward internally
> and for the client if MirRenderSurface doesn't have one-time factory-like
> methods (the _get_ ones), but rather there are create/destroy (discussed in
> email chain). So, I guess on this point, I'm 'needs info' about if we're going
> to use create/destroy as opposed to get (and can wait for the email chain to
> figure that out).
> Like, the function is described as
> + * Obtain the presentation chain backing a given render surface
> but what's really going on is an object is created that has the same lifetime
> as MirRenderSurface, and it precludes the use of
> mir_render_surface_get_buffer_stream

Ack, will wait for the outcome of the discussion.

>
> prior problem, may as well fix in this MP:
> 121: + throw std::runtime_error(error);
> Recently popped up in one of my MP's I guess the 'standard mir' way is to use
> BOOST_THROW_EXCEPTION instead of just throw:
> https://code.launchpad.net/~mir-team/mir/mesa-auth-extensions/+merge/312045

Fixed, also fixed those in error_stream.cpp

>
> needs info:
> + EXPECT_THAT(bs2, Eq(nullptr));
> Returning an "error object" would seem more appropriate to the mir client api?
>

Though seemingly innocent, this will require a bunch of superfluous code changes for not much return (due to the throw coming from render_surface.cpp, not mir_connection.cpp where the error stream code is, along with necessary constructs like the surface_map). There are also other cases where we return nullptr (e.g. in buffer_stream_api.cpp, so it's not totally inconsistent). We can more easily do this once, the render surfaces become the authoritative way to obtain content (as opposed to being able to do that through MirConnection, too).

> needs fixing:
> + auto determine_physical_size = [](MirRenderSurface* rs) -> geom::Size
> + {
> + int width = -1;
> + int height = -1;
> + mir_render_surface_get_size(rs, &width, &height);
> + return {width, height};
> + };
> repeated code, and not really needed. The client is not required to create its
> buffers (or bufferstreams) based from the logical size that the rs has.

Fixed.