WIP Handle AwContents needing multiple live functors.

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 c53081505d10fae594486f9336a97243de958d51..bccada1aa7d06872155acde76c63fd9f4a72e3dc 100644
--- a/android_webview/browser/browser_view_renderer.cc
+++ b/android_webview/browser/browser_view_renderer.cc
@@ -92,7 +92,8 @@ BrowserViewRenderer::BrowserViewRenderer(
     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),
@@ -110,24 +111,22 @@ BrowserViewRenderer::BrowserViewRenderer(
 
 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);
   }
 }
 
@@ -200,22 +199,23 @@ bool BrowserViewRenderer::CanOnDraw() {
 }
 
 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_);
@@ -244,7 +244,7 @@ bool BrowserViewRenderer::OnDrawHardware() {
   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_;
@@ -256,22 +256,36 @@ bool BrowserViewRenderer::OnDrawHardware() {
       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(
@@ -438,11 +452,13 @@ void BrowserViewRenderer::OnComputeScroll(base::TimeTicks 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();