Rewire Android WebView's compositor changed signal.

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 a1d981e4eba9861699382aafa06155458b2b460c..c5ccc5b3b9abd4af20562047d4a335350988cadb 100644
--- a/android_webview/browser/browser_view_renderer.cc
+++ b/android_webview/browser/browser_view_renderer.cc
@@ -19,6 +19,8 @@
 #include "base/trace_event/trace_event_argument.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/output/compositor_frame_ack.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "gpu/command_buffer/service/gpu_switches.h"
@@ -88,12 +90,16 @@ BrowserViewRenderer* BrowserViewRenderer::FromWebContents(
 }
 
 BrowserViewRenderer::BrowserViewRenderer(
+    content::WebContents* web_contents,
     BrowserViewRendererClient* client,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner)
-    : client_(client),
+    : content::WebContentsObserver(web_contents),
+      client_(client),
       ui_task_runner_(ui_task_runner),
       current_compositor_frame_consumer_(nullptr),
-      compositor_(NULL),
+      compositor_(nullptr),
+      compositor_id_(web_contents->GetRenderProcessHost()->GetID(),
+                     web_contents->GetRenderViewHost()->GetRoutingID()),
       is_paused_(false),
       view_visible_(false),
       window_visible_(false),
@@ -105,8 +111,7 @@ BrowserViewRenderer::BrowserViewRenderer(
       max_page_scale_factor_(0.f),
       on_new_picture_enable_(false),
       clear_view_(false),
-      offscreen_pre_raster_(false),
-      next_compositor_id_(1) {}
+      offscreen_pre_raster_(false) {}
 
 BrowserViewRenderer::~BrowserViewRenderer() {
   DCHECK(compositor_map_.empty());
@@ -247,10 +252,9 @@ bool BrowserViewRenderer::OnDrawHardware() {
   }
 
   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));
+      frame.output_surface_id, std::move(frame.frame), compositor_id_,
+      viewport_rect_for_tile_priority.IsEmpty(), transform_for_tile_priority,
+      offscreen_pre_raster_, external_draw_constraints_.is_layer));
 
   ReturnUnusedResource(
       current_compositor_frame_consumer_->PassUncommittedFrameOnUI());
@@ -304,16 +308,14 @@ 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;
+  for (auto& pair : returned_resource_map) {
+    CompositorID compositor_id = pair.first;
     content::SynchronousCompositor* compositor = compositor_map_[compositor_id];
     cc::CompositorFrameAck frame_ack;
-    frame_ack.resources.swap(iterator->second.resources);
+    frame_ack.resources.swap(pair.second.resources);
 
     if (compositor && !frame_ack.resources.empty()) {
-      compositor->ReturnResources(iterator->second.output_surface_id,
-                                  frame_ack);
+      compositor->ReturnResources(pair.second.output_surface_id, frame_ack);
     }
   }
 }
@@ -467,54 +469,67 @@ gfx::Rect BrowserViewRenderer::GetScreenRect() const {
   return gfx::Rect(client_->GetLocationOnScreen(), size_);
 }
 
-uint32_t BrowserViewRenderer::GetCompositorID(
-    content::SynchronousCompositor* compositor) {
-  for (auto iterator = compositor_map_.begin();
-       iterator != compositor_map_.end(); iterator++) {
-    if (iterator->second == compositor) {
-      return iterator->first;
-    }
-  }
-
-  DCHECK(false);
-  // Return an invalid ID (0), because ID starts with 1.
-  return 0;
-}
-
 void BrowserViewRenderer::DidInitializeCompositor(
-    content::SynchronousCompositor* compositor) {
+    content::SynchronousCompositor* compositor,
+    int process_id,
+    int routing_id) {
   TRACE_EVENT_INSTANT0("android_webview",
                        "BrowserViewRenderer::DidInitializeCompositor",
                        TRACE_EVENT_SCOPE_THREAD);
   DCHECK(compositor);
-  // This happens when id overflows to 0, unlikely in practice.
-  if (next_compositor_id_ == 0)
-    ++next_compositor_id_;
+  CompositorID compositor_id(process_id, routing_id);
+  // This assumes that a RenderViewHost has at most 1 synchronous compositor
+  // througout its lifetime.
+  DCHECK(compositor_map_.count(compositor_id) == 0);
+  compositor_map_[compositor_id] = compositor;
 
-  DCHECK(compositor_map_.find(next_compositor_id_) == compositor_map_.end());
-  compositor_map_[next_compositor_id_] = compositor;
-  next_compositor_id_++;
+  // At this point, the RVHChanged event for the new RVH that contains the
+  // |compositor| might have been fired already, in which case just set the
+  // current compositor with the new compositor.
+  if (!compositor_ && compositor_id == compositor_id_)
+    compositor_ = compositor;
 }
 
 void BrowserViewRenderer::DidDestroyCompositor(
-    content::SynchronousCompositor* compositor) {
+    content::SynchronousCompositor* compositor,
+    int process_id,
+    int routing_id) {
   TRACE_EVENT_INSTANT0("android_webview",
                        "BrowserViewRenderer::DidDestroyCompositor",
                        TRACE_EVENT_SCOPE_THREAD);
-  DCHECK(compositor_);
-  if (compositor_ == compositor)
+  CompositorID compositor_id(process_id, routing_id);
+  DCHECK(compositor_map_.count(compositor_id));
+  if (compositor_ == compositor) {
     compositor_ = nullptr;
-  compositor_map_.erase(GetCompositorID(compositor));
-}
+    compositor_id_ = CompositorID();

[boliu, 2016/06/13 15:30:44]

hmm, would this be more consistent that only OnRVChanged (and the constructor)
modifies compositor_id_? ie just remove this line

I think that's a better mental model, compositor_id_ always points to what
WebContents things is the active RVH, and compositor_ will point to its the sync
compositor if there is one.

[hush (inactive), 2016/06/14 23:48:28]

removed

+  }
 
-void BrowserViewRenderer::DidBecomeCurrent(
-    content::SynchronousCompositor* compositor) {
-  TRACE_EVENT_INSTANT0("android_webview",
-                       "BrowserViewRenderer::DidBecomeCurrent",
-                       TRACE_EVENT_SCOPE_THREAD);
-  DCHECK(compositor);
-  DCHECK(GetCompositorID(compositor));
-  compositor_ = compositor;
+  compositor_map_.erase(compositor_id);
+}
+
+void BrowserViewRenderer::RenderViewHostChanged(
+    content::RenderViewHost* old_host,
+    content::RenderViewHost* new_host) {
+  DCHECK(new_host);
+
+  int process_id = new_host->GetProcess()->GetID();
+  int routing_id = new_host->GetRoutingID();
+  CompositorID compositor_id(process_id, routing_id);
+  // At this point, the current RVH may or may not contain a compositor. So
+  // compositor_ may be nullptr, in which case DidInitializeCompositor()
+  // callback is time when the new compositor is constructed.
+  if (compositor_map_.count(compositor_id)) {
+    compositor_ = compositor_map_[compositor_id];
+    compositor_id_ = compositor_id;
+    // Now transfer states to the new compositor, like memory policy and root
+    // layer scroll offset.
+    UpdateMemoryPolicy();

[boliu, 2016/06/13 15:30:44]

if these two lines are new, then can we add these in a separate CL?

[hush (inactive), 2016/06/14 23:48:28]

sure. 

+    compositor_->DidChangeRootLayerScrollOffset(
+        gfx::ScrollOffset(scroll_offset_dip_));
+  } else {
+    compositor_ = nullptr;

[boliu, 2016/06/13 15:30:44]

ditto about "compositor_id_ always point to active RVH", so just remove this
else block and move the compositor_id_ assignment out of the if block

[hush (inactive), 2016/06/14 23:48:28]

let's discuss in person on this one....

+    compositor_id_ = compositor_id;
+  }
 }
 
 void BrowserViewRenderer::SetDipScale(float dip_scale) {