Android: Consolidate and simplify VSync logic

Android: Consolidate and simplify VSync logic

This moves handling of VSync over to WindowAndroid.
It can have one client to receive vsync signals 
for the main browser-side scheduling logic.
It also forwards the vsync to the WindowAndroidObservers,
i.e. RenderWidgetHostView where it's used for sending
BeginFrame signals to the renderer.

This prepares for consolidating the scheduling logic
in a single place upstream.

Which will then further allow us to use the
cc scheduler (with SingleThreadProxy) and
unblock the landing of https://codereview.chromium.org/134623005/.

BUG=234173, 363479
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266995

[no sievers, 2014-04-15 21:19:26]

sami: everything

dtrainor: Java things (it mostly removes code)

brianderson: at least render_widget_host_view_android.cc

jared: to make sure I didn't accidentally break anything input event or
overscroll related in render_widget_host_view_android.cc

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:492: "enabled",
enabled);
Hmm, can this have an early out if enabled == needs_begine_frame_?
Does it happen on every frame or is it send from the renderer only when the
state changes?

https://codereview.chromium.org/236193013/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/236193013/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:38:
private final WindowAndroid mRootWindow;
@dtrainor: Is it ok to hold a ref to the WindowAndroid here?

[jdduke (slow), 2014-04-15 22:08:43]

Sweet hallelujah this is nice. overscroll/input in
render_widget_host_view_android lgtm.

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:492: "enabled",
enabled);
On 2014/04/15 21:19:26, sievers wrote:
> Hmm, can this have an early out if enabled == needs_begine_frame_?
> Does it happen on every frame or is it send from the renderer only when the
> state changes?

Yeah, I think an early out should work as long as we RequestVSyncUpdate when a
valid content_view_core_ is initialized (looks like you've added that already).

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1258:
content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
Probably worth making
|content_view_core_->GetWindowAndroid()->RequestVSyncUpdate()| a single member
function |RequestVSyncUpdate()|.

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1307:
requested_vsync_for_touch_event_ = false;
Nit: Maybe move |requested_vsync_for_touch_event_ = false;| to the top of this
code block so it doesn't get lost in the BeginFrame flow.

[no sievers, 2014-04-16 01:19:54]

https://codereview.chromium.org/236193013/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/236193013/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:115:
TraceEvent.instant("ContentViewRenderView:bail");
So the 'bail' path should probably call mRootWindow.requestVSyncUpdate().

[David Trainor- moved to gerrit, 2014-04-16 06:18:19]

lgtm

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:30:
public class ContentViewRenderView extends FrameLayout implements
WindowAndroid.VSyncClient {
Indent +8

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right):

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/VSyncMonitor.java:68: public
VSyncMonitor(Context context, VSyncMonitor.Listener listener, boolean
enableJBVSync) {
want to take a stab at these constructor javadocs?

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:52: @Override public
void onVSync(VSyncMonitor monitor,
new line after @Override (not 100% sure on if this style is okay).

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:53: long
vsyncTimeMicros) {
does this fit on the above line?

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:55:
mVSyncClient.onVSync(vsyncTimeMicros);
{} around this or move it to the above line

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:56:
nativeOnVSync(mNativeWindowAndroid, vsyncTimeMicros);
assert mNativeWindowAndroid != 0 or if check?

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:236: public void
onVSync(long vsyncTimeMicros);
Javadoc on this method.  Don't need public on interface methods (we have it
below too but that isn't needed either).

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:242: public void
setVSyncClient(VSyncClient client) {
Fix javadoc (@param client xyz)

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:247: @CalledByNative
Move below javadoc

[Sami, 2014-04-16 17:26:53]

Yay, WindowAndroid is a much better home for this stuff. Thanks!

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:493: if
(content_view_core_ && enabled)
Are we sure the calls here are balanced and never overlap badly? Maybe we can
adds some asserts to keep it that way. IIRC the problem was that the renderer
was doing something funky during navigation and the BeginFrame-disable from the
earlier page ended up disabling BeginFrame for the new page.

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1167: // Send a
proactive BeginFrame on the next vsync to reduce latency.
I don't think this is needed any more since VSyncMonitor triggers immediately
when activated from idle. Let's make sure with a trace though :)

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1300:
base::TimeDelta::FromMilliseconds(1000.0 / (3 * 60));
This drops some precision -- better use FromMicroseconds.

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(left):

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:120:
private static final long INPUT_EVENT_LAG_FROM_VSYNC_MICROSECONDS = 3200;
Should probably verify with a trace, but does the fact that we now buffer input
events until vsync mean that we no longer need this hack for ICS?

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:243: assert
mVSyncClient == null || client == null;
++Indent (here and below)

[brianderson, 2014-04-17 01:05:02]

https://codereview.chromium.org/236193013/diff/40001/ui/base/android/window_a...
File ui/base/android/window_android_observer.h (right):

https://codereview.chromium.org/236193013/diff/40001/ui/base/android/window_a...
ui/base/android/window_android_observer.h:19: base::TimeDelta vsync_period) {}
Should this also be pure virtual?

[no sievers, 2014-04-25 22:23:33]

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:492: "enabled",
enabled);
On 2014/04/15 22:08:43, jdduke wrote:
> On 2014/04/15 21:19:26, sievers wrote:
> > Hmm, can this have an early out if enabled == needs_begine_frame_?
> > Does it happen on every frame or is it send from the renderer only when the
> > state changes?
> 
> Yeah, I think an early out should work as long as we RequestVSyncUpdate when a
> valid content_view_core_ is initialized (looks like you've added that
already).

Done.

https://codereview.chromium.org/236193013/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1307:
requested_vsync_for_touch_event_ = false;
On 2014/04/15 22:08:43, jdduke wrote:
> Nit: Maybe move |requested_vsync_for_touch_event_ = false;| to the top of this
> code block so it doesn't get lost in the BeginFrame flow.

Hm, I left it as is because it otherwise hides the fact that you need a valid
content_view_core_ pointer to be able to make this call.

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:493: if
(content_view_core_ && enabled)
On 2014/04/16 17:26:54, Sami wrote:
> Are we sure the calls here are balanced and never overlap badly? Maybe we can
> adds some asserts to keep it that way. IIRC the problem was that the renderer
> was doing something funky during navigation and the BeginFrame-disable from
the
> earlier page ended up disabling BeginFrame for the new page.

Maybe that was just a problem with CVC<->RWHV badness? There is a whole bunch of
bugs because of that.
I don't think you can get IPC messages in RWHV from the wrong compositor
(RenderWidget and RWHV should be 1:1). Otherwise you'd also be able to receive
CompositorFrame messages from the wrong content :)

With some logging I do see a few transitions in terms of SetNeedsBeginFrame()
when I navigate from one domain to another, both for previous RWHV/OS and the
new pair. But doesn't look like there would be a problem.

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1167: // Send a
proactive BeginFrame on the next vsync to reduce latency.
On 2014/04/16 17:26:54, Sami wrote:
> I don't think this is needed any more since VSyncMonitor triggers immediately
> when activated from idle. Let's make sure with a trace though :) 

Chatted with Brian. I'll leave it in this patch just to make sure it does not
regress because of the extra latency. The render compositor will only send
SetNeedsBeginFrame when needs_redraw becomes true, which might only be after we
send a scrollBy on the next vsync or so.

FWIU with this logic:

vsync1
BeginGesture
requestVsync()
SendBeginFrame() 1

vsync2
SendBeginFrame() 2
scroll

Now when the scroll happens in the renderer, the scheduler there falls into the
retroactive  BeginFrame logic and can draw immediately. I guess SendBeginFrame()
1 is not even needed.

Without this logic:

vsync1
BeginGesture

vsync2
scroll
renderer needs redraw
OnSetNeedsBeginFrame
requestVSync
OnVSync

Does that make sense?

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1294: if
(needs_begin_frame_ || requested_vsync_for_touch_event_) {
I removed this if-condition as it seems to be a functional change from before,
where we'd allow spurious extra BeginFrames (according to
MAX_AUTO_ONVSYNC_COUNT).

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1300:
base::TimeDelta::FromMilliseconds(1000.0 / (3 * 60));
On 2014/04/16 17:26:54, Sami wrote:
> This drops some precision -- better use FromMicroseconds.

Done.

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(left):

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:120:
private static final long INPUT_EVENT_LAG_FROM_VSYNC_MICROSECONDS = 3200;
On 2014/04/16 17:26:54, Sami wrote:
> Should probably verify with a trace, but does the fact that we now buffer
input
> events until vsync mean that we no longer need this hack for ICS?

I thought that the deadline scheduler and retroactive BeginFrame eliminate the
need for this. But now that I look again, I'm actually not sure what it does :)

Note that this is used on JB and up in the code below but only when there are no
vsync subscribers (?).

You introduced it in 'Change-Id: I3df344b8a0fecda7c9c9a49be892875c01afeea8'
(patch set 2).

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/236193013/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:30:
public class ContentViewRenderView extends FrameLayout implements
WindowAndroid.VSyncClient {
On 2014/04/16 06:18:20, David Trainor wrote:
> Indent +8

there's actually no line break here :)

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/VSyncMonitor.java (right):

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/VSyncMonitor.java:68: public
VSyncMonitor(Context context, VSyncMonitor.Listener listener, boolean
enableJBVSync) {
On 2014/04/16 06:18:20, David Trainor wrote:
> want to take a stab at these constructor javadocs?

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:52: @Override public
void onVSync(VSyncMonitor monitor,
On 2014/04/16 06:18:20, David Trainor wrote:
> new line after @Override (not 100% sure on if this style is okay).

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:53: long
vsyncTimeMicros) {
On 2014/04/16 06:18:20, David Trainor wrote:
> does this fit on the above line?

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:55:
mVSyncClient.onVSync(vsyncTimeMicros);
On 2014/04/16 06:18:20, David Trainor wrote:
> {} around this or move it to the above line

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:56:
nativeOnVSync(mNativeWindowAndroid, vsyncTimeMicros);
On 2014/04/16 06:18:20, David Trainor wrote:
> assert mNativeWindowAndroid != 0 or if check?

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:236: public void
onVSync(long vsyncTimeMicros);
On 2014/04/16 06:18:20, David Trainor wrote:
> Javadoc on this method.  Don't need public on interface methods (we have it
> below too but that isn't needed either).

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:242: public void
setVSyncClient(VSyncClient client) {
On 2014/04/16 06:18:20, David Trainor wrote:
> Fix javadoc (@param client xyz)

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:243: assert
mVSyncClient == null || client == null;
On 2014/04/16 17:26:54, Sami wrote:
> ++Indent (here and below)

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:247: @CalledByNative
On 2014/04/16 06:18:20, David Trainor wrote:
> Move below javadoc

Done.

https://codereview.chromium.org/236193013/diff/40001/ui/base/android/window_a...
File ui/base/android/window_android_observer.h (right):

https://codereview.chromium.org/236193013/diff/40001/ui/base/android/window_a...
ui/base/android/window_android_observer.h:19: base::TimeDelta vsync_period) {}
On 2014/04/17 01:05:02, brianderson wrote:
> Should this also be pure virtual?

Done.

[brianderson, 2014-04-25 23:26:02]

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1167: // Send a
proactive BeginFrame on the next vsync to reduce latency.
On 2014/04/25 22:23:33, sievers wrote:
> On 2014/04/16 17:26:54, Sami wrote:
> > I don't think this is needed any more since VSyncMonitor triggers
immediately
> > when activated from idle. Let's make sure with a trace though :) 
> 
> Chatted with Brian. I'll leave it in this patch just to make sure it does not
> regress because of the extra latency. The render compositor will only send
> SetNeedsBeginFrame when needs_redraw becomes true, which might only be after
we
> send a scrollBy on the next vsync or so.
> 
> FWIU with this logic:
> 
> vsync1
> BeginGesture
> requestVsync()
> SendBeginFrame() 1
> 
> vsync2
> SendBeginFrame() 2
> scroll
> 
> Now when the scroll happens in the renderer, the scheduler there falls into
the
> retroactive  BeginFrame logic and can draw immediately. I guess
SendBeginFrame()
> 1 is not even needed.
> 
> Without this logic:
> 
> vsync1
> BeginGesture
> 
> vsync2
> scroll
> renderer needs redraw
> OnSetNeedsBeginFrame
> requestVSync
> OnVSync
> 
> Does that make sense?

Does the Browser send a BeginFrame to the Renderer even before it's requested
it? I thought that was the case, so I think we should keep this logic and what
you say makes sense.

If the Browser doesn't send the BeginFrame early, I think we should fix that in
a separate patch and still keep this logic.

[no sievers, 2014-04-28 16:13:53]

On 2014/04/25 23:26:02, brianderson wrote:
>
https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_android.cc:1167: // Send
a
> proactive BeginFrame on the next vsync to reduce latency.
> On 2014/04/25 22:23:33, sievers wrote:
> > On 2014/04/16 17:26:54, Sami wrote:
> > > I don't think this is needed any more since VSyncMonitor triggers
> immediately
> > > when activated from idle. Let's make sure with a trace though :) 
> > 
> > Chatted with Brian. I'll leave it in this patch just to make sure it does
not
> > regress because of the extra latency. The render compositor will only send
> > SetNeedsBeginFrame when needs_redraw becomes true, which might only be after
> we
> > send a scrollBy on the next vsync or so.
> > 
> > FWIU with this logic:
> > 
> > vsync1
> > BeginGesture
> > requestVsync()
> > SendBeginFrame() 1
> > 
> > vsync2
> > SendBeginFrame() 2
> > scroll
> > 
> > Now when the scroll happens in the renderer, the scheduler there falls into
> the
> > retroactive  BeginFrame logic and can draw immediately. I guess
> SendBeginFrame()
> > 1 is not even needed.
> > 
> > Without this logic:
> > 
> > vsync1
> > BeginGesture
> > 
> > vsync2
> > scroll
> > renderer needs redraw
> > OnSetNeedsBeginFrame
> > requestVSync
> > OnVSync
> > 
> > Does that make sense?
> 
> Does the Browser send a BeginFrame to the Renderer even before it's requested
> it? I thought that was the case, so I think we should keep this logic and what
> you say makes sense.

Yes, it sends it when there is a touch event, see ContentViewCore.java and
mRequestedVSyncForInput where
it would send at least one unsolicited BeginFrame. So this code here would keep
that behavior.

> 
> If the Browser doesn't send the BeginFrame early, I think we should fix that
in
> a separate patch and still keep this logic.

[Sami, 2014-04-28 16:21:52]

On 2014/04/28 16:13:53, sievers wrote:
> On 2014/04/25 23:26:02, brianderson wrote:
> >
>
https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
> > File content/browser/renderer_host/render_widget_host_view_android.cc
(right):
> > 
> >
>
https://codereview.chromium.org/236193013/diff/40001/content/browser/renderer...
> > content/browser/renderer_host/render_widget_host_view_android.cc:1167: //
Send
> a
> > proactive BeginFrame on the next vsync to reduce latency.
> > On 2014/04/25 22:23:33, sievers wrote:
> > > On 2014/04/16 17:26:54, Sami wrote:
> > > > I don't think this is needed any more since VSyncMonitor triggers
> > immediately
> > > > when activated from idle. Let's make sure with a trace though :) 
> > > 
> > > Chatted with Brian. I'll leave it in this patch just to make sure it does
> not
> > > regress because of the extra latency. The render compositor will only send
> > > SetNeedsBeginFrame when needs_redraw becomes true, which might only be
after
> > we
> > > send a scrollBy on the next vsync or so.
> > > 
> > > FWIU with this logic:
> > > 
> > > vsync1
> > > BeginGesture
> > > requestVsync()
> > > SendBeginFrame() 1
> > > 
> > > vsync2
> > > SendBeginFrame() 2
> > > scroll
> > > 
> > > Now when the scroll happens in the renderer, the scheduler there falls
into
> > the
> > > retroactive  BeginFrame logic and can draw immediately. I guess
> > SendBeginFrame()
> > > 1 is not even needed.
> > > 
> > > Without this logic:
> > > 
> > > vsync1
> > > BeginGesture
> > > 
> > > vsync2
> > > scroll
> > > renderer needs redraw
> > > OnSetNeedsBeginFrame
> > > requestVSync
> > > OnVSync
> > > 
> > > Does that make sense?

ScrollBegin and ScrollUpdate should be getting sent in the same vsync and not
consecutive ones. At least that was the case last time I checked.

> > Does the Browser send a BeginFrame to the Renderer even before it's
requested
> > it? I thought that was the case, so I think we should keep this logic and
what
> > you say makes sense.
> 
> Yes, it sends it when there is a touch event, see ContentViewCore.java and
> mRequestedVSyncForInput where
> it would send at least one unsolicited BeginFrame. So this code here would
keep
> that behavior.
> 
> > 
> > If the Browser doesn't send the BeginFrame early, I think we should fix that
> in
> > a separate patch and still keep this logic.

I'm all for keeping the logic too, if it turns out we still need it that is :)
It seems weird to have two mechanisms that are trying to achieve the same thing.
It's fine to fix it in another patch though, since this is already scary enough.
Just leave a TODO here?

[Sami, 2014-04-28 17:28:30]

lgtm with the TODO about figuring out proactive BeginFrames.

[no sievers, 2014-04-28 23:19:08]

+Ted for OWNERS

[Ted C, 2014-04-28 23:28:46]

lgtm - OWNERS

[no sievers, 2014-04-29 23:30:49]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-04-29 23:33:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/236193013/180001

[commit-bot: I haz the power, 2014-04-29 23:40:35]

Change committed as 266995