Code review comment for lp:~cemil-azizoglu/mir/improve-raii

===
48 + virtual std::shared_ptr<BufferHandle> compositor_acquire(void const* user_id) = 0;
49 + virtual void compositor_release(graphics::Buffer* const buffer) = 0;
50 + virtual std::shared_ptr<BufferHandle> snapshot_acquire() = 0;
51 + virtual void snapshot_release(graphics::Buffer* const buffer) = 0;
==

I'd rather have compositor_acquire and snapshot_acquire return a BufferHandle by value.
BufferHandle should be a moveable only type.

The BufferHandle constructor could also be:

BufferHande(std::function<void()> const& release)

Then we can remove compositor_release and snapshot_release.

===
334 + return buffer_bundle->compositor_acquire(user_id)->buffer();
===

Nobody is holding the return shared_ptr<BufferHandle> so at the end of this scope, the buffer will be returned to the queue - possibly resulting in tearing artifacts.

In order for this to work correctly the caller of lock_compositor_buffer (i.e. SurfaceSnapshot) needs to be able to receive the BufferHandle object. That would pretty much make BufferStream a useless replicated interface - they should all just collapse to BufferBundle instead.

===
160 + explicit BufferHandle(BufferBundle* bundle,
161 + std::shared_ptr<graphics::Buffer> buffer);
162 + BufferBundle* buffer_bundle;
===

As mentioned above this could probably be:

class BufferHandle
{
public:
    typedef std::function<void(graphics::Buffer* buffer)> Callback;
    explicit BufferHandle(std::shared_ptr<graphics::Buffer> const& buffer,
                          Callback const& release);

    //Move constructor
    BufferHandle(BufferHandle&& other);
    //Move assignment
    BufferHandle& operator=(BufferHandle&& other);

    std::shared_ptr<graphics::Buffer> buffer();

private:
    // Moveable only type
    BufferHandle(BufferHandle const&) = delete;
    BufferHandle& operator=(BufferHandle const&) = delete;

    std::shared_ptr<graphics::Buffer> wrapped;
    Callback release;
};

===
213 + std::shared_ptr<mc::BufferHandle> const acquired_buffer =
214 + std::make_shared<mc::CompositorBufferHandle>(this, buffer_for(current_compositor_buffer, buffers));
===

With the BufferHandle above then this would become:

BufferHandle const handle{buffer_for(current_compositor_buffer, buffers),
 [weak_ptr_this](Buffer* buffer)
 {
    if (auto self = weak_ptr_this.lock())
    {
       compositor_release(buffer);
    }
 }

The weak_ptr_this is needed to ensure you won't be calling dead code if the BufferQueue gets destroyed before one of the BufferHandles. Alternatively, you could keep a shared_ptr instead to keep the buffer queue object alive but I would prefer the BufferHandle to not impose that requirement.

The above will require enabled_shared_from_this support in the BufferQueue class.