Have RenderWidgetHostViewAndroid own sync compositor

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index c182e2982a386ead45fdc64039a74409dc498795..43982902d11c29ab23fb175fa9063b4da3f276f8 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -328,7 +328,6 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
                             ui::GestureProviderConfigType::CURRENT_PLATFORM),
                         this),
       stylus_text_selector_(this),
-      using_browser_compositor_(CompositorImpl::IsInitialized()),
       frame_evictor_(new DelegatedFrameEvictor(this)),
       locks_on_frame_count_(0),
       observing_root_window_(false),
@@ -872,7 +871,7 @@ void RenderWidgetHostViewAndroid::CopyFromCompositingSurface(
     return;
   }
   base::TimeTicks start_time = base::TimeTicks::Now();
-  if (using_browser_compositor_ && !IsSurfaceAvailableForCopy()) {
+  if (!sync_compositor_ && !IsSurfaceAvailableForCopy()) {

[no sievers, 2015/10/07 18:52:58]

nit: Actually even better would be to remove the sync_compositor_ check here and
add this:

bool RenderWidgetHostViewAndroid::IsSurfaceAvailableForCopy() const {
  // We can always produce a frame in that case.
  if (sync_compositor_)
    return true;

[boliu, 2015/10/08 03:14:44]

Done.

     callback.Run(SkBitmap(), READBACK_SURFACE_UNAVAILABLE);
     return;
   }
@@ -884,7 +883,7 @@ void RenderWidgetHostViewAndroid::CopyFromCompositingSurface(
   gfx::Rect src_subrect_in_pixel =
       gfx::ConvertRectToPixel(device_scale_factor, src_subrect);
 
-  if (!using_browser_compositor_) {
+  if (sync_compositor_) {
     SynchronousCopyContents(src_subrect_in_pixel, dst_size_in_pixel, callback,
                             preferred_color_type);
     UMA_HISTOGRAM_TIMES("Compositing.CopyFromSurfaceTimeSynchronous",
@@ -1176,6 +1175,11 @@ void RenderWidgetHostViewAndroid::RetainFrame(
   last_frame_info_.reset(new LastFrameInfo(output_surface_id, frame.Pass()));
 }
 
+SynchronousCompositorImpl*
+RenderWidgetHostViewAndroid::GetSynchronousCompositorImpl() {
+  return sync_compositor_.get();
+}
+
 void RenderWidgetHostViewAndroid::SynchronousFrameMetadata(
     const cc::CompositorFrameMetadata& frame_metadata) {
   if (!content_view_core_)
@@ -1208,12 +1212,12 @@ void RenderWidgetHostViewAndroid::SetOverlayVideoMode(bool enabled) {
 bool RenderWidgetHostViewAndroid::SupportsAnimation() const {
   // The synchronous (WebView) compositor does not have a proper browser
   // compositor with which to drive animations.
-  return using_browser_compositor_;
+  return !sync_compositor_;
 }
 
 void RenderWidgetHostViewAndroid::SetNeedsAnimate() {
   DCHECK(content_view_core_window_android_);
-  DCHECK(using_browser_compositor_);
+  DCHECK(!sync_compositor_);
   content_view_core_window_android_->SetNeedsAnimate();
 }
 
@@ -1251,7 +1255,7 @@ void RenderWidgetHostViewAndroid::OnSelectionEvent(
 scoped_ptr<ui::TouchHandleDrawable>
 RenderWidgetHostViewAndroid::CreateDrawable() {
   DCHECK(content_view_core_);
-  if (!using_browser_compositor_)
+  if (sync_compositor_)
     return PopupTouchHandleDrawable::Create(content_view_core_);
 
   return scoped_ptr<ui::TouchHandleDrawable>(new CompositedTouchHandleDrawable(
@@ -1267,6 +1271,7 @@ void RenderWidgetHostViewAndroid::SynchronousCopyContents(
     const gfx::Size& dst_size_in_pixel,
     const ReadbackRequestCallback& callback,
     const SkColorType color_type) {
+  DCHECK(sync_compositor_);
   gfx::Size input_size_in_pixel;
   if (src_subrect_in_pixel.IsEmpty())
     input_size_in_pixel = texture_size_in_layer_;
@@ -1281,14 +1286,6 @@ void RenderWidgetHostViewAndroid::SynchronousCopyContents(
   int output_width = output_size_in_pixel.width();
   int output_height = output_size_in_pixel.height();
 
-  SynchronousCompositor* compositor =
-      SynchronousCompositorImpl::FromID(host_->GetProcess()->GetID(),
-                                        host_->GetRoutingID());
-  if (!compositor) {
-    callback.Run(SkBitmap(), READBACK_FAILED);
-    return;
-  }
-
   SkBitmap bitmap;
   bitmap.allocPixels(SkImageInfo::Make(output_width,
                                        output_height,
@@ -1298,7 +1295,7 @@ void RenderWidgetHostViewAndroid::SynchronousCopyContents(
   canvas.scale(
       (float)output_width / (float)input_size_in_pixel.width(),
       (float)output_height / (float)input_size_in_pixel.height());
-  compositor->DemandDrawSw(&canvas);
+  sync_compositor_->DemandDrawSw(&canvas);
   callback.Run(bitmap, READBACK_SUCCESS);
 }
 
@@ -1473,7 +1470,7 @@ void RenderWidgetHostViewAndroid::SendBeginFrame(base::TimeTicks frame_time,
   TRACE_EVENT1("cc", "RenderWidgetHostViewAndroid::SendBeginFrame",
                "frame_time_us", frame_time.ToInternalValue());
 
-  if (using_browser_compositor_) {
+  if (!sync_compositor_) {
     // TODO(brianderson): Replace this hardcoded deadline after Android
     // switches to Surfaces and the Browser's commit isn't in the critcal path.
     base::TimeTicks deadline = frame_time + (vsync_period * 0.6);
@@ -1483,15 +1480,11 @@ void RenderWidgetHostViewAndroid::SendBeginFrame(base::TimeTicks frame_time,
         cc::BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, frame_time, deadline,
                                    vsync_period, cc::BeginFrameArgs::NORMAL)));
   } else {
-    SynchronousCompositorImpl* compositor = SynchronousCompositorImpl::FromID(
-        host_->GetProcess()->GetID(), host_->GetRoutingID());
-    if (compositor) {
-      // The synchronous compositor synchronously does it's work in this call.
-      // It does not use a deadline.
-      compositor->BeginFrame(cc::BeginFrameArgs::Create(
-          BEGINFRAME_FROM_HERE, frame_time, base::TimeTicks(), vsync_period,
-          cc::BeginFrameArgs::NORMAL));
-    }
+    // The synchronous compositor synchronously does it's work in this call.
+    // It does not use a deadline.
+    sync_compositor_->BeginFrame(cc::BeginFrameArgs::Create(
+        BEGINFRAME_FROM_HERE, frame_time, base::TimeTicks(), vsync_period,
+        cc::BeginFrameArgs::NORMAL));
   }
 }
 
@@ -1609,11 +1602,8 @@ InputEventAckState RenderWidgetHostViewAndroid::FilterInputEvent(
       shim->Send(new GpuMsg_WakeUpGpu);
   }
 
-  SynchronousCompositorImpl* compositor =
-      SynchronousCompositorImpl::FromID(host_->GetProcess()->GetID(),
-                                          host_->GetRoutingID());
-  if (compositor)
-    return compositor->HandleInputEvent(input_event);
+  if (sync_compositor_.get())
+    return sync_compositor_->HandleInputEvent(input_event);
   return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
 }
 
@@ -1745,6 +1735,9 @@ uint32_t RenderWidgetHostViewAndroid::GetSurfaceIdNamespace() {
 
 void RenderWidgetHostViewAndroid::SetContentViewCore(
     ContentViewCoreImpl* content_view_core) {
+  // Android webview does not currently support switching ContentViewCoreImpl.
+  DCHECK_IMPLIES(sync_compositor_,
+                 !content_view_core || !content_view_core_);

[no sievers, 2015/10/07 18:52:58]

So this is what I think we support here:

- setting CVC to NULL when WebContents gets destroyed
- setting CVC to NULL when Java CVC gets destroyed (because CVC is not
functional then)
- reset to null from destructor
- calling this from RWHVA constructor i.e. init with CVC/WebContents
- the renderer crashed and we create a new RWHV for an existing CVC/WebContents.
I'm *guessing* that RWHVA might then get created without a CVC and we later call
SetCVC() from ContentViewCoreImpl::RenderViewHostChanged().

So the check could actually go a bit further and also rule out this:
RWHVA::SetCVC(cvc1);
RWHVA::SetCVC(null);
RWHVA::SetCVC(cvc2);

[boliu, 2015/10/08 03:14:44]

That case sounds the same as normally creating CVC/WebContents, ie constructor
has the CVC pointer?

Other case where CVC can be set after construction include pop up windows (at
least in webview)
Err, how do you do that inside SetCVC itself? Want me to add a bool that's like
"had_cvc_before_"?

[no sievers, 2015/10/08 22:39:29]

Maybe splitting up this function into AttachToContentViewCore(CVC*) and
DetachFromContentViewCore() would also make it a bit clearer what's going on
here. But would still need some bool was_attached_ that can never be reset.

Maybe something for a separate patch. Although... you do care about this, right?
Because if it happened you'd end up with an existing SyncCompositor that points
to the wrong (and possibly deleted) SyncCompositorClient.

(There are also most likely corner case bugs in this anyways since calling
WCV->SetContentViewCore()from CVC is dynamic wrt what RWHV it ends up forwarding
this to.)

[boliu, 2015/10/09 02:22:16]

Yes. But that never happens (yet) in webview, since it would have completely
broken rendering, with or without this patch.

   RemoveLayers();
   StopObservingRootWindow();
 
@@ -1772,9 +1765,10 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
   }
 
   AttachLayers();
-
-  if (!content_view_core_)
+  if (!content_view_core_) {
+    sync_compositor_.reset();

[no sievers, 2015/10/07 18:52:57]

Actually I missed that if you do this, then some of the check for
sync_compositor_ in here might be dangerous and lead you to take the browser
compositor path when the WebContents went away or so.

[boliu, 2015/10/08 03:14:44]

Hmm, you are right. CVC can be set much later after RWHVA is created, so even
checking if there is a sync client set is not good enough in general. Reverted
back to using_browser_compositor_ logic I guess

[no sievers, 2015/10/08 22:39:29]

Ok. Maybe we can revisit later. For example, maybe it's nicer to always have a
fixed interface here that always exist (but will then internally get detached
from the SyncCompositor or maybe just the SyncCompositor getting detached from
its client). It's a bit related to my other comment about WebContents and
views/windows.

[boliu, 2015/10/09 02:22:16]

Maybe don't need that. I think most RVHWA is created with a CVC, so has the
SyncCompositorClient set already. For the edge case RVHWA created for pop up,
could just inherit the value from the source RVHWA where the pop up is created
from.

But yeah, separate patch maybe.

     return;
+  }
 
   if (is_showing_)
     StartObservingRootWindow();
@@ -1789,6 +1783,11 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
       content_view_core_window_android_->GetCompositor()) {
     overscroll_controller_ = CreateOverscrollController(content_view_core_);
   }
+
+  if (!sync_compositor_) {
+    sync_compositor_ = SynchronousCompositorImpl::Create(
+        this, content_view_core_->GetWebContents());
+  }
 }
 
 void RenderWidgetHostViewAndroid::RunAckCallbacks(
@@ -1837,7 +1836,7 @@ void RenderWidgetHostViewAndroid::OnAttachCompositor() {
 
 void RenderWidgetHostViewAndroid::OnDetachCompositor() {
   DCHECK(content_view_core_);
-  DCHECK(using_browser_compositor_);
+  DCHECK(!sync_compositor_);
   RunAckCallbacks(cc::SurfaceDrawStatus::DRAW_SKIPPED);
   overscroll_controller_.reset();
 }