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.
>

Sure, would even be happy to spike the idea.

> >
> > 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).
>

So sounds like something that we will do, once its easier to move the code in src/client. The MirConnection object particularly needs a cleanup to make this easier.

> > 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.