SyncCompositor wait for client before registration

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 f8701c1a00af71945d2af9cb96164ffda94dc0ef..823a89738238014ede7b229ec232e8f3f4ec39b2 100644
--- a/content/browser/android/in_process/synchronous_compositor_impl.cc
+++ b/content/browser/android/in_process/synchronous_compositor_impl.cc
@@ -78,14 +78,9 @@ SynchronousCompositorImpl::SynchronousCompositorImpl(WebContents* contents)
       weak_ptr_factory_(this) {
   DCHECK(contents);
   DCHECK_NE(routing_id_, MSG_ROUTING_NONE);
-  SynchronousCompositorRegistry::GetInstance()->RegisterCompositor(routing_id_,
-                                                                   this);
 }
 
 SynchronousCompositorImpl::~SynchronousCompositorImpl() {
-  SynchronousCompositorRegistry::GetInstance()->UnregisterCompositor(
-      routing_id_, this);
-
   DCHECK(!output_surface_);
   DCHECK(!begin_frame_source_);
   DCHECK(!input_handler_);
@@ -94,7 +89,22 @@ SynchronousCompositorImpl::~SynchronousCompositorImpl() {
 void SynchronousCompositorImpl::SetClient(
     SynchronousCompositorClient* compositor_client) {
   DCHECK(CalledOnValidThread());
+  DCHECK_IMPLIES(compositor_client, !compositor_client_);
+  DCHECK_IMPLIES(!compositor_client, compositor_client_);

[hush (inactive), 2015/05/06 21:57:49]

I'm wondering if the above 2 lines means:
DCHECK_EQ(!!compositor_client, !compositor_client_)

[boliu, 2015/05/06 22:02:49]

Umm...yes, yes it does. DCHECK_IMPLIES is clearer though I think?

+
+  if (!compositor_client) {
+    SynchronousCompositorRegistry::GetInstance()->UnregisterCompositor(
+        routing_id_, this);
+  }
+
   compositor_client_ = compositor_client;
+
+  // SetClient is essentially the constructor and destructor of
+  // SynchronousCompositorImpl.
+  if (compositor_client_) {
+    SynchronousCompositorRegistry::GetInstance()->RegisterCompositor(
+        routing_id_, this);
+  }
 }
 
 // static
@@ -139,21 +149,14 @@ void SynchronousCompositorImpl::DidInitializeRendererObjects(
 void SynchronousCompositorImpl::DidDestroyRendererObjects() {
   DCHECK(output_surface_);
   DCHECK(begin_frame_source_);
+  DCHECK(compositor_client_);
 
   begin_frame_source_->SetCompositor(nullptr);
   output_surface_->SetCompositor(nullptr);
-  if (compositor_client_)
-    compositor_client_->DidDestroyCompositor(this);
-  compositor_client_ = nullptr;
+  SetInputHandler(nullptr);

[hush (inactive), 2015/05/06 21:57:49]

why pulling this line up here? I guess no big deal, because it just sets
input_handler_ to null

[boliu, 2015/05/06 22:02:49]

It also does SetRootLayerScrollOffsetDelegate(NULL);

InputHandler is a renderer object. So I'm trying to make sure that gets detached
first before compositor_client_ becomes invalid

+  compositor_client_->DidDestroyCompositor(this);
   output_surface_ = nullptr;
   begin_frame_source_ = nullptr;
-  SetInputHandler(nullptr);
-}
-
-void SynchronousCompositorImpl::NotifyDidDestroyCompositorToClient() {

[hush (inactive), 2015/05/06 21:57:49]

there was no code calling this function before you removed it?

[boliu, 2015/05/06 22:02:49]

Right. Should have removed this when I added the registry class.

-  if (compositor_client_)
-    compositor_client_->DidDestroyCompositor(this);
-  compositor_client_ = nullptr;
 }
 
 scoped_ptr<cc::CompositorFrame> SynchronousCompositorImpl::DemandDrawHw(
@@ -277,11 +280,10 @@ void SynchronousCompositorImpl::SetInputHandler(
 
 void SynchronousCompositorImpl::DidOverscroll(
     const DidOverscrollParams& params) {
-  if (compositor_client_) {
-    compositor_client_->DidOverscroll(params.accumulated_overscroll,
-                                      params.latest_overscroll_delta,
-                                      params.current_fling_velocity);
-  }
+  DCHECK(compositor_client_);
+  compositor_client_->DidOverscroll(params.accumulated_overscroll,
+                                    params.latest_overscroll_delta,
+                                    params.current_fling_velocity);
 }
 
 void SynchronousCompositorImpl::DidStopFlinging() {
@@ -310,26 +312,23 @@ void SynchronousCompositorImpl::DeliverMessages() {
 }
 
 void SynchronousCompositorImpl::DidActivatePendingTree() {
-  if (compositor_client_)
-    compositor_client_->DidUpdateContent();
+  DCHECK(compositor_client_);
+  compositor_client_->DidUpdateContent();
 }
 
 gfx::ScrollOffset SynchronousCompositorImpl::GetTotalScrollOffset() {
   DCHECK(CalledOnValidThread());
-  if (compositor_client_) {
-    // TODO(miletus): Make GetTotalRootLayerScrollOffset return
-    // ScrollOffset. crbug.com/414283.
-    return gfx::ScrollOffset(
-        compositor_client_->GetTotalRootLayerScrollOffset());
-  }
-  return gfx::ScrollOffset();
+  DCHECK(compositor_client_);
+  // TODO(miletus): Make GetTotalRootLayerScrollOffset return
+  // ScrollOffset. crbug.com/414283.
+  return gfx::ScrollOffset(
+      compositor_client_->GetTotalRootLayerScrollOffset());
 }
 
 bool SynchronousCompositorImpl::IsExternalFlingActive() const {
   DCHECK(CalledOnValidThread());
-  if (compositor_client_)
-    return compositor_client_->IsExternalFlingActive();
-  return false;
+  DCHECK(compositor_client_);
+  return compositor_client_->IsExternalFlingActive();
 }
 
 void SynchronousCompositorImpl::UpdateRootLayerState(
@@ -340,8 +339,7 @@ void SynchronousCompositorImpl::UpdateRootLayerState(
     float min_page_scale_factor,
     float max_page_scale_factor) {
   DCHECK(CalledOnValidThread());
-  if (!compositor_client_)
-    return;
+  DCHECK(compositor_client_);
 
   // TODO(miletus): Pass in ScrollOffset. crbug.com/414283.
   compositor_client_->UpdateRootLayerState(