Refactor BrowserViewRenderer-RenderThreadManager relationship.

android_webview/browser/browser_view_renderer.cc

Index: android_webview/browser/browser_view_renderer.cc
diff --git a/android_webview/browser/browser_view_renderer.cc b/android_webview/browser/browser_view_renderer.cc
index 9f91213dd0e5b74313ac33d20db2821491d28f87..ce6ecf54a61404a47a4b66ca57c77745b3e4f347 100644
--- a/android_webview/browser/browser_view_renderer.cc
+++ b/android_webview/browser/browser_view_renderer.cc
@@ -92,7 +92,7 @@ BrowserViewRenderer::BrowserViewRenderer(
     bool disable_page_visibility)
     : client_(client),
       ui_task_runner_(ui_task_runner),
-      render_thread_manager_(nullptr),
+      compositor_frame_consumer_(nullptr),
       disable_page_visibility_(disable_page_visibility),
       compositor_(NULL),
       is_paused_(false),
@@ -111,11 +111,23 @@ BrowserViewRenderer::BrowserViewRenderer(
 
 BrowserViewRenderer::~BrowserViewRenderer() {
   DCHECK(compositor_map_.empty());
+  if (compositor_frame_consumer_) {
+    compositor_frame_consumer_->OnCompositorFrameProducerWillDestroy();

[boliu, 2016/04/21 17:05:39]

This seems redundant. Just do SetCompositorFrameConsumer(nullptr) here and looks
things will just work? Don't need the WillDestroy on consumer.

[Tobias Sargeant, 2016/04/21 17:21:12]

OK.

[Tobias Sargeant, 2016/04/21 17:48:27]

Done.

+  }
 }
 
-void BrowserViewRenderer::SetRenderThreadManager(
-    RenderThreadManager* render_thread_manager) {
-  render_thread_manager_ = render_thread_manager;
+void BrowserViewRenderer::SetCompositorFrameConsumer(
+    CompositorFrameConsumer* compositor_frame_consumer) {
+  if (compositor_frame_consumer == compositor_frame_consumer_) {
+    return;
+  }
+  if (compositor_frame_consumer_) {
+    compositor_frame_consumer_->SetCompositorFrameProducer(nullptr);
+  }
+  compositor_frame_consumer_ = compositor_frame_consumer;
+  if (compositor_frame_consumer_) {
+    compositor_frame_consumer_->SetCompositorFrameProducer(this);
+  }
 }
 
 void BrowserViewRenderer::RegisterWithWebContents(
@@ -132,8 +144,8 @@ void BrowserViewRenderer::TrimMemory() {
   // TODO(hush): need to setMemoryPolicy to 0 for non-current compositors too.
   // But WebView only has non-current compositors temporarily. So don't have to
   // do it now.
-  if (!offscreen_pre_raster_)
-    ReleaseHardware();
+  if (compositor_frame_consumer_ && !offscreen_pre_raster_)
+    ReleaseHardware(compositor_frame_consumer_);

[boliu, 2016/04/21 17:05:39]

This code smells bad..

ReleaseHardware "mostly" means set the memory budget to 0. Since the consumer
potentially holds other resources, it's the consumer to ensure those resources
are returned in trim. So ReleaseHardware should happen regardless, and shouldn't
care whether there is a consumer or not.

[Tobias Sargeant, 2016/04/21 17:21:11]

Yes. The test for whether there is a consumer should be pushed down into
ReleaseHardware.

[Tobias Sargeant, 2016/04/21 17:48:27]

Done.

 }
 
 void BrowserViewRenderer::UpdateMemoryPolicy() {
@@ -187,22 +199,24 @@ bool BrowserViewRenderer::CanOnDraw() {
 }
 
 bool BrowserViewRenderer::OnDrawHardware() {
-  DCHECK(render_thread_manager_);
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::OnDrawHardware");
+  if (!compositor_frame_consumer_) {

[boliu, 2016/04/21 17:05:39]

why did a DCHECK become a null check..? bad code smell again

[Tobias Sargeant, 2016/04/21 17:21:12]

If RTM is destroyed before BVR, then unless there's something that stops
OnDrawHardware from happening, we will need to handle not having a
compositor_frame_consumer_.

[boliu, 2016/04/21 17:25:46]

hmm, should the gurantee should be that BVRClient knows RTM is gone, and stop
calling into OnDrawHardware instead?

[boliu, 2016/04/21 18:01:32]

No change?

For now at least, RTM is guaranteed to outlive BVR, so should be fine to turn
this into a DCHECK.

+    return false;
+  }
 
-  render_thread_manager_->InitializeHardwareDrawIfNeededOnUI();
+  compositor_frame_consumer_->InitializeHardwareDrawIfNeededOnUI();
 
   if (!CanOnDraw()) {
     return false;
   }
 
-  render_thread_manager_->SetScrollOffsetOnUI(last_on_draw_scroll_offset_);
+  compositor_frame_consumer_->SetScrollOffsetOnUI(last_on_draw_scroll_offset_);
   hardware_enabled_ = true;
 
   external_draw_constraints_ =
-      render_thread_manager_->GetParentDrawConstraintsOnUI();
+      compositor_frame_consumer_->GetParentDrawConstraintsOnUI();
 
-  ReturnResourceFromParent();
+  ReturnResourceFromParent(compositor_frame_consumer_);
   UpdateMemoryPolicy();
 
   gfx::Size surface_size(size_);
@@ -231,7 +245,7 @@ bool BrowserViewRenderer::OnDrawHardware() {
   if (!frame.frame.get()) {
     TRACE_EVENT_INSTANT0("android_webview", "NoNewFrame",
                          TRACE_EVENT_SCOPE_THREAD);
-    hardware_enabled_ = render_thread_manager_->HasFrameOnUI();
+    hardware_enabled_ = compositor_frame_consumer_->HasFrameOnUI();
     if (!hardware_enabled_)
       UpdateMemoryPolicy();
     return hardware_enabled_;
@@ -243,19 +257,26 @@ bool BrowserViewRenderer::OnDrawHardware() {
       transform_for_tile_priority, offscreen_pre_raster_,
       external_draw_constraints_.is_layer));
 
-  ReturnUnusedResource(render_thread_manager_->PassUncommittedFrameOnUI());
-  render_thread_manager_->SetFrameOnUI(std::move(child_frame));
+  ReturnUnusedResource(compositor_frame_consumer_->PassUncommittedFrameOnUI());
+  compositor_frame_consumer_->SetFrameOnUI(std::move(child_frame));
   return true;
 }
 
-void BrowserViewRenderer::OnParentDrawConstraintsUpdated() {
-  DCHECK(render_thread_manager_);
+void BrowserViewRenderer::OnParentDrawConstraintsUpdated(
+    CompositorFrameConsumer* compositor_frame_consumer) {
+  DCHECK(compositor_frame_consumer_ == compositor_frame_consumer);

[boliu, 2016/04/21 17:05:39]

there is DCHECK_EQ

[boliu, 2016/04/21 17:07:42]

If it's supposed to be 1:many, how are these DCHECKs going to hold?

I think what you are going for is 1:1 at any one point in time, but the link can
change at run time.

[Tobias Sargeant, 2016/04/21 17:21:12]

Eventually I was envisaging that we have an active consumer, and a list of all
consumers. That way every consumer on the list can be signalled to release
memory, but only the one that is active receives frames, and that's the only one
from which we care about updated draw constraints.

[boliu, 2016/04/21 17:25:46]

That's fine, but you need to make this CL consistent

[Tobias Sargeant, 2016/04/21 17:48:27]

Done.

   PostInvalidate();
   external_draw_constraints_ =
-      render_thread_manager_->GetParentDrawConstraintsOnUI();
+      compositor_frame_consumer_->GetParentDrawConstraintsOnUI();
   UpdateMemoryPolicy();
 }
 
+void BrowserViewRenderer::OnCompositorFrameConsumerWillDestroy(
+    CompositorFrameConsumer* compositor_frame_consumer) {
+  DCHECK(compositor_frame_consumer_ == compositor_frame_consumer);
+  compositor_frame_consumer_ = nullptr;

[boliu, 2016/04/21 17:05:39]

SetCompositorFrameConsumer(nullptr)

[Tobias Sargeant, 2016/04/21 17:48:27]

Done.

+}
+
 void BrowserViewRenderer::ReturnUnusedResource(
     std::unique_ptr<ChildFrame> child_frame) {
   if (!child_frame.get() || !child_frame->frame.get())
@@ -271,10 +292,10 @@ void BrowserViewRenderer::ReturnUnusedResource(
     compositor->ReturnResources(child_frame->output_surface_id, frame_ack);
 }
 
-void BrowserViewRenderer::ReturnResourceFromParent() {
-  DCHECK(render_thread_manager_);
-  RenderThreadManager::ReturnedResourcesMap returned_resource_map;
-  render_thread_manager_->SwapReturnedResourcesOnUI(&returned_resource_map);
+void BrowserViewRenderer::ReturnResourceFromParent(
+    CompositorFrameConsumer* compositor_frame_consumer) {
+  CompositorFrameConsumer::ReturnedResourcesMap returned_resource_map;
+  compositor_frame_consumer->SwapReturnedResourcesOnUI(&returned_resource_map);
   for (auto iterator = returned_resource_map.begin();
        iterator != returned_resource_map.end(); iterator++) {
     uint32_t compositor_id = iterator->first;
@@ -401,7 +422,9 @@ void BrowserViewRenderer::OnAttachedToWindow(int width, int height) {
 void BrowserViewRenderer::OnDetachedFromWindow() {
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::OnDetachedFromWindow");
   attached_to_window_ = false;
-  ReleaseHardware();
+  if (compositor_frame_consumer_) {

[boliu, 2016/04/21 17:05:39]

ditto

[Tobias Sargeant, 2016/04/21 17:48:27]

Done.

+    ReleaseHardware(compositor_frame_consumer_);
+  }
   UpdateCompositorIsActive();
 }
 
@@ -419,10 +442,11 @@ void BrowserViewRenderer::OnComputeScroll(base::TimeTicks animation_time) {
   compositor_->OnComputeScroll(animation_time);
 }
 
-void BrowserViewRenderer::ReleaseHardware() {
-  ReturnUnusedResource(render_thread_manager_->PassUncommittedFrameOnUI());
-  ReturnResourceFromParent();
-  DCHECK(render_thread_manager_->ReturnedResourcesEmptyOnUI());
+void BrowserViewRenderer::ReleaseHardware(
+    CompositorFrameConsumer* compositor_frame_consumer) {
+  ReturnUnusedResource(compositor_frame_consumer->PassUncommittedFrameOnUI());
+  ReturnResourceFromParent(compositor_frame_consumer);
+  DCHECK(compositor_frame_consumer->ReturnedResourcesEmptyOnUI());
   hardware_enabled_ = false;
   UpdateMemoryPolicy();
 }