cc: Make separate interface for BeginFrame ipc from OutputSurface

content/browser/android/in_process/synchronous_compositor_impl.cc

Index: content/browser/android/in_process/synchronous_compositor_impl.cc
diff --git a/content/browser/android/in_process/synchronous_compositor_impl.cc b/content/browser/android/in_process/synchronous_compositor_impl.cc
index 06be5be5696b865d8f0a91047519bdeccc416dfe..a5c9b42bbd25ef69810ec8f6fe4f764d43c298f7 100644
--- a/content/browser/android/in_process/synchronous_compositor_impl.cc
+++ b/content/browser/android/in_process/synchronous_compositor_impl.cc
@@ -4,9 +4,11 @@
 
 #include "content/browser/android/in_process/synchronous_compositor_impl.h"
 
+#include "base/auto_reset.h"
 #include "base/lazy_instance.h"
 #include "base/message_loop/message_loop.h"
 #include "cc/input/input_handler.h"
+#include "content/browser/android/in_process/synchronous_compositor_external_begin_frame_source.h"
 #include "content/browser/android/in_process/synchronous_compositor_factory_impl.h"
 #include "content/browser/android/in_process/synchronous_input_event_filter.h"
 #include "content/browser/renderer_host/render_widget_host_view_android.h"
@@ -65,8 +67,10 @@ SynchronousCompositorImpl* SynchronousCompositorImpl::FromRoutingID(
 SynchronousCompositorImpl::SynchronousCompositorImpl(WebContents* contents)
     : compositor_client_(NULL),
       output_surface_(NULL),
+      begin_frame_source_(nullptr),
       contents_(contents),
       input_handler_(NULL),
+      invoking_composite_(false),
       weak_ptr_factory_(this) {
   DCHECK(contents);
 }
@@ -74,6 +78,8 @@ SynchronousCompositorImpl::SynchronousCompositorImpl(WebContents* contents)
 SynchronousCompositorImpl::~SynchronousCompositorImpl() {
   if (compositor_client_)
     compositor_client_->DidDestroyCompositor(this);
+  if (begin_frame_source_)
+    begin_frame_source_->SetCompositor(nullptr);
   SetInputHandler(NULL);
 }
 
@@ -94,6 +100,13 @@ void SynchronousCompositor::SetRecordFullDocument(bool record_full_document) {
   g_factory.Get().SetRecordFullDocument(record_full_document);
 }
 
+void SynchronousCompositorImpl::SetExternalBeginFrameSource(
+    SynchronousCompositorExternalBeginFrameSource* begin_frame_source) {
+  begin_frame_source_ = begin_frame_source;

[boliu, 2014/10/31 16:48:21]

It's a bit awkard to DCHECK this, and so let's split this out into
DidInitializeBeginFrameSource and DidDestroyBFS:

DidInititalize(BFS* bfs) {
  DCHECK(!begin_frame_source_);
  DCHECK(!output_surface_);
  DCHECK(bfs);
  begin_frame_source_ = bfs;
  begin_frame_source_->SetCompositor(this);
}

DidDestroy(BFS* bfs) {
  DCHECK(!begin_frame_source_);
  DCHECK_EQ(begin_frame_source_, bfs);
  bfs->SetCompositor(nullptr);
  begin_frame_source_ = nullptr;

  if (output_surface_)
     output_surface_->SetBeginFrameSource(nullptr);
  
  // These 3 lines should probably be a helper function.
  if (compositor_client_)
    compositor_client_->DidDestroyCompositor(this);
  compositor_client_ = nullptr;
}

[simonhong, 2014/11/01 02:13:06]

Done.

+  if (begin_frame_source_)
+    begin_frame_source_->SetCompositor(this);
+}
+
 bool SynchronousCompositorImpl::InitializeHwDraw() {
   DCHECK(CalledOnValidThread());
   DCHECK(output_surface_);
@@ -124,7 +137,12 @@ scoped_ptr<cc::CompositorFrame> SynchronousCompositorImpl::DemandDrawHw(
     const gfx::Transform& transform_for_tile_priority) {
   DCHECK(CalledOnValidThread());
   DCHECK(output_surface_);
+  DCHECK(!invoking_composite_);
+  DCHECK(compositor_client_);
+  DCHECK(begin_frame_source_);
 
+  base::AutoReset<bool> invoking_composite_resetter(&invoking_composite_,
+                                                    true);
   scoped_ptr<cc::CompositorFrame> frame =
       output_surface_->DemandDrawHw(surface_size,
                                     transform,
@@ -135,6 +153,9 @@ scoped_ptr<cc::CompositorFrame> SynchronousCompositorImpl::DemandDrawHw(
   if (frame.get())
     UpdateFrameMetaData(frame->metadata);
 
+  compositor_client_->SetContinuousInvalidate(
+      begin_frame_source_->NeedsBeginFrames());
+
   return frame.Pass();
 }
 
@@ -147,10 +168,20 @@ void SynchronousCompositorImpl::ReturnResources(
 bool SynchronousCompositorImpl::DemandDrawSw(SkCanvas* canvas) {
   DCHECK(CalledOnValidThread());
   DCHECK(output_surface_);
+  DCHECK(!invoking_composite_);
+  DCHECK(compositor_client_);
+  DCHECK(begin_frame_source_);
 
-  scoped_ptr<cc::CompositorFrame> frame = output_surface_->DemandDrawSw(canvas);
+  base::AutoReset<bool> invoking_composite_resetter(&invoking_composite_,
+                                                    true);
+  scoped_ptr<cc::CompositorFrame> frame =
+      output_surface_->DemandDrawSw(canvas);
   if (frame.get())
     UpdateFrameMetaData(frame->metadata);
+
+  compositor_client_->SetContinuousInvalidate(
+      begin_frame_source_->NeedsBeginFrames());
+
   return !!frame.get();
 }
 
@@ -176,15 +207,17 @@ void SynchronousCompositorImpl::DidChangeRootLayerScrollOffset() {
 }
 
 void SynchronousCompositorImpl::DidBindOutputSurface(
-      SynchronousCompositorOutputSurface* output_surface) {
+    SynchronousCompositorOutputSurface* output_surface) {
   DCHECK(CalledOnValidThread());

[boliu, 2014/10/31 16:48:21]

You actually do want DCHECK(begin_frame_source_); then :)
Also should add a DCHECK(output_surface); that should have been here already.

[simonhong, 2014/11/01 02:13:06]

Done.

   output_surface_ = output_surface;
   if (compositor_client_)
     compositor_client_->DidInitializeCompositor(this);
+
+  output_surface_->SetBeginFrameSource(begin_frame_source_);
 }
 
 void SynchronousCompositorImpl::DidDestroySynchronousOutputSurface(
-       SynchronousCompositorOutputSurface* output_surface) {
+    SynchronousCompositorOutputSurface* output_surface) {
   DCHECK(CalledOnValidThread());
 
   // Allow for transient hand-over when two output surfaces may refer to
@@ -194,6 +227,10 @@ void SynchronousCompositorImpl::DidDestroySynchronousOutputSurface(
     if (compositor_client_)
       compositor_client_->DidDestroyCompositor(this);
     compositor_client_ = NULL;
+
+    DCHECK(begin_frame_source_);

[boliu, 2014/10/31 16:48:21]

Don't assume particular destruction order either. You can just remove this new
block I think.

[simonhong, 2014/11/01 02:13:06]

Done.

+    begin_frame_source_->SetCompositor(nullptr);
+    begin_frame_source_ = nullptr;
   }
 }
 
@@ -226,10 +263,14 @@ void SynchronousCompositorImpl::DidStopFlinging() {
     rwhv->DidStopFlinging();
 }
 
-void SynchronousCompositorImpl::SetContinuousInvalidate(bool enable) {
+void SynchronousCompositorImpl::SetContinuousInvalidate(

[boliu, 2014/10/31 16:48:21]

Prefer only have one way to get "needs_begin_frames". So make this
"NeedsBeginFrameChanged()" with the parameter removed, and then just ask BFS for
the value below.

[simonhong, 2014/11/01 02:13:06]

Done.

+    bool needs_begin_frames) const {
   DCHECK(CalledOnValidThread());
+  if (invoking_composite_)
+    return;
+
   if (compositor_client_)
-    compositor_client_->SetContinuousInvalidate(enable);
+    compositor_client_->SetContinuousInvalidate(needs_begin_frames);
 }
 
 InputEventAckState SynchronousCompositorImpl::HandleInputEvent(