Mir

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

Revision history for this message
Kevin DuBois (kdub) wrote :

(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

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

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

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.

review: Needs Fixing

« Back to merge proposal