Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink

components/exo/surface.cc

Index: components/exo/surface.cc
diff --git a/components/exo/surface.cc b/components/exo/surface.cc
index eda22743a678d08f7d042eeeff16d8bfefd0286d..29aa089ec2c0878e982e000796a91264c3bd7c55 100644
--- a/components/exo/surface.cc
+++ b/components/exo/surface.cc
@@ -24,6 +24,7 @@
 #include "components/exo/buffer.h"
 #include "components/exo/surface_delegate.h"
 #include "components/exo/surface_observer.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
 #include "third_party/khronos/GLES2/gl2.h"
 #include "ui/aura/env.h"
 #include "ui/aura/window_delegate.h"
@@ -162,52 +163,16 @@ void RequireCallback(cc::SurfaceManager* manager,
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
-// SurfaceFactoryOwner, public:
-
-SurfaceFactoryOwner::SurfaceFactoryOwner() {}
-
-////////////////////////////////////////////////////////////////////////////////
-// cc::SurfaceFactoryClient overrides:
-
-void SurfaceFactoryOwner::ReturnResources(
-    const cc::ReturnedResourceArray& resources) {
-  scoped_refptr<SurfaceFactoryOwner> holder(this);
-  for (auto& resource : resources) {
-    auto it = release_callbacks_.find(resource.id);
-    DCHECK(it != release_callbacks_.end());
-    it->second.second->Run(resource.sync_token, resource.lost);
-    release_callbacks_.erase(it);
-  }
-}
-
-void SurfaceFactoryOwner::WillDrawSurface(const cc::LocalFrameId& id,
-                                          const gfx::Rect& damage_rect) {
-  if (surface_)
-    surface_->WillDraw();
-}
-
-void SurfaceFactoryOwner::SetBeginFrameSource(
-    cc::BeginFrameSource* begin_frame_source) {
-  if (surface_)
-    surface_->SetBeginFrameSource(begin_frame_source);
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// SurfaceFactoryOwner, private:
-
-SurfaceFactoryOwner::~SurfaceFactoryOwner() {
-  if (surface_factory_->manager())
-    surface_factory_->manager()->InvalidateFrameSinkId(frame_sink_id_);
-}
-
-////////////////////////////////////////////////////////////////////////////////
 // Surface, public:
 
 Surface::Surface()
     : window_(new aura::Window(new CustomWindowDelegate(this))),
-      surface_manager_(
-          aura::Env::GetInstance()->context_factory()->GetSurfaceManager()),
-      factory_owner_(new SurfaceFactoryOwner) {
+      surface_id_(
+          aura::Env::GetInstance()->context_factory()->AllocateFrameSinkId(),
+          cc::LocalFrameId()),
+      compositor_frame_sink_(new ExoCompositorFrameSink(
+          surface_id_.frame_sink_id(),
+          aura::Env::GetInstance()->context_factory()->GetSurfaceManager())) {
   window_->SetType(ui::wm::WINDOW_TYPE_CONTROL);
   window_->SetName("ExoSurface");
   window_->SetProperty(kSurfaceKey, this);
@@ -215,15 +180,6 @@ Surface::Surface()
   window_->SetEventTargeter(base::WrapUnique(new CustomWindowTargeter));
   window_->set_owned_by_parent(false);
   window_->AddObserver(this);
-  factory_owner_->surface_ = this;
-  factory_owner_->frame_sink_id_ =
-      aura::Env::GetInstance()->context_factory()->AllocateFrameSinkId();
-  factory_owner_->id_allocator_.reset(new cc::SurfaceIdAllocator());
-  surface_manager_->RegisterFrameSinkId(factory_owner_->frame_sink_id_);
-  surface_manager_->RegisterSurfaceFactoryClient(factory_owner_->frame_sink_id_,
-                                                 factory_owner_.get());
-  factory_owner_->surface_factory_.reset(new cc::SurfaceFactory(
-      factory_owner_->frame_sink_id_, surface_manager_, factory_owner_.get()));
   aura::Env::GetInstance()->context_factory()->AddObserver(this);
 }
 
@@ -235,8 +191,6 @@ Surface::~Surface() {
   window_->RemoveObserver(this);
   window_->layer()->SetShowSolidColorContent();
 
-  factory_owner_->surface_ = nullptr;
-
   // Call pending frame callbacks with a null frame time to indicate that they
   // have been cancelled.
   frame_callbacks_.splice(frame_callbacks_.end(), pending_frame_callbacks_);
@@ -247,12 +201,6 @@ Surface::~Surface() {
 
   if (begin_frame_source_ && needs_begin_frame_)
     begin_frame_source_->RemoveObserver(this);
-
-  if (local_frame_id_.is_valid())
-    factory_owner_->surface_factory_->Destroy(local_frame_id_);
-
-  surface_manager_->UnregisterSurfaceFactoryClient(
-      factory_owner_->frame_sink_id_);
 }
 
 // static
@@ -261,7 +209,7 @@ Surface* Surface::AsSurface(const aura::Window* window) {
 }
 
 cc::SurfaceId Surface::GetSurfaceId() const {
-  return cc::SurfaceId(factory_owner_->frame_sink_id_, local_frame_id_);
+  return surface_id_;

[Fady Samuel, 2016/11/16 15:10:58]

I think what we might want to do here is *temporarily* make the current
"local_frame_id()" accessible from ExoCompositorFrameSink and combine that with
the fixed |frame_sink_id_| stored here. 

In the longer term, local_frame_id_s will be allocated by the client anyway so
we won't have this problem.

[Alex Z., 2016/11/16 19:56:16]

Done.

 }
 
 void Surface::Attach(Buffer* buffer) {
@@ -474,22 +422,10 @@ void Surface::CommitSurfaceHierarchy() {
     UpdateResource(true);
   }
 
-  cc::LocalFrameId old_local_frame_id = local_frame_id_;
-  if (needs_commit_to_new_surface_ || !local_frame_id_.is_valid()) {
-    needs_commit_to_new_surface_ = false;
-    local_frame_id_ = factory_owner_->id_allocator_->GenerateId();
-    factory_owner_->surface_factory_->Create(local_frame_id_);
-  }
-
   UpdateSurface(true);
 
-  if (old_local_frame_id.is_valid() && old_local_frame_id != local_frame_id_) {
-    factory_owner_->surface_factory_->SetPreviousFrameSurface(
-        local_frame_id_, old_local_frame_id);
-    factory_owner_->surface_factory_->Destroy(old_local_frame_id);
-  }
-
-  if (old_local_frame_id != local_frame_id_) {
+  if (needs_commit_to_new_surface_) {
+    needs_commit_to_new_surface_ = false;
     float contents_surface_to_layer_scale = 1.0;
     // The bounds must be updated before switching to the new surface, because
     // the layer may be mirrored, in which case a surface change causes the
@@ -497,9 +433,13 @@ void Surface::CommitSurfaceHierarchy() {
     window_->layer()->SetBounds(
         gfx::Rect(window_->layer()->bounds().origin(), content_size_));
     window_->layer()->SetShowSurface(
-        cc::SurfaceId(factory_owner_->frame_sink_id_, local_frame_id_),
-        base::Bind(&SatisfyCallback, base::Unretained(surface_manager_)),
-        base::Bind(&RequireCallback, base::Unretained(surface_manager_)),
+        // TODO(staraz): use the new id created in compositor_frame_sink_
+        // instead
+        surface_id_,
+        base::Bind(&SatisfyCallback,
+                   base::Unretained(compositor_frame_sink_->surface_manager())),
+        base::Bind(&RequireCallback,
+                   base::Unretained(compositor_frame_sink_->surface_manager())),
         content_size_, contents_surface_to_layer_scale, content_size_);
     window_->layer()->SetFillsBoundsOpaquely(
         state_.blend_mode == SkXfermode::kSrc_Mode ||
@@ -510,8 +450,9 @@ void Surface::CommitSurfaceHierarchy() {
   // Reset damage.
   pending_damage_.setEmpty();
 
-  DCHECK(!current_resource_.id ||
-         factory_owner_->release_callbacks_.count(current_resource_.id));
+  DCHECK(
+      !current_resource_.id ||
+      compositor_frame_sink_->release_callbacks_.count(current_resource_.id));
 
   // Move pending frame callbacks to the end of frame_callbacks_.
   frame_callbacks_.splice(frame_callbacks_.end(), pending_frame_callbacks_);
@@ -636,7 +577,7 @@ void Surface::CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces() {
 }
 
 void Surface::OnLostResources() {
-  if (!local_frame_id_.is_valid())
+  if (!surface_id_.is_valid())
     return;
 
   UpdateResource(false);
@@ -645,13 +586,13 @@ void Surface::OnLostResources() {
 
 void Surface::OnWindowAddedToRootWindow(aura::Window* window) {
   window->layer()->GetCompositor()->AddFrameSink(
-      factory_owner_->frame_sink_id_);
+      compositor_frame_sink_->frame_sink_id_);

[Fady Samuel, 2016/11/16 15:10:58]

The exo::Surface should have a fixed frame_sink_id allocated at creation. You
don't need to read it from the CompositorFrameSink.

[Alex Z., 2016/11/16 19:56:16]

Done.

 }
 
 void Surface::OnWindowRemovingFromRootWindow(aura::Window* window,
                                              aura::Window* new_root) {
   window->layer()->GetCompositor()->RemoveFrameSink(
-      factory_owner_->frame_sink_id_);
+      compositor_frame_sink_->frame_sink_id_);

[Fady Samuel, 2016/11/16 15:10:58]

The exo::Surface should have a fixed frame_sink_id allocated at creation. You
don't need to read it from the CompositorFrameSink.

[Alex Z., 2016/11/16 19:56:16]

Done.

 }
 
 void Surface::OnBeginFrame(const cc::BeginFrameArgs& args) {
@@ -751,8 +692,8 @@ void Surface::UpdateResource(bool client_usage) {
                                                  texture_mailbox.target());
     resource.is_overlay_candidate = texture_mailbox.is_overlay_candidate();
 
-    factory_owner_->release_callbacks_[resource.id] = std::make_pair(
-        factory_owner_, std::move(texture_mailbox_release_callback));
+    compositor_frame_sink_->release_callbacks_[resource.id] = std::make_pair(
+        compositor_frame_sink_, std::move(texture_mailbox_release_callback));
     current_resource_ = resource;
   } else {
     current_resource_.id = 0;
@@ -845,8 +786,7 @@ void Surface::UpdateSurface(bool full_damage) {
   cc::CompositorFrame frame;
   frame.delegated_frame_data = std::move(delegated_frame);
 
-  factory_owner_->surface_factory_->SubmitCompositorFrame(
-      local_frame_id_, std::move(frame), cc::SurfaceFactory::DrawCallback());
+  compositor_frame_sink_->SubmitCompositorFrame(std::move(frame));
 }
 
 void Surface::UpdateNeedsBeginFrame() {