blimp: Make Blimp client use a Display for compositing.

blimp/client/feature/compositor/blimp_delegating_output_surface.h

Index: blimp/client/feature/compositor/blimp_delegating_output_surface.h
diff --git a/blimp/client/feature/compositor/blimp_delegating_output_surface.h b/blimp/client/feature/compositor/blimp_delegating_output_surface.h
new file mode 100644
index 0000000000000000000000000000000000000000..0a69ef02380b6159ae93f3d203e2f4005820409a
--- /dev/null
+++ b/blimp/client/feature/compositor/blimp_delegating_output_surface.h
@@ -0,0 +1,70 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BLIMP_CLIENT_FEATURE_COMPOSITOR_BLIMP_DELEGATING_OUTPUT_SURFACE_H_
+#define BLIMP_CLIENT_FEATURE_COMPOSITOR_BLIMP_DELEGATING_OUTPUT_SURFACE_H_
+
+#include "cc/output/output_surface.h"
+#include "cc/output/renderer_settings.h"
+#include "cc/scheduler/begin_frame_source.h"
+#include "cc/surfaces/display.h"
+#include "cc/surfaces/display_client.h"
+#include "cc/surfaces/surface_factory.h"
+#include "cc/surfaces/surface_factory_client.h"
+#include "cc/surfaces/surface_id_allocator.h"
+#include "cc/surfaces/surface_manager.h"
+
+namespace blimp {
+namespace client {
+
+class BlimpDelegatingOutputSurface : public cc::OutputSurface,
+                                     public cc::SurfaceFactoryClient,
+                                     public cc::DisplayClient {

[Wez, 2016/08/12 01:56:49]

Can we add a brief comment to explain what this is for, plz?

Specifically, if this is a Blimp-specific OutputSurface, and it is constructed
with an OutputSurface, then why does the caller not just use that OutputSurface
directly..?

Basically what bits is this tying together that is so Blimp-specific? :P

[danakj, 2016/08/12 18:16:53]

It's a delegating output surface that delegates compositing to a Display. This
is going to get more clear when
https://bugs.chromium.org/p/chromium/issues/detail?id=606056 is done. Cuz they
won't both be called OutputSurface.

+ public:
+  BlimpDelegatingOutputSurface(
+      scoped_refptr<cc::ContextProvider> compositor_context_provider,
+      scoped_refptr<cc::ContextProvider> worker_context_provider,
+      std::unique_ptr<cc::OutputSurface> display_output_surface,
+      gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,

[Wez, 2016/08/12 01:56:50]

Is this ref-counted, or passing ownership? If neither then please add a comment
to clarify the lifetime expectation. It looks like the GPU memory buffer manager
must out-live the OutputSurface in this case.

[danakj, 2016/08/12 18:16:53]

Neither, it's a raw pointer which are not ownership transfers. I thought this is
generally assumed?

[Wez, 2016/08/18 23:22:40]

Right, now that we have unique_ptr and scoped_refptr it's most common to use
bare pointers for interface injection, where the recipient doesn't own the
received pointer, but we still need to be clear about how long the interface
impl needs to remain valid for - the duration of the receiving call, the
lifetime of the receiving object, or something else, IMO.

We have comments elsewhere along the lines of "Caller must ensure that |foo|
outlives this Bar."

+      const cc::RendererSettings& renderer_settings,
+      base::SingleThreadTaskRunner* task_runner);

[Wez, 2016/08/12 01:56:50]

nit: const scoped_refptr<>& for clarity here.

[danakj, 2016/08/12 18:16:53]

So, the stuff it passes the task_runner to don't take a ref, so I'm not sure
what using scoped_refptr<>& is for? To keep a ref inside the constructor while
it's running?

[danakj, 2016/08/12 18:17:53]

(It should actually be taking a scoped_refptr here cuz I'm pretty sure something
turns it into one way down in the stack, but this is a mess across all our
output surfaces, and holding a ref in the constructor here doesn't seem to be
really of assistance to me.)

[Wez, 2016/08/18 23:22:40]

It's not so much whether the ctor takes a reference as about making the API
contract clearer:

You have two bare pointers here (the memory buffer manager and the task runner),
so if I were to make assumptions about the intention (as you suggest for the
memory buffer manager) then the only safe assumption is that the caller is
responsible for ensuring that both out-live the DelegatingOutputSurface instance
(i.e. we're using objects to inject interfaces for this class to use).

In practice, though, the ctor might just take a reference to task_runner, so
that the caller doesn't actually have to make any guarantee about task_runner's
lifetime - using scoped_refptr<> here makes the lifetime expectations explicit
(similarly to unique_ptr<>).

If the ctor just uses task_runner but doesn't keep hold of it then const
scoped_refptr<>& is still preferable to a bare pointer, in protecting later
editors of this code from accidentally introducing a task_runner lifetime
dependency that isn't valid - they'd have to pull out the bare pointer using
get() to achieve that, which should ring alarm bells.

+  ~BlimpDelegatingOutputSurface() override;
+
+  // OutputSurface implementation.
+  bool BindToClient(cc::OutputSurfaceClient* client) override;
+  void DetachFromClient() override;
+  void SwapBuffers(cc::CompositorFrame frame) override;
+  void ForceReclaimResources() override;
+  void BindFramebuffer() override;
+  uint32_t GetFramebufferCopyTextureFormat() override;
+
+  // SurfaceFactoryClient implementation.
+  void ReturnResources(const cc::ReturnedResourceArray& resources) override;
+  void SetBeginFrameSource(cc::BeginFrameSource* begin_frame_source) override;
+
+  // DisplayClient implementation.
+  void DisplayOutputSurfaceLost() override;
+  void DisplaySetMemoryPolicy(const cc::ManagedMemoryPolicy& policy) override;
+  void DisplayWillDrawAndSwap(bool will_draw_and_swap,
+                              const cc::RenderPassList& render_passes) override;
+  void DisplayDidDrawAndSwap() override;
+
+ private:
+  // TODO(danakj): These don't to be stored in unique_ptrs when OutputSurface

[Wez, 2016/08/12 01:56:49]

You mean "don't need to be..." or something else?
Why does the thread that OutputSurface is owned on affect whether these own the
objects they point to or not?

[danakj, 2016/08/12 18:16:53]

Ya, woops, that's in the original place I copied this from too.
Because they have to be destroyed in DetachFromClient right now, which is our
"impl side destructor" but happens before the object is actually destroyed.

+  // is owned/destroyed on the compositor thread.
+  std::unique_ptr<cc::SurfaceManager> surface_manager_;
+  std::unique_ptr<cc::SurfaceIdAllocator> surface_id_allocator_;
+  cc::SurfaceId delegated_surface_id_;
+
+  // Uses surface_manager_.
+  std::unique_ptr<cc::SurfaceFactory> surface_factory_;
+
+  // Uses surface_manager_.
+  std::unique_ptr<cc::Display> display_;
+};
+
+}  // namespace client
+}  // namespace blimp
+
+#endif  // BLIMP_CLIENT_FEATURE_COMPOSITOR_BLIMP_DELEGATING_OUTPUT_SURFACE_H_