WIP Handle AwContents needing multiple live functors.

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 @@
 BrowserViewRenderer* BrowserViewRenderer::FromWebContents(
     content::WebContents* web_contents) {
   return BrowserViewRendererUserData::GetBrowserViewRenderer(web_contents);
 }
 
 BrowserViewRenderer::BrowserViewRenderer(
     BrowserViewRendererClient* client,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner)
     : client_(client),
       ui_task_runner_(ui_task_runner),
-      compositor_frame_consumer_(nullptr),
+      current_compositor_frame_consumer_(nullptr),
+      compositor_frame_consumers_(),

[boliu, 2016/05/04 16:58:52]

not necessary

[Tobias Sargeant, 2016/05/05 18:36:56]

Removed.

       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());
-  SetCompositorFrameConsumer(nullptr);
+  while (compositor_frame_consumers_.size()) {
+    RemoveCompositorFrameConsumer(*compositor_frame_consumers_.begin());
+  }
 }
 
 void BrowserViewRenderer::SetCompositorFrameConsumer(

[boliu, 2016/05/04 16:58:52]

Feels like "AddToSet" and "MakeCurrent" should be two different calls. This
feels kinda sketchy implicitly doing both.

If we always do them at the same time, then at least rename this method to say
it does those two things.

[Tobias Sargeant, 2016/05/05 13:53:19]

I thought about breaking it into two methods, but I think there's always going
to be impedance mismatch between this and java AwContents that manages the
current functor. All AwContents wants to do is set the current functor. We need
to keep a set of consumers associated with the BVR, so that we can clean them up
even if they are not current. But other than that, and the implied constraint
that you can't make a functor current until it has been removed from any other
BVR it is associated with, the set is a private implementation detail.

     CompositorFrameConsumer* compositor_frame_consumer) {
-  if (compositor_frame_consumer == compositor_frame_consumer_) {
+  if (compositor_frame_consumer == current_compositor_frame_consumer_) {
     return;
   }
-  if (compositor_frame_consumer_) {
-    compositor_frame_consumer_->DeleteHardwareRendererOnUI();
-    ReturnUnusedResource(
-        compositor_frame_consumer_->PassUncommittedFrameOnUI());
-    ReturnResourceFromParent(compositor_frame_consumer_);
-    compositor_frame_consumer_->SetCompositorFrameProducer(nullptr);
-  }
-  compositor_frame_consumer_ = compositor_frame_consumer;
-  if (compositor_frame_consumer_) {
-    compositor_frame_consumer_->SetCompositorFrameProducer(this);
+  current_compositor_frame_consumer_ = compositor_frame_consumer;

[boliu, 2016/05/04 16:58:52]

should update parent draw constraints if the current consumer changed

[Tobias Sargeant, 2016/05/05 13:53:19]

Done.

+  LOG(WARNING) << "XXX Setting current_compositor_frame_consumer_ to "
+               << current_compositor_frame_consumer_;
+  if (current_compositor_frame_consumer_) {
+    compositor_frame_consumers_.insert(current_compositor_frame_consumer_);

[boliu, 2016/05/04 16:58:52]

dcheck for duplicates?

[Tobias Sargeant, 2016/05/05 13:53:19]

Insertion of the same value may happen if (for example) when we exit full screen
mode, and the initial functor is set as the current functor again.

+    current_compositor_frame_consumer_->SetCompositorFrameProducer(this);
   }
 }
 
 void BrowserViewRenderer::RegisterWithWebContents(
     content::WebContents* web_contents) {
   web_contents->SetUserData(kBrowserViewRendererUserDataKey,
                             new BrowserViewRendererUserData(this));
 }
 
 void BrowserViewRenderer::TrimMemory() {
@@ skipping 52 matching lines @@
   if (clear_view_) {
     TRACE_EVENT_INSTANT0("android_webview", "EarlyOut_ClearView",
                          TRACE_EVENT_SCOPE_THREAD);
     return false;
   }
 
   return true;
 }
 
 bool BrowserViewRenderer::OnDrawHardware() {
-  DCHECK(compositor_frame_consumer_);
+  DCHECK(current_compositor_frame_consumer_);
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::OnDrawHardware");
 
-  compositor_frame_consumer_->InitializeHardwareDrawIfNeededOnUI();
+  current_compositor_frame_consumer_->InitializeHardwareDrawIfNeededOnUI();
 
   if (!CanOnDraw()) {
     return false;
   }
 
-  compositor_frame_consumer_->SetScrollOffsetOnUI(last_on_draw_scroll_offset_);
+  current_compositor_frame_consumer_->SetScrollOffsetOnUI(
+      last_on_draw_scroll_offset_);
   hardware_enabled_ = true;
 
   external_draw_constraints_ =
-      compositor_frame_consumer_->GetParentDrawConstraintsOnUI();
+      current_compositor_frame_consumer_->GetParentDrawConstraintsOnUI();
 
-  ReturnResourceFromParent(compositor_frame_consumer_);
+  ReturnResourceFromParent(current_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_ = compositor_frame_consumer_->HasFrameOnUI();
+    hardware_enabled_ = current_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(compositor_frame_consumer_->PassUncommittedFrameOnUI());
-  compositor_frame_consumer_->SetFrameOnUI(std::move(child_frame));
+  ReturnUnusedResource(
+      current_compositor_frame_consumer_->PassUncommittedFrameOnUI());
+  current_compositor_frame_consumer_->SetFrameOnUI(std::move(child_frame));
   return true;
 }
 
-void BrowserViewRenderer::OnParentDrawConstraintsUpdated() {
-  DCHECK(compositor_frame_consumer_);
+void BrowserViewRenderer::OnParentDrawConstraintsUpdated(
+    CompositorFrameConsumer* compositor_frame_consumer) {
+  DCHECK(current_compositor_frame_consumer_);
   PostInvalidate();
   external_draw_constraints_ =
-      compositor_frame_consumer_->GetParentDrawConstraintsOnUI();
+      current_compositor_frame_consumer_->GetParentDrawConstraintsOnUI();

[boliu, 2016/05/04 16:58:52]

what if compositor_frame_consumer != current?

[Tobias Sargeant, 2016/05/05 13:53:19]

Should be NOP in that case.

   UpdateMemoryPolicy();
 }
 
-void BrowserViewRenderer::OnCompositorFrameConsumerWillDestroy() {
-  DCHECK(compositor_frame_consumer_);
-  SetCompositorFrameConsumer(nullptr);
+void BrowserViewRenderer::RemoveCompositorFrameConsumer(
+    CompositorFrameConsumer* compositor_frame_consumer) {
+  DCHECK(compositor_frame_consumers_.count(compositor_frame_consumer) == 1);

[boliu, 2016/05/04 16:58:52]

== 1 part is redundant

[Tobias Sargeant, 2016/05/05 13:53:19]

Removed.

+  compositor_frame_consumers_.erase(compositor_frame_consumer);
+  if (current_compositor_frame_consumer_ == compositor_frame_consumer) {
+    current_compositor_frame_consumer_ = nullptr;

[boliu, 2016/05/04 16:58:52]

SetCompositorFrameConsumer(nullptr), maybe that one should be renamed to the
"update current" one

[Tobias Sargeant, 2016/05/05 18:36:56]

Removed the need for this.

+    LOG(WARNING) << "XXX Setting current_compositor_frame_consumer_ to null";

[boliu, 2016/05/04 16:58:52]

So yeah, what's responsible for setting the current consumer now?

[boliu, 2016/05/05 14:09:34]

Didn't really resolve this one

[Tobias Sargeant, 2016/05/05 18:36:56]

I think it's actually reasonable to enforce that you can't remove the current
consumer. When switching back from full screen, we set the view functor to
current, and then destroy the RTM (which causes the BVR to remove it). That's
the only way that this case occurs. Otherwise, we're destroying the whole BVR,
in which case we can just clear the current consumer before removing all of
them.

[boliu, 2016/05/05 21:40:31]

I'm not suggesting don't allow destroying the current consumer. I'm asking
exactly what I asked. What is responsible for setting the new current consumer
*after* this.

Like what code guarantees OnDrawHardware won't be called until after the current
consumer is set again?

+  }
+
+  // At this point the compositor frame consumer has to hand back all resources
+  // to the child compositor.
+  compositor_frame_consumer->DeleteHardwareRendererOnUI();
+  ReturnUnusedResource(compositor_frame_consumer->PassUncommittedFrameOnUI());
+  ReturnResourceFromParent(compositor_frame_consumer);
+  compositor_frame_consumer->SetCompositorFrameProducer(nullptr);
 }
 
 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,
@@ skipping 146 matching lines @@
 }
 
 void BrowserViewRenderer::OnComputeScroll(base::TimeTicks animation_time) {
   if (!compositor_)
     return;
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::OnComputeScroll");
   compositor_->OnComputeScroll(animation_time);
 }
 
 void BrowserViewRenderer::ReleaseHardware() {
-  if (compositor_frame_consumer_) {
-    ReturnUnusedResource(
-        compositor_frame_consumer_->PassUncommittedFrameOnUI());
-    ReturnResourceFromParent(compositor_frame_consumer_);
-    DCHECK(compositor_frame_consumer_->ReturnedResourcesEmptyOnUI());
+  for (auto compositor_frame_consumer_ : compositor_frame_consumers_) {
+    if (compositor_frame_consumer_) {

[boliu, 2016/05/04 16:58:52]

make sure null is never inserted instead of having a check here

[Tobias Sargeant, 2016/05/05 18:36:56]

Done.

+      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_);
 }
 
@@ skipping 264 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