Introducing SurfaceReferenceFactory

content/renderer/child_frame_compositing_helper.cc

Index: content/renderer/child_frame_compositing_helper.cc
diff --git a/content/renderer/child_frame_compositing_helper.cc b/content/renderer/child_frame_compositing_helper.cc
index ca8009acffcd4d44246419ab6c873e4bcc7a1ddb..0f937bfd823feb0c79ff667589ef3b73cd238468 100644
--- a/content/renderer/child_frame_compositing_helper.cc
+++ b/content/renderer/child_frame_compositing_helper.cc
@@ -14,6 +14,7 @@
 #include "cc/output/copy_output_request.h"
 #include "cc/output/copy_output_result.h"
 #include "cc/resources/single_release_callback.h"
+#include "cc/surfaces/surface_embedding.h"
 #include "content/child/thread_safe_sender.h"
 #include "content/common/browser_plugin/browser_plugin_messages.h"
 #include "content/common/content_switches_internal.h"
@@ -38,6 +39,56 @@
 
 namespace content {
 
+namespace {
+
+class SurfaceEmbedding : public cc::SurfaceEmbeddingUsingSequence {

[Fady Samuel, 2016/12/12 16:25:07]

IframeSurfaceEmbedding

+ public:
+  SurfaceEmbedding(scoped_refptr<ThreadSafeSender> sender, int routing_id)
+      : sender_(sender), routing_id_(routing_id) {}
+
+ protected:
+  ~SurfaceEmbedding() override = default;
+
+ private:
+  void RequireSequence(const cc::SurfaceId& id,
+                       const cc::SurfaceSequence& seq) const override {
+    sender_->Send(new FrameHostMsg_RequireSequence(routing_id_, id, seq));
+  }
+
+  void SatisfySequence(const cc::SurfaceSequence& seq) const override {
+    sender_->Send(new FrameHostMsg_SatisfySequence(routing_id_, seq));
+  }
+  scoped_refptr<ThreadSafeSender> sender_;
+  int routing_id_;

[Fady Samuel, 2016/12/12 16:25:07]

DISALLOW_COPY_AND_ASSIGN(IframeSurfaceEmbedding)

[Fady Samuel, 2016/12/12 16:25:07]

const int routing_id_

+};
+
+class PluginSurfaceEmbedding : public cc::SurfaceEmbeddingUsingSequence {

[Fady Samuel, 2016/12/12 16:25:07]

nit: BrowserPluginSurfaceEmbedding

+ public:
+  PluginSurfaceEmbedding(scoped_refptr<ThreadSafeSender> sender,
+                         int routing_id,
+                         int plugin_id)
+      : sender_(sender), routing_id_(routing_id), plugin_id_(plugin_id) {}
+
+ protected:
+  ~PluginSurfaceEmbedding() override = default;
+
+ private:
+  void SatisfySequence(const cc::SurfaceSequence& seq) const override {
+    sender_->Send(
+        new BrowserPluginHostMsg_SatisfySequence(routing_id_, plugin_id_, seq));
+  }
+  void RequireSequence(const cc::SurfaceId& id,
+                       const cc::SurfaceSequence& seq) const override {
+    sender_->Send(new BrowserPluginHostMsg_RequireSequence(
+        routing_id_, plugin_id_, id, seq));
+  }
+  scoped_refptr<ThreadSafeSender> sender_;
+  int routing_id_;

[Fady Samuel, 2016/12/12 16:25:07]

const int routing_id_;

+  int plugin_id_;

[Fady Samuel, 2016/12/12 16:25:07]

const int browser_plugin_instance_id_; Longer, more descriptive names demystify
code in the future.

[Fady Samuel, 2016/12/12 16:25:07]

DISALLOW_COPY_AND_ASSIGN(BrowserPluginSurfaceEmbedding)

+};
+
+}  // namespace
+
 ChildFrameCompositingHelper*
 ChildFrameCompositingHelper::CreateForBrowserPlugin(
     const base::WeakPtr<BrowserPlugin>& browser_plugin) {
@@ -133,46 +184,6 @@ void ChildFrameCompositingHelper::ChildFrameGone() {
   UpdateWebLayer(std::move(layer));
 }
 
-// static
-void ChildFrameCompositingHelper::SatisfyCallback(
-    scoped_refptr<ThreadSafeSender> sender,
-    int host_routing_id,
-    const cc::SurfaceSequence& sequence) {
-  // This may be called on either the main or impl thread.
-  sender->Send(new FrameHostMsg_SatisfySequence(host_routing_id, sequence));
-}
-
-// static
-void ChildFrameCompositingHelper::SatisfyCallbackBrowserPlugin(
-    scoped_refptr<ThreadSafeSender> sender,
-    int host_routing_id,
-    int browser_plugin_instance_id,
-    const cc::SurfaceSequence& sequence) {
-  sender->Send(new BrowserPluginHostMsg_SatisfySequence(
-      host_routing_id, browser_plugin_instance_id, sequence));
-}
-
-// static
-void ChildFrameCompositingHelper::RequireCallback(
-    scoped_refptr<ThreadSafeSender> sender,
-    int host_routing_id,
-    const cc::SurfaceId& id,
-    const cc::SurfaceSequence& sequence) {
-  // This may be called on either the main or impl thread.
-  sender->Send(new FrameHostMsg_RequireSequence(host_routing_id, id, sequence));
-}
-
-void ChildFrameCompositingHelper::RequireCallbackBrowserPlugin(
-    scoped_refptr<ThreadSafeSender> sender,
-    int host_routing_id,
-    int browser_plugin_instance_id,
-    const cc::SurfaceId& id,
-    const cc::SurfaceSequence& sequence) {
-  // This may be called on either the main or impl thread.
-  sender->Send(new BrowserPluginHostMsg_RequireSequence(
-      host_routing_id, browser_plugin_instance_id, id, sequence));
-}
-
 void ChildFrameCompositingHelper::OnSetSurface(
     const cc::SurfaceId& surface_id,
     const gfx::Size& frame_size,
@@ -181,31 +192,22 @@ void ChildFrameCompositingHelper::OnSetSurface(
   surface_id_ = surface_id;
   scoped_refptr<ThreadSafeSender> sender(
       RenderThreadImpl::current()->thread_safe_sender());
-  cc::SurfaceLayer::SatisfyCallback satisfy_callback =
-      render_frame_proxy_
-          ? base::Bind(&ChildFrameCompositingHelper::SatisfyCallback, sender,
-                       host_routing_id_)
-          : base::Bind(
-                &ChildFrameCompositingHelper::SatisfyCallbackBrowserPlugin,
-                sender, host_routing_id_,
-                browser_plugin_->browser_plugin_instance_id());
-  cc::SurfaceLayer::RequireCallback require_callback =
-      render_frame_proxy_
-          ? base::Bind(&ChildFrameCompositingHelper::RequireCallback, sender,
-                       host_routing_id_)
-          : base::Bind(
-                &ChildFrameCompositingHelper::RequireCallbackBrowserPlugin,
-                sender, host_routing_id_,
-                browser_plugin_->browser_plugin_instance_id());
+  cc::SurfaceEmbeddingPtr surface_ref;
+  if (render_frame_proxy_)

[Fady Samuel, 2016/12/12 16:25:07]

Braces.

+    surface_ref = new SurfaceEmbedding(sender, host_routing_id_);

[Fady Samuel, 2016/12/12 16:25:07]

It feels like we should have a single instance of this thing, rather than
creating a new one on every OnSetSetSurface

+  else
+    surface_ref = new PluginSurfaceEmbedding(
+        sender, host_routing_id_,
+        browser_plugin_->browser_plugin_instance_id());
   scoped_refptr<cc::SurfaceLayer> surface_layer =
-      cc::SurfaceLayer::Create(satisfy_callback, require_callback);
+      cc::SurfaceLayer::Create(std::move(surface_ref));

[Fady Samuel, 2016/12/12 16:25:07]

A surface embedding is ref counted, right? Do we actually need a copy of this
thing for every surfacelayer?

   // TODO(oshima): This is a stopgap fix so that the compositor does not
   // scaledown the content when 2x frame data is added to 1x parent frame data.
   // Fix this in cc/.
   if (IsUseZoomForDSFEnabled())
     scale_factor = 1.0f;
-
-  surface_layer->SetSurfaceId(surface_id, scale_factor, frame_size);
+  cc::SurfaceInfo info(surface_id, scale_factor, frame_size);
+  surface_layer->SetSurfaceInfo(info);
   surface_layer->SetMasksToBounds(true);
   std::unique_ptr<cc_blink::WebLayerImpl> layer(
       new cc_blink::WebLayerImpl(surface_layer));