SyncCompositor wait for client before registration

content/browser/android/in_process/synchronous_compositor_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/android/in_process/synchronous_compositor_impl.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 60 matching lines @@
       output_surface_(NULL),
       begin_frame_source_(nullptr),
       contents_(contents),
       routing_id_(contents->GetRoutingID()),
       input_handler_(NULL),
       is_active_(false),
       renderer_needs_begin_frames_(false),
       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_);
 }
 
 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
 void SynchronousCompositor::SetGpuService(
     scoped_refptr<gpu::InProcessCommandBuffer::Service> service) {
   g_factory.Get().SetDeferredGpuService(service);
 }
 
 // static
 void SynchronousCompositor::SetRecordFullDocument(bool record_full_document) {
@@ skipping 24 matching lines @@
   OnNeedsBeginFramesChange(begin_frame_source_->NeedsBeginFrames());
 
   compositor_client_->DidInitializeCompositor(this);
 
   SetInputHandler(input_handler);
 }
 
 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(
     gfx::Size surface_size,
     const gfx::Transform& transform,
     gfx::Rect viewport,
     gfx::Rect clip,
     gfx::Rect viewport_rect_for_tile_priority,
     const gfx::Transform& transform_for_tile_priority) {
   DCHECK(CalledOnValidThread());
@@ skipping 103 matching lines @@
     input_handler_->SetRootLayerScrollOffsetDelegate(NULL);
 
   input_handler_ = input_handler;
 
   if (input_handler_)
     input_handler_->SetRootLayerScrollOffsetDelegate(this);
 }
 
 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() {
   RenderWidgetHostViewAndroid* rwhv = static_cast<RenderWidgetHostViewAndroid*>(
       contents_->GetRenderWidgetHostView());
   if (rwhv)
     rwhv->DidStopFlinging();
 }
 
 InputEventAckState SynchronousCompositorImpl::HandleInputEvent(
     const blink::WebInputEvent& input_event) {
   DCHECK(CalledOnValidThread());
   return g_factory.Get().synchronous_input_event_filter()->HandleInputEvent(
       contents_->GetRoutingID(), input_event);
 }
 
 void SynchronousCompositorImpl::DeliverMessages() {
   ScopedVector<IPC::Message> messages;
   output_surface_->GetMessagesToDeliver(&messages);
   RenderProcessHost* rph = contents_->GetRenderProcessHost();
   for (ScopedVector<IPC::Message>::const_iterator i = messages.begin();
        i != messages.end();
        ++i) {
     rph->OnMessageReceived(**i);
   }
 }
 
 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(
     const gfx::ScrollOffset& total_scroll_offset,
     const gfx::ScrollOffset& max_scroll_offset,
     const gfx::SizeF& scrollable_size,
     float page_scale_factor,
     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(
       gfx::ScrollOffsetToVector2dF(total_scroll_offset),
       gfx::ScrollOffsetToVector2dF(max_scroll_offset),
       scrollable_size,
       page_scale_factor,
       min_page_scale_factor,
       max_page_scale_factor);
 }
@@ skipping 13 matching lines @@
     g_factory.Get();  // Ensure it's initialized.
     SynchronousCompositorImpl::CreateForWebContents(contents);
   }
   SynchronousCompositorImpl* instance =
       SynchronousCompositorImpl::FromWebContents(contents);
   DCHECK(instance);
   instance->SetClient(client);
 }
 
 }  // namespace content