[Android] Consolidate vsync request logic in RWHVAndroid

[Android] Consolidate vsync request logic in RWHVAndroid

The current vsync subscription scheme in RWHVAndroid is rather ad-hoc. Formalize
the request logic by creating explicit request types, avoiding redundant vsync
requests (and the associated JNI call) while also enforcing the validity of
such requests (for WebView).

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

[jdduke (slow), 2014-08-20 22:45:25]

sievers@: WDYT? It's not really that much cleaner, honestly, but at least it
prevents us from pushing BeginFrame messages when such weren't explicitly
requested.

[no sievers, 2014-08-21 17:57:11]

lgtm

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

https://codereview.chromium.org/492923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1257:
DCHECK(observing_root_window_);
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).

https://codereview.chromium.org/492923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1660:
current_vsync_requests & PERSISTENT_BEGIN_FRAME)
nit: braces

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

OK, I pulled the start/stop observing into separate methods, I think it's a
little easier to follow now and we get safer guarantees for resuming a vsync
request when we resume observing.

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

https://codereview.chromium.org/492923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1257:
DCHECK(observing_root_window_);
On 2014/08/21 17:57:10, sievers wrote:
> 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).

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

https://codereview.chromium.org/492923002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1660:
current_vsync_requests & PERSISTENT_BEGIN_FRAME)
On 2014/08/21 17:57:10, sievers wrote:
> nit: braces

Done.

[no sievers, 2014-08-21 20:54:14]

lgtm

[jdduke (slow), 2014-08-21 21:57:23]

OK, one last minor update (sorry!). Bo mentioned I should tweak this to
accomodate WebView, and I think this last version does just that.  Now both
Chrome/WebView share the same observer lifetime, and we simply filter out
BEGIN_FRAME requests for WebView. 

He also mentioned we'll be picking this to M37, so I removed the
"FlushInput(TimeTicks)" change to RenderWidgetHostImpl.  Please take one last
look (boliu@ already lgtm'ed offline). Thanks.

[no sievers, 2014-08-21 22:02:12]

On 2014/08/21 21:57:23, jdduke wrote:
> OK, one last minor update (sorry!). Bo mentioned I should tweak this to
> accomodate WebView, and I think this last version does just that.  Now both
> Chrome/WebView share the same observer lifetime, and we simply filter out
> BEGIN_FRAME requests for WebView. 
> 
> He also mentioned we'll be picking this to M37, so I removed the
> "FlushInput(TimeTicks)" change to RenderWidgetHostImpl.  Please take one last
> look (boliu@ already lgtm'ed offline). Thanks.

lgtm!

[jdduke (slow), 2014-08-21 22:02:39]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-21 22:05:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/492923002/80001

[commit-bot: I haz the power, 2014-08-21 23:21:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-21 23:29:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 23:29:01]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[jdduke (slow), 2014-08-21 23:42:25]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-21 23:44:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/492923002/80001

[commit-bot: I haz the power, 2014-08-22 00:53:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 01:27:23]

Committed patchset #5 (80001) as 291296

[cjhopman, 2014-08-23 00:19:18]

A revert of this CL (patchset #5) has been created in
https://codereview.chromium.org/479083006/ by cjhopman@chromium.org.

The reason for reverting is: BUG=406651

In tests, seems we are calling StartObservingRootWindow multiple times..