Code review comment for lp:~raof/mir/1hz-rendering-always

> 1. While “SwapBuffersStateObserver” is not an incorrect name, I don't think
> it's a *useful* name; it tells you information you can easily deduce from the
> names of the methods in the interface, but doesn't tell you what the class is
> expected to do.

I guess we disagree about what detail can be abstracted away and what is essential. The code calling "swap_now_blocking()/enter_swap_blocked()" needs to know about the "observer" - it doesn't need to know about framedropping.

> 2. Yes, although it'll be easy enough to fold the last piece of
> FrameDroppingPolicy into the class.

There are two pieces, but agreed.

> 3. Yes, the policy is about framedropping. To expand:
>
> The top-level problem this solves is ‘we don't want mir_surface_swap_buffers
> to block indefinitely’. Given that frames may remain undisplayed indefinitely
> - such as when the surface is entirely obscured or the screen is off - this
> means that either we allocate and return new buffers indefinitely or we drop
> frames.
>
> Unbounded memory use is obviously not acceptable, so we've chosen to drop
> frames. When to drop frames (currently only in this case, but easily
> implemented in general) is the purview of FrameDroppingPolicy.

I understand the problem and accept the solution. But I'm not convinced it makes sense splitting out just the framedropping logic from the rest of the swap buffers algorithm.

> No, this particular piece of abstraction is not necessary for anything we need
> to implement now, nor is a prerequisite for a near-term goal. This would have
> been just as effectively implemented by the MP circa one month ago, where I
> deliberately implemented the simplest solution.
> (https://code.launchpad.net/~raof/mir/1hz-rendering-
> always/+merge/217377/comments/516869)
>
> That said, seems a fairly obvious piece of abstraction to me - although this
> MP suggests it's not obvious to others :).

Quite.