[ABANDONED] Revert of Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink

Revert of Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink (patchset #55 id:1070001 of https://codereview.chromium.org/2493223002/ )

Reason for revert:
Speculative revert for a chrome_public_test_apk breakage.

https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/38004

Original issue's description:
> Moved exo::SurfaceFactoryOwner to its own file and renamed it to exo::CompositorFrameSink.
> 
> CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink.
> 
> CompositorFrameSink is no longer a friend class of exo::Surface.
> 
> Added exo::CompositorFrameSinkHolder class that implements cc::mojom::MojoCompositorFrameSinkClient.
> 
> BUG=659601
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
> 
> Review-Url: https://codereview.chromium.org/2493223002
> Review-Url: https://codereview.chromium.org/2493223002

TBR=reveman@chromium.org,fsamuel@chromium.org,jbauman@chromium.org,sky@chromium.org,sadrul@chromium.org,staraz@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=659601

[yhirano, 2016-12-12 02:16:24]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-12-12 02:16:24]

Created Revert of Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink

[commit-bot: I haz the power, 2016-12-12 02:16:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-12-12 02:40:50]

Seems unlikely given this is a Chrome OS specific change? I haven't dug
deeply though.

Fady

On Sun, Dec 11, 2016, 9:16 PM commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> CQ is trying da patch. Follow status at
>
>
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
>
>
> https://codereview.chromium.org/2562263002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-12-12 03:58:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 03:58:32]

Failed to apply patch for components/exo/surface.cc:
While running git apply --index -p1;
  error: patch failed: components/exo/surface.cc:405
  error: components/exo/surface.cc: patch does not apply

Patch:       components/exo/surface.cc
Index: components/exo/surface.cc
diff --git a/components/exo/surface.cc b/components/exo/surface.cc
index
2c23fc3b970315845e196b8bd4f84ae73ddfbd5b..8c5ed61a34fbf8d36487c17dee035e2b90c4db1a
100644
--- a/components/exo/surface.cc
+++ b/components/exo/surface.cc
@@ -18,7 +18,9 @@
 #include "cc/quads/texture_draw_quad.h"
 #include "cc/resources/single_release_callback.h"
 #include "cc/surfaces/surface.h"
+#include "cc/surfaces/surface_factory.h"
 #include "cc/surfaces/surface_id_allocator.h"
+#include "cc/surfaces/surface_manager.h"
 #include "components/exo/buffer.h"
 #include "components/exo/surface_delegate.h"
 #include "components/exo/surface_observer.h"
@@ -139,24 +141,73 @@
   DISALLOW_COPY_AND_ASSIGN(CustomWindowTargeter);
 };
 
+void SatisfyCallback(cc::SurfaceManager* manager,
+                     const cc::SurfaceSequence& sequence) {
+  std::vector<uint32_t> sequences;
+  sequences.push_back(sequence.sequence);
+  manager->DidSatisfySequences(sequence.frame_sink_id, &sequences);
+}
+
+void RequireCallback(cc::SurfaceManager* manager,
+                     const cc::SurfaceId& id,
+                     const cc::SurfaceSequence& sequence) {
+  cc::Surface* surface = manager->GetSurfaceForId(id);
+  if (!surface) {
+    LOG(ERROR) << "Attempting to require callback on nonexistent surface";
+    return;
+  }
+  surface->AddDestructionDependency(sequence);
+}
+
 }  // 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()) {
-  cc::mojom::MojoCompositorFrameSinkClientPtr frame_sink_holder_ptr;
-  cc::mojom::MojoCompositorFrameSinkClientRequest frame_sink_client_request =
-      mojo::GetProxy(&frame_sink_holder_ptr);
-  std::unique_ptr<CompositorFrameSink> frame_sink(new CompositorFrameSink(
-      frame_sink_id_,
-      aura::Env::GetInstance()->context_factory()->GetSurfaceManager(),
-      std::move(frame_sink_holder_ptr)));
-  compositor_frame_sink_holder_ = new CompositorFrameSinkHolder(
-      this, std::move(frame_sink), std::move(frame_sink_client_request));
+      surface_manager_(
+          aura::Env::GetInstance()->context_factory()->GetSurfaceManager()),
+      factory_owner_(new SurfaceFactoryOwner) {
   window_->SetType(ui::wm::WINDOW_TYPE_CONTROL);
   window_->SetName("ExoSurface");
   window_->SetProperty(kSurfaceKey, this);
@@ -164,6 +215,15 @@
   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);
 }
 
@@ -175,11 +235,22 @@
   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_);
-  compositor_frame_sink_holder_->ActivateFrameCallbacks(&frame_callbacks_);
-  compositor_frame_sink_holder_->CancelFrameCallbacks();
-
-  compositor_frame_sink_holder_->GetCompositorFrameSink()->EvictFrame();
+  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();
+
+  surface_manager_->UnregisterSurfaceFactoryClient(
+      factory_owner_->frame_sink_id_);
 }
 
 // static
@@ -188,7 +259,7 @@
 }
 
 cc::SurfaceId Surface::GetSurfaceId() const {
-  return cc::SurfaceId(frame_sink_id_, local_frame_id_);
+  return cc::SurfaceId(factory_owner_->frame_sink_id_, local_frame_id_);
 }
 
 void Surface::Attach(Buffer* buffer) {
@@ -405,7 +476,7 @@
   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_ = id_allocator_.GenerateId();
+    local_frame_id_ = factory_owner_->id_allocator_->GenerateId();
   }
 
   UpdateSurface(true);
@@ -418,11 +489,9 @@
     window_->layer()->SetBounds(
         gfx::Rect(window_->layer()->bounds().origin(), content_size_));
     window_->layer()->SetShowSurface(
-        cc::SurfaceId(frame_sink_id_, local_frame_id_),
-        base::Bind(&CompositorFrameSinkHolder::Satisfy,
-                   compositor_frame_sink_holder_),
-        base::Bind(&CompositorFrameSinkHolder::Require,
-                   compositor_frame_sink_holder_),
+        cc::SurfaceId(factory_owner_->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_);
     window_->layer()->SetFillsBoundsOpaquely(
         state_.blend_mode == SkBlendMode::kSrc ||
@@ -434,8 +503,7 @@
   pending_damage_.setEmpty();
 
   DCHECK(!current_resource_.id ||
-         compositor_frame_sink_holder_->HasReleaseCallbackForResource(
-             current_resource_.id));
+         factory_owner_->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_);
@@ -540,7 +608,18 @@
 }
 
 void Surface::WillDraw() {
-  compositor_frame_sink_holder_->ActivateFrameCallbacks(&frame_callbacks_);
+  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();
 }
 
 void Surface::CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces() {
@@ -557,12 +636,26 @@
 }
 
 void Surface::OnWindowAddedToRootWindow(aura::Window* window) {
-  window->layer()->GetCompositor()->AddFrameSink(frame_sink_id_);
+  window->layer()->GetCompositor()->AddFrameSink(
+      factory_owner_->frame_sink_id_);
 }
 
 void Surface::OnWindowRemovingFromRootWindow(aura::Window* window,
                                              aura::Window* new_root) {
-  window->layer()->GetCompositor()->RemoveFrameSink(frame_sink_id_);
+  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_;
 }
 
 Surface::State::State() : input_region(SkIRect::MakeLargest()) {}
@@ -650,8 +743,8 @@
                                                  texture_mailbox.target());
     resource.is_overlay_candidate = texture_mailbox.is_overlay_candidate();
 
-    compositor_frame_sink_holder_->AddResourceReleaseCallback(
-        resource.id, std::move(texture_mailbox_release_callback));
+    factory_owner_->release_callbacks_[resource.id] = std::make_pair(
+        factory_owner_, std::move(texture_mailbox_release_callback));
     current_resource_ = resource;
   } else {
     current_resource_.id = 0;…
(message too large)

[yhirano, 2016-12-12 08:00:24]

Description was changed from

==========
Revert of Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink
(patchset #55 id:1070001 of https://codereview.chromium.org/2493223002/ )

Reason for revert:
Speculative revert for a chrome_public_test_apk breakage.

https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...

Original issue's description:
> Moved exo::SurfaceFactoryOwner to its own file and renamed it to
exo::CompositorFrameSink.
> 
> CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink.
> 
> CompositorFrameSink is no longer a friend class of exo::Surface.
> 
> Added exo::CompositorFrameSinkHolder class that implements
cc::mojom::MojoCompositorFrameSinkClient.
> 
> BUG=659601
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
> 
> Review-Url: https://codereview.chromium.org/2493223002
> Review-Url: https://codereview.chromium.org/2493223002

TBR=reveman@chromium.org,fsamuel@chromium.org,jbauman@chromium.org,sky@chromi...
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=659601
==========

to

==========
Revert of Change exo::SurfaceFactoryOwner to exo::ExoCompositorFrameSink
(patchset #55 id:1070001 of https://codereview.chromium.org/2493223002/ )

Reason for revert:
Speculative revert for a chrome_public_test_apk breakage.

https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...

Original issue's description:
> Moved exo::SurfaceFactoryOwner to its own file and renamed it to
exo::CompositorFrameSink.
> 
> CompositorFrameSink implements cc::mojom::MojoCompositorFrameSink.
> 
> CompositorFrameSink is no longer a friend class of exo::Surface.
> 
> Added exo::CompositorFrameSinkHolder class that implements
cc::mojom::MojoCompositorFrameSinkClient.
> 
> BUG=659601
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
> 
> Review-Url: https://codereview.chromium.org/2493223002
> Review-Url: https://codereview.chromium.org/2493223002

TBR=reveman@chromium.org,fsamuel@chromium.org,jbauman@chromium.org,sky@chromi...
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=659601
==========

[yhirano, 2016-12-12 08:00:24]

yhirano@chromium.org changed reviewers:
+ gab@chromium.org

[yhirano, 2016-12-12 08:03:37]

On 2016/12/12 02:40:50, Fady Samuel wrote:
> Seems unlikely given this is a Chrome OS specific change? I haven't dug
> deeply though.
> 
> Fady
> 
> On Sun, Dec 11, 2016, 9:16 PM mailto:commit-bot@chromium.org via
> http://codereview.chromium.org
<mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > CQ is trying da patch. Follow status at
> >
> >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> >
> >
> > https://codereview.chromium.org/2562263002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thank you. I failed to revert the CL and gab@ is trying to revert the other CL.
So I'm waiting for the other CL to be reverted now. By the way,
WebRtcMediaRecorderTest.PeerConnection starts failing in the same range (437705
- 437707) and I see the CL modifies a webrtc/ file. Is it possible that the CL
broke the test?

[Alex Z., 2016-12-12 13:43:33]

On 2016/12/12 08:03:37, yhirano wrote:
> On 2016/12/12 02:40:50, Fady Samuel wrote:
> > Seems unlikely given this is a Chrome OS specific change? I haven't dug
> > deeply though.
> > 
> > Fady
> > 
> > On Sun, Dec 11, 2016, 9:16 PM mailto:commit-bot@chromium.org via
> > http://codereview.chromium.org
> <mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> > 
> > > CQ is trying da patch. Follow status at
> > >
> > >
> > >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> > >
> > >
> > > https://codereview.chromium.org/2562263002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Thank you. I failed to revert the CL and gab@ is trying to revert the other
CL.
> So I'm waiting for the other CL to be reverted now. By the way,
> WebRtcMediaRecorderTest.PeerConnection starts failing in the same range
(437705
> - 437707) and I see the CL modifies a webrtc/ file. Is it possible that the CL
> broke the test?

Unlikely. My webrtc change overrides DesktopMediaListAshTest::TearDown() and 
WebRtcMediaRecorderTest doesn't use that class.