[Android] Provide synchronized input flush with BeginFrame

[Android] Provide synchronized input flush with BeginFrame

The BufferedInputRouter relies on periodic flush signals.  Provide hooks for
such flush requests and signals via the RenderWidgetHostView, and wire this
to BeginFrame messages on Android. Expose the functionality behind the
--enable-buffered-input-router flag.

BUG=245499, 264869
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225387

[jdduke (slow), 2013-09-12 21:05:42]

PTAL.

aelias@: render_widget_host_impl.{h,cc} and
render_widget_host_view_android.{h,cc}.

brianderson@: Could you look at the hook up to BeginFrame in
render_widget_host_view_android.{h,cc}?  Going forward, as BeginFrame is adopted
by other platforms, we'll want to unify the handling of BeginFrame and input
flushing, but this is probably the simplest route for now.

[aelias_OOO_until_Jul13, 2013-09-12 22:50:25]

lgtm

You'll need another OWNER for content/port/browser/

[jdduke (slow), 2013-09-13 18:25:39]

@brianderson: Any objections here?

[brianderson, 2013-09-13 21:49:27]

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2155: void
RenderWidgetHostImpl::SetNeedsFlush() {
I see that this function existed before, but was empty. Who calls this function?

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:850:
content_view_core_->SetNeedsBeginFrame(true);
Can you rename ContentViewCore's SetNeedsBeginFrame to AddBeginFrameSubscriber
as well?

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_base.cc:460: // The
notification can safely be ignored by most implementations.
Can you explain why?

https://codereview.chromium.org/23983036/diff/10001/content/public/common/con...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/23983036/diff/10001/content/public/common/con...
content/public/common/content_switches.cc:335: // Batch and synchronize input
event delivery to and response from the renderer.
Did you forget to delete "response"?

[jdduke (slow), 2013-09-16 15:16:04]

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2155: void
RenderWidgetHostImpl::SetNeedsFlush() {
On 2013/09/13 21:49:27, Brian Anderson wrote:
> I see that this function existed before, but was empty. Who calls this
function?

Nobody, currently; this was added in the patch that added the
BufferedInputRouter. I wanted to keep that self-contained, and have the "wiring"
patch (this patch) separate.

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:850:
content_view_core_->SetNeedsBeginFrame(true);
On 2013/09/13 21:49:27, Brian Anderson wrote:
> Can you rename ContentViewCore's SetNeedsBeginFrame to AddBeginFrameSubscriber
> as well?

Done.

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/23983036/diff/10001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_base.cc:460: // The
notification can safely be ignored by most implementations.
On 2013/09/13 21:49:27, Brian Anderson wrote:
> Can you explain why?

In the comment? Or to you? :) 

This was previously used when we blocked the "BeginFrame" signal during a flush,
but we don't do that any longer, so it's actually not even used by Android.  I
was wondering if I should rip it out entirely, but I think it could be useful.

https://codereview.chromium.org/23983036/diff/10001/content/public/common/con...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/23983036/diff/10001/content/public/common/con...
content/public/common/content_switches.cc:335: // Batch and synchronize input
event delivery to and response from the renderer.
On 2013/09/13 21:49:27, Brian Anderson wrote:
> Did you forget to delete "response"?

Not really, although it does read rather awkwardly... I was trying to fit it all
in one line, but commas would help: "input event delivery to, and response from,
the renderer."  I'll remove it as I don't think it really adds anything.

[jdduke (slow), 2013-09-25 21:16:57]

PTAL for owners.  Thanks!

tedchoc@: content/browser/android, content/public/android
joi@: content/port/browser, content/public/common
davemoore@: chrome/browser/chromeos/login

[Jói, 2013-09-25 22:03:10]

//content/port and //content/public/common LGTM

[brianderson, 2013-09-25 22:23:13]

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:472: // we have
to make sure calls to ContentViewCoreImpl's SetNeedsBeginFrame
Comment should not refer to SetNeedsBeginFrame anymore.

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:470:
content_view_core_->RemoveBeginFrameSubscriber();
Is it possible that this class gets destroyed after OnSetNeedsFlushInput but
before we get here?  If so, we should probably check for that case and make sure
we balance the calls in the destructor if we need to.

We should probably do the same thing for OnSetNeedsBeginFrame's subscription as
well.

[jdduke (slow), 2013-09-25 23:31:08]

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:472: // we have
to make sure calls to ContentViewCoreImpl's SetNeedsBeginFrame
On 2013/09/25 22:23:14, Brian Anderson wrote:
> Comment should not refer to SetNeedsBeginFrame anymore.

Done.

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/23983036/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:470:
content_view_core_->RemoveBeginFrameSubscriber();
On 2013/09/25 22:23:14, Brian Anderson wrote:
> Is it possible that this class gets destroyed after OnSetNeedsFlushInput but
> before we get here?  If so, we should probably check for that case and make
sure
> we balance the calls in the destructor if we need to.
> 
> We should probably do the same thing for OnSetNeedsBeginFrame's subscription
as
> well.

ContentViewCore will automatically reset any vsync subscribers when this is
destroyed, but you're right that it would be unbalanced otherwise.

[Ted C, 2013-09-25 23:35:21]

lgtm w/ nit

https://codereview.chromium.org/23983036/diff/30001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/23983036/diff/30001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:309:
if (mVSyncProvider != null) {
collapse the two conditionals into one to avoid the inner block (and in the
following method)

[jdduke (slow), 2013-09-26 00:04:10]

piman@: review for chrome/browser/chromeos/login? Thanks.

https://codereview.chromium.org/23983036/diff/30001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/23983036/diff/30001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:309:
if (mVSyncProvider != null) {
On 2013/09/25 23:35:22, Ted C wrote:
> collapse the two conditionals into one to avoid the inner block (and in the
> following method)

Done.

[piman, 2013-09-26 00:29:45]

lgtm

[commit-bot: I haz the power, 2013-09-26 03:50:22]

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

[commit-bot: I haz the power, 2013-09-26 08:07:56]

Change committed as 225387