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 d2419a5b64bc41a7e3ad84c24a9f16e80ddad2c3..44598d62cd03c5d8c4db3b41fd65759c01a9e44a 100644
--- a/components/exo/surface.cc
+++ b/components/exo/surface.cc
@@ -162,52 +162,24 @@ 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))),
+      frame_sink_id_(
+          aura::Env::GetInstance()->context_factory()->AllocateFrameSinkId()),
       surface_manager_(
           aura::Env::GetInstance()->context_factory()->GetSurfaceManager()),
-      factory_owner_(new SurfaceFactoryOwner) {
+      weak_ptr_factory_(this) {
+  cc::mojom::MojoCompositorFrameSinkClientPtr pholder;
+  cc::mojom::MojoCompositorFrameSinkClientRequest request =
+      mojo::GetProxy(&pholder);
+  std::unique_ptr<ExoCompositorFrameSink> compositor_frame_sink(
+      new ExoCompositorFrameSink(frame_sink_id_, surface_manager_,
+                                 std::move(pholder)));
+  compositor_frame_sink_holder_ = new CompositorFrameSinkHolder(
+      std::move(compositor_frame_sink), weak_ptr_factory_.GetWeakPtr(),
+      std::move(request));
   window_->SetType(ui::wm::WINDOW_TYPE_CONTROL);
   window_->SetName("ExoSurface");
   window_->SetProperty(kSurfaceKey, this);
@@ -215,15 +187,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,22 +198,11 @@ 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_);
-  active_frame_callbacks_.splice(active_frame_callbacks_.end(),
-                                 frame_callbacks_);
-  for (const auto& frame_callback : active_frame_callbacks_)
-    frame_callback.Run(base::TimeTicks());
-  if (begin_frame_source_ && needs_begin_frame_)
-    begin_frame_source_->RemoveObserver(this);
-
-  factory_owner_->surface_factory_->EvictSurface();
+  compositor_frame_sink_holder_->ActivateFrameCallbacks(frame_callbacks_);

[Fady Samuel, 2016/11/30 16:41:57]

This is a weird API. I really don't understand what it's doing and the line
above. Please clarify? Also, do we need |frame_callbacks_|. Furthermore, you
shouldn't pass things by (non-const) ref. Either pass by pointer if they will
change, or pass by const ref. 

[Alex Z., 2016/11/30 19:04:04]

The comments in surface.h says these callbacks notify the clients when it is a
good time to start producing a new frame. I guess it's similar to what a
BeginFrame does. The callbacks are added in Surface::RequestFrameCallback() in
wayland server. They run in OnBeginFrame(). We might be able to get rid of them
with more plumbing for BeginFrame.

In the line above, since the Surface is being destroyed, we call all frame
callbacks with a null frame time to indicate that they are canceled.

CompositorFrameSinkHolder::ActivateFrameCallbacks() calls std::list::splice(),
which takes either a non-const reference or an rvalue reference.
 

+  compositor_frame_sink_holder_->CancelFrameCallbacks();
 
-  surface_manager_->UnregisterSurfaceFactoryClient(
-      factory_owner_->frame_sink_id_);
+  compositor_frame_sink_holder_->EvictFrame();
 }
 
 // static
@@ -259,7 +211,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 cc::SurfaceId(frame_sink_id_, local_frame_id_);
 }
 
 void Surface::Attach(Buffer* buffer) {
@@ -476,12 +428,13 @@ void Surface::CommitSurfaceHierarchy() {
   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();
+    local_frame_id_ = id_allocator_.GenerateId();
   }
 
   UpdateSurface(true);
 
   if (old_local_frame_id != local_frame_id_) {
+    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
@@ -489,7 +442,7 @@ 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_),
+        cc::SurfaceId(frame_sink_id_, local_frame_id_),
         base::Bind(&SatisfyCallback, base::Unretained(surface_manager_)),
         base::Bind(&RequireCallback, base::Unretained(surface_manager_)),
         content_size_, contents_surface_to_layer_scale, content_size_);
@@ -502,8 +455,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_holder_->HasReleaseCallbacks(current_resource_.id));
 
   // Move pending frame callbacks to the end of frame_callbacks_.
   frame_callbacks_.splice(frame_callbacks_.end(), pending_frame_callbacks_);
@@ -608,18 +562,8 @@ std::unique_ptr<base::trace_event::TracedValue> Surface::AsTracedValue() const {
 }
 
 void Surface::WillDraw() {
-  active_frame_callbacks_.splice(active_frame_callbacks_.end(),
-                                 frame_callbacks_);
-  UpdateNeedsBeginFrame();
-}
-
-void Surface::SetBeginFrameSource(cc::BeginFrameSource* begin_frame_source) {
-  if (begin_frame_source_ && needs_begin_frame_) {
-    begin_frame_source_->RemoveObserver(this);
-    needs_begin_frame_ = false;
-  }
-  begin_frame_source_ = begin_frame_source;
-  UpdateNeedsBeginFrame();
+  compositor_frame_sink_holder_->ActivateFrameCallbacks(frame_callbacks_);
+  compositor_frame_sink_holder_->UpdateNeedsBeginFrame();

[Fady Samuel, 2016/11/30 16:41:57]

I don't think you should expose this?

[Alex Z., 2016/11/30 20:00:30]

Done.

 }
 
 void Surface::CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces() {
@@ -636,26 +580,12 @@ void Surface::OnLostResources() {
 }
 
 void Surface::OnWindowAddedToRootWindow(aura::Window* window) {
-  window->layer()->GetCompositor()->AddFrameSink(
-      factory_owner_->frame_sink_id_);
+  window->layer()->GetCompositor()->AddFrameSink(frame_sink_id_);
 }
 
 void Surface::OnWindowRemovingFromRootWindow(aura::Window* window,
                                              aura::Window* new_root) {
-  window->layer()->GetCompositor()->RemoveFrameSink(
-      factory_owner_->frame_sink_id_);
-}
-
-void Surface::OnBeginFrame(const cc::BeginFrameArgs& args) {
-  while (!active_frame_callbacks_.empty()) {
-    active_frame_callbacks_.front().Run(args.frame_time);
-    active_frame_callbacks_.pop_front();
-  }
-  last_begin_frame_args_ = args;
-}
-
-const cc::BeginFrameArgs& Surface::LastUsedBeginFrameArgs() const {
-  return last_begin_frame_args_;
+  window->layer()->GetCompositor()->RemoveFrameSink(frame_sink_id_);
 }
 
 Surface::State::State() : input_region(SkIRect::MakeLargest()) {}
@@ -743,8 +673,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_holder_->AddResourceReleaseCallback(
+        resource.id, std::move(texture_mailbox_release_callback));
     current_resource_ = resource;
   } else {
     current_resource_.id = 0;
@@ -833,24 +763,9 @@ void Surface::UpdateSurface(bool full_damage) {
   }
 
   frame.render_pass_list.push_back(std::move(render_pass));
-
-  factory_owner_->surface_factory_->SubmitCompositorFrame(
-      local_frame_id_, std::move(frame), cc::SurfaceFactory::DrawCallback());
-}
-
-void Surface::UpdateNeedsBeginFrame() {
-  if (!begin_frame_source_)
-    return;
-
-  bool needs_begin_frame = !active_frame_callbacks_.empty();
-  if (needs_begin_frame == needs_begin_frame_)
-    return;
-
-  needs_begin_frame_ = needs_begin_frame;
-  if (needs_begin_frame)
-    begin_frame_source_->AddObserver(this);
-  else
-    begin_frame_source_->RemoveObserver(this);
+  compositor_frame_sink_holder_->SetNeedsBeginFrame(true);

[Fady Samuel, 2016/11/30 16:41:57]

I don't think this line is necessary anymore?

[Alex Z., 2016/11/30 20:00:30]

Done.

+  compositor_frame_sink_holder_->SubmitCompositorFrame(local_frame_id_,
+                                                       std::move(frame));
 }
 
 int64_t Surface::SetPropertyInternal(const void* key,