[Android] Consolidate vsync request logic in RWHVAndroid

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 062ad08ff44756ac344e7a8bee7ed3671007b692..03a42ee27bf10af087db00f17ae0c7ddcccfe913 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -245,7 +245,7 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
     RenderWidgetHostImpl* widget_host,
     ContentViewCoreImpl* content_view_core)
     : host_(widget_host),
-      needs_begin_frame_(false),
+      outstanding_vsync_requests_(0),
       is_showing_(!widget_host->is_hidden()),
       content_view_core_(NULL),
       ime_adapter_android_(this),
@@ -259,7 +259,6 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
       gesture_text_selector_(this),
       touch_scrolling_(false),
       potentially_active_fling_count_(0),
-      flush_input_requested_(false),
       accelerated_surface_route_id_(0),
       using_synchronous_compositor_(SynchronousCompositorImpl::FromID(
                                         widget_host->GetProcess()->GetID(),
@@ -325,8 +324,8 @@ void RenderWidgetHostViewAndroid::WasShown() {
 
   if (content_view_core_ && !using_synchronous_compositor_) {
     content_view_core_->GetWindowAndroid()->AddObserver(this);
-    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
     observing_root_window_ = true;
+    RequestVSyncUpdate(BEGIN_FRAME);
   }
 }
 
@@ -628,15 +627,12 @@ void RenderWidgetHostViewAndroid::OnDidChangeBodyBackgroundColor(
 }
 
 void RenderWidgetHostViewAndroid::OnSetNeedsBeginFrame(bool enabled) {
-  if (enabled == needs_begin_frame_)
-    return;
-
   TRACE_EVENT1("cc", "RenderWidgetHostViewAndroid::OnSetNeedsBeginFrame",
                "enabled", enabled);
-  if (content_view_core_ && enabled)
-    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
-
-  needs_begin_frame_ = enabled;
+  if (enabled)
+    RequestVSyncUpdate(PERSISTENT_BEGIN_FRAME);
+  else
+    outstanding_vsync_requests_ &= ~PERSISTENT_BEGIN_FRAME;
 }
 
 void RenderWidgetHostViewAndroid::OnStartContentIntent(
@@ -1257,6 +1253,35 @@ void RenderWidgetHostViewAndroid::RemoveLayers() {
     overscroll_effect_->Disable();
 }
 
+void RenderWidgetHostViewAndroid::RequestVSyncUpdate(uint32 requests) {
+  DCHECK(observing_root_window_);

[no sievers, 2014/08/21 17:57:10]

I'm not sure this is always true. I think receiving OnSetNeedsBeginFrame() IPCs
might be racy with
a) the view being hidden by the app
b) the view getting attached to a different CVC

In both cases we remove ourselves as an observer from the root window. Which is
fine as long as if we get reattached we request a vsync update correctly if
necessary (line 328 and 1607).

[jdduke (slow), 2014/08/21 20:50:39]

Ah, I see, it looks like I might need to be more careful about the |WasShown()|
case then.

+  bool should_request_vsync = !outstanding_vsync_requests_ && requests;
+  outstanding_vsync_requests_ |= requests;
+  // Note that if |content_view_core_| is NULL, outstanding vsync requests will
+  // be pushed if/when it next becomes valid via |SetContentViewCore()|.
+  if (content_view_core_ && should_request_vsync) {
+    DCHECK(!using_synchronous_compositor_ || requests == FLUSH_INPUT);
+    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+  }
+}
+
+void RenderWidgetHostViewAndroid::SendBeginFrame(base::TimeTicks frame_time,
+                                                 base::TimeDelta vsync_period) {
+  TRACE_EVENT0("cc", "RenderWidgetHostViewAndroid::SendBeginFrame");
+  base::TimeTicks display_time = frame_time + vsync_period;
+
+  // TODO(brianderson): Use adaptive draw-time estimation.
+  base::TimeDelta estimated_browser_composite_time =
+      base::TimeDelta::FromMicroseconds(
+          (1.0f * base::Time::kMicrosecondsPerSecond) / (3.0f * 60));
+
+  base::TimeTicks deadline = display_time - estimated_browser_composite_time;
+
+  host_->Send(new ViewMsg_BeginFrame(
+      host_->GetRoutingID(),
+      cc::BeginFrameArgs::Create(frame_time, deadline, vsync_period)));
+}
+
 bool RenderWidgetHostViewAndroid::Animate(base::TimeTicks frame_time) {
   bool needs_animate =
       overscroll_effect_ ? overscroll_effect_->Animate(frame_time) : false;
@@ -1405,11 +1430,8 @@ InputEventAckState RenderWidgetHostViewAndroid::FilterInputEvent(
 }
 
 void RenderWidgetHostViewAndroid::OnSetNeedsFlushInput() {
-  if (flush_input_requested_ || !content_view_core_)
-    return;
   TRACE_EVENT0("input", "RenderWidgetHostViewAndroid::OnSetNeedsFlushInput");
-  flush_input_requested_ = true;
-  content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+  RequestVSyncUpdate(FLUSH_INPUT);
 }
 
 BrowserAccessibilityManager*
@@ -1450,9 +1472,8 @@ void RenderWidgetHostViewAndroid::SendTouchEvent(
   // This is good enough as long as the first touch event has Begin semantics
   // and the actual scroll happens on the next vsync.
   // TODO: Is this actually still needed?
-  if (content_view_core_ && observing_root_window_) {
-    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
-  }
+  if (content_view_core_ && observing_root_window_)
+    RequestVSyncUpdate(BEGIN_FRAME);
 }
 
 void RenderWidgetHostViewAndroid::SendMouseEvent(
@@ -1580,8 +1601,10 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
   if (!using_synchronous_compositor_) {
     content_view_core_->GetWindowAndroid()->AddObserver(this);
     observing_root_window_ = true;
-    if (needs_begin_frame_)
-      content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+    // Clear existing vsync requests to allow a request to the new window.
+    uint32 outstanding_vsync_requests = outstanding_vsync_requests_;
+    outstanding_vsync_requests_ = 0;
+    RequestVSyncUpdate(outstanding_vsync_requests);
   }
 
   if (resize)
@@ -1627,27 +1650,18 @@ void RenderWidgetHostViewAndroid::OnVSync(base::TimeTicks frame_time,
   if (!host_)
     return;
 
-  if (flush_input_requested_) {
-    flush_input_requested_ = false;
-    host_->FlushInput();
-  }
+  const uint32 current_vsync_requests = outstanding_vsync_requests_;
+  outstanding_vsync_requests_ = 0;
 
-  TRACE_EVENT0("cc", "RenderWidgetHostViewAndroid::SendBeginFrame");
-  base::TimeTicks display_time = frame_time + vsync_period;
+  if (current_vsync_requests & FLUSH_INPUT)
+    host_->FlushInput(frame_time);
 
-  // TODO(brianderson): Use adaptive draw-time estimation.
-  base::TimeDelta estimated_browser_composite_time =
-      base::TimeDelta::FromMicroseconds(
-          (1.0f * base::Time::kMicrosecondsPerSecond) / (3.0f * 60));
-
-  base::TimeTicks deadline = display_time - estimated_browser_composite_time;
+  if (current_vsync_requests & BEGIN_FRAME ||
+      current_vsync_requests & PERSISTENT_BEGIN_FRAME)

[no sievers, 2014/08/21 17:57:10]

nit: braces

[jdduke (slow), 2014/08/21 20:50:39]

Done.

+    SendBeginFrame(frame_time, vsync_period);
 
-  host_->Send(new ViewMsg_BeginFrame(
-      host_->GetRoutingID(),
-      cc::BeginFrameArgs::Create(frame_time, deadline, vsync_period)));
-
-  if (needs_begin_frame_)
-    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+  if (current_vsync_requests & PERSISTENT_BEGIN_FRAME)
+    RequestVSyncUpdate(PERSISTENT_BEGIN_FRAME);
 }
 
 void RenderWidgetHostViewAndroid::OnAnimate(base::TimeTicks begin_frame_time) {