Refactor BrowserViewRenderer-RenderThreadManager relationship.

android_webview/browser/browser_view_renderer.cc

 // Copyright 2014 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 "android_webview/browser/browser_view_renderer.h"
 
 #include <utility>
 
 #include "android_webview/browser/browser_view_renderer_client.h"
 #include "android_webview/browser/child_frame.h"
@@ skipping 74 matching lines @@
     content::WebContents* web_contents) {
   return BrowserViewRendererUserData::GetBrowserViewRenderer(web_contents);
 }
 
 BrowserViewRenderer::BrowserViewRenderer(
     BrowserViewRendererClient* client,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner,
     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),
       view_visible_(false),
       window_visible_(false),
       attached_to_window_(false),
       hardware_enabled_(false),
       dip_scale_(0.f),
       page_scale_factor_(1.f),
       min_page_scale_factor_(0.f),
       max_page_scale_factor_(0.f),
       on_new_picture_enable_(false),
       clear_view_(false),
       offscreen_pre_raster_(false),
       next_compositor_id_(1) {}
 
 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(
     content::WebContents* web_contents) {
   web_contents->SetUserData(kBrowserViewRendererUserDataKey,
                             new BrowserViewRendererUserData(this));
 }
 
 void BrowserViewRenderer::TrimMemory() {
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::TrimMemory");
   // Just set the memory limit to 0 and drop all tiles. This will be reset to
   // normal levels in the next DrawGL call.
   // 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() {
   if (!compositor_) {
     return;
   }
 
   if (!hardware_enabled_) {
     compositor_->SetMemoryPolicy(0u);
     return;
@@ skipping 33 matching lines @@
   if (clear_view_) {
     TRACE_EVENT_INSTANT0("android_webview", "EarlyOut_ClearView",
                          TRACE_EVENT_SCOPE_THREAD);
     return false;
   }
 
   return true;
 }
 
 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_);
   gfx::Rect viewport(surface_size);
   gfx::Rect clip = viewport;
   gfx::Transform transform_for_tile_priority =
       external_draw_constraints_.transform;
 
   // If the WebView is on a layer, WebView does not know what transform is
   // applied onto the layer so global visible rect does not make sense here.
   // In this case, just use the surface rect for tiling.
   gfx::Rect viewport_rect_for_tile_priority;
 
   // Leave viewport_rect_for_tile_priority empty if offscreen_pre_raster_ is on.
   if (!offscreen_pre_raster_ && !external_draw_constraints_.is_layer) {
     viewport_rect_for_tile_priority = last_on_draw_global_visible_rect_;
   }
 
   content::SynchronousCompositor::Frame frame =
       compositor_->DemandDrawHw(surface_size,
                                 gfx::Transform(),
                                 viewport,
                                 clip,
                                 viewport_rect_for_tile_priority,
                                 transform_for_tile_priority);
   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_;
   }
 
   std::unique_ptr<ChildFrame> child_frame = base::WrapUnique(new ChildFrame(
       frame.output_surface_id, std::move(frame.frame),
       GetCompositorID(compositor_), viewport_rect_for_tile_priority.IsEmpty(),
       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())
     return;
 
   cc::CompositorFrameAck frame_ack;
   cc::TransferableResource::ReturnResources(
       child_frame->frame->delegated_frame_data->resource_list,
       &frame_ack.resources);
   content::SynchronousCompositor* compositor =
       compositor_map_[child_frame->compositor_id];
   if (compositor && !frame_ack.resources.empty())
     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;
     content::SynchronousCompositor* compositor = compositor_map_[compositor_id];
     cc::CompositorFrameAck frame_ack;
     frame_ack.resources.swap(iterator->second.resources);
 
     if (compositor && !frame_ack.resources.empty()) {
       compositor->ReturnResources(iterator->second.output_surface_id,
                                   frame_ack);
@@ skipping 106 matching lines @@
   attached_to_window_ = true;
   size_.SetSize(width, height);
   if (offscreen_pre_raster_)
     UpdateMemoryPolicy();
   UpdateCompositorIsActive();
 }
 
 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();
 }
 
 void BrowserViewRenderer::ZoomBy(float delta) {
   if (!compositor_)
     return;
   compositor_->SynchronouslyZoomBy(
       delta, gfx::Point(size_.width() / 2, size_.height() / 2));
 }
 
 void BrowserViewRenderer::OnComputeScroll(base::TimeTicks animation_time) {
   if (!compositor_)
     return;
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::OnComputeScroll");
   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();
 }
 
 bool BrowserViewRenderer::IsVisible() const {
   // Ignore |window_visible_| if |attached_to_window_| is false.
   return view_visible_ && (!attached_to_window_ || window_visible_);
 }
 
 bool BrowserViewRenderer::IsClientVisible() const {
@@ skipping 266 matching lines @@
   base::StringAppendF(&str,
                       "overscroll_rounding_error_: %s ",
                       overscroll_rounding_error_.ToString().c_str());
   base::StringAppendF(
       &str, "on_new_picture_enable: %d ", on_new_picture_enable_);
   base::StringAppendF(&str, "clear_view: %d ", clear_view_);
   return str;
 }
 
 }  // namespace android_webview