Hook up DidReceiveInputEvent to the blink scheduler

Hook up DidReceiveInputEvent to the blink scheduler and fixes the bug where
flings did not send DidReceiveInputEvent.  In addition we also pass the input
event type through because we want to ignore keyboard and mouse events.

BUG=391005, 426375, 429814
Committed: https://crrev.com/3722366bff9bf187c151053220968931f3d377c4
Cr-Commit-Position: refs/heads/master@{#303621}

[alex clarke (OOO till 29th), 2014-11-03 21:31:38]

alexclarke@chromium.org changed reviewers:
+ skyostil@google.com

[Sami, 2014-11-03 23:54:06]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2014-11-03 23:54:07]

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_manager.h:48: RendererScheduler*
render_scheduler);
The parameter should probably be called |renderer_scheduler| (here and
elsewhere).

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:622:
client_->DidReceiveInputEvent();
It feels a little wrong to lie to the client about getting input events. How
about we add a new notification called DidAnimateFling, or just DidAnimate?

[rmcilroy, 2014-11-04 00:31:12]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[alex clarke (OOO till 29th), 2014-11-04 01:14:35]

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_manager.h:48: RendererScheduler*
render_scheduler);
On 2014/11/03 23:54:06, Sami wrote:
> The parameter should probably be called |renderer_scheduler| (here and
> elsewhere).

Done.

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/692483002/diff/20001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:622:
client_->DidReceiveInputEvent();
On 2014/11/03 23:54:07, Sami wrote:
> It feels a little wrong to lie to the client about getting input events. How
> about we add a new notification called DidAnimateFling, or just DidAnimate?

Done.

[Sami, 2014-11-04 01:20:14]

Thanks Alex. I think the upload only includes the changes from patch set 1 to 2
instead of the changes needed on top of Ross's patch. You should either squash
the changes into a single commit with git rebase, or git cl upload HEAD~N, where
N is the number of patches you want to upload as a combined patch (probably 2).

[jdduke (slow), 2014-11-04 01:21:02]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2014-11-04 01:21:02]

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:611: 
Would it kill us to put just a single |DidAnimate| call here?

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
File content/renderer/input/input_handler_wrapper.cc (right):

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
content/renderer/input/input_handler_wrapper.cc:65:
renderer_scheduler_->DidReceiveInputEvent();
Nit: No braces necessary (also below).

[Sami, 2014-11-04 01:29:23]

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:611: 
On 2014/11/04 01:21:02, jdduke wrote:
> Would it kill us to put just a single |DidAnimate| call here?

Good idea, it doesn't really matter if we send one too many of these.

[alex clarke (OOO till 29th), 2014-11-04 01:49:06]

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
File content/renderer/input/input_handler_proxy.cc (right):

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:611: 
On 2014/11/04 01:21:02, jdduke wrote:
> Would it kill us to put just a single |DidAnimate| call here?

Done.

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
content/renderer/input/input_handler_proxy.cc:611: 
On 2014/11/04 01:29:23, Sami wrote:
> On 2014/11/04 01:21:02, jdduke wrote:
> > Would it kill us to put just a single |DidAnimate| call here?
> 
> Good idea, it doesn't really matter if we send one too many of these.

Acknowledged.

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
File content/renderer/input/input_handler_wrapper.cc (right):

https://codereview.chromium.org/692483002/diff/40001/content/renderer/input/i...
content/renderer/input/input_handler_wrapper.cc:65:
renderer_scheduler_->DidReceiveInputEvent();
On 2014/11/04 01:21:02, jdduke wrote:
> Nit: No braces necessary (also below).

I slightly prefer this style, because I've seen bugs creep in when people are
not careful otherwise.

[Sami, 2014-11-06 00:09:08]

This patch still looks like it's patching the wrong base version :P

[alex clarke (OOO till 29th), 2014-11-06 00:17:32]

Looks like the diffbase is fixed now.  PTAL

[Sami, 2014-11-06 00:31:21]

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_manager.cc:71: renderer_scheduler));
This should probably be base::Unretained(renderer_scheduler) since the scheduler
isn't refcounted. Not sure how it compiles otherwise.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_manager.h:48: RendererScheduler*
renderer_scheduler);
Could you add a comment mentioning that renderer_scheduler needs to outlive
InputHandlerManager? (and maybe say why we need to pass the scheduler here in
the first place.)

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_wrapper.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_wrapper.cc:64: if (renderer_scheduler_) {
Instead of having renderer_scheduler_ as a member of this class, could we put it
into InputHandlerManager and just call
input_handler_manager_->DidReceiveInputEvent() here? Seems more consistent with
the other functions here.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_wrapper.cc:70: if (renderer_scheduler_) {
The scheduler should never be null, right?

[rmcilroy, 2014-11-06 00:53:17]

https://codereview.chromium.org/692483002/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:2150:
render_thread->blink_platform_impl()->renderer_scheduler());
You need to rebase - renderer_scheduler() is now owned by render_thread, not
blink_platform_impl()

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler.h (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler.h:51: virtual void DidAnimate() =
0;
Is this called for all animations, or only input related animations (e.g.,
fling).  If the latter, how about DidAnimateForInput() ?  Also is this called by
the compositor thread?  If so suffix with "OnCompositorThread" to match
DidReceiveInputEventOnCompositorThread().  Mention which thread it can be called
on in the comment.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:242:
TEST_F(RendererSchedulerImplTest, TestCompositorPolicy_DidAnimate) {
I don't think we need to add _DidAnimate here.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:261:
scheduler_->DidAnimate();
Please keep the DidReceiveInputEvent() compositor mode trigger testing here, and
also add a separate test for DidAnimate (either in the same test function or a
new one).

[alex clarke (OOO till 29th), 2014-11-06 19:21:45]

PTAL

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_manager.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_manager.cc:71: renderer_scheduler));
On 2014/11/06 00:31:20, Sami wrote:
> This should probably be base::Unretained(renderer_scheduler) since the
scheduler
> isn't refcounted. Not sure how it compiles otherwise.

Acknowledged.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_manager.h:48: RendererScheduler*
renderer_scheduler);
On 2014/11/06 00:31:20, Sami wrote:
> Could you add a comment mentioning that renderer_scheduler needs to outlive
> InputHandlerManager? (and maybe say why we need to pass the scheduler here in
> the first place.)

Done.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
File content/renderer/input/input_handler_wrapper.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_wrapper.cc:64: if (renderer_scheduler_) {
On 2014/11/06 00:31:20, Sami wrote:
> Instead of having renderer_scheduler_ as a member of this class, could we put
it
> into InputHandlerManager and just call
> input_handler_manager_->DidReceiveInputEvent() here? Seems more consistent
with
> the other functions here.

Done.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/input/i...
content/renderer/input/input_handler_wrapper.cc:70: if (renderer_scheduler_) {
On 2014/11/06 00:31:20, Sami wrote:
> The scheduler should never be null, right?

Acknowledged.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/render_...
content/renderer/render_view_impl.cc:2150:
render_thread->blink_platform_impl()->renderer_scheduler());
On 2014/11/06 00:53:16, rmcilroy wrote:
> You need to rebase - renderer_scheduler() is now owned by render_thread, not
> blink_platform_impl()

Done.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler.h (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler.h:51: virtual void DidAnimate() =
0;
On 2014/11/06 00:53:16, rmcilroy wrote:
> Is this called for all animations, or only input related animations (e.g.,
> fling).  If the latter, how about DidAnimateForInput() ?  Also is this called
by
> the compositor thread?  If so suffix with "OnCompositorThread" to match
> DidReceiveInputEventOnCompositorThread().  Mention which thread it can be
called
> on in the comment.

Done.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:242:
TEST_F(RendererSchedulerImplTest, TestCompositorPolicy_DidAnimate) {
On 2014/11/06 00:53:16, rmcilroy wrote:
> I don't think we need to add _DidAnimate here.

It kind of is, I didn't intend to delete the original test.

https://codereview.chromium.org/692483002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:261:
scheduler_->DidAnimate();
On 2014/11/06 00:53:17, rmcilroy wrote:
> Please keep the DidReceiveInputEvent() compositor mode trigger testing here,
and
> also add a separate test for DidAnimate (either in the same test function or a
> new one).

Done.

[rmcilroy, 2014-11-06 19:45:13]

lgtm (focusing on the RendererScheduler changes)

https://codereview.chromium.org/692483002/diff/140001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/692483002/diff/140001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl.cc:106:
DidReceiveInputEventOnCompositorThread();
Maybe pull out the code in DidReceiveInputEventOnCompositorThread to a separate
function (e.g., UpdateForInputEvent() and have both public methods call this.

[alex clarke (OOO till 29th), 2014-11-06 21:19:06]

https://codereview.chromium.org/692483002/diff/140001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/692483002/diff/140001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl.cc:106:
DidReceiveInputEventOnCompositorThread();
On 2014/11/06 19:45:12, rmcilroy wrote:
> Maybe pull out the code in DidReceiveInputEventOnCompositorThread to a
separate
> function (e.g., UpdateForInputEvent() and have both public methods call this.

Done.

[alex clarke (OOO till 29th), 2014-11-06 21:22:17]

alexclarke@chromium.org changed reviewers:
+ sievers@chromium.org

[alex clarke (OOO till 29th), 2014-11-06 21:24:30]

+Daniel, can you please take a look.

[Sami, 2014-11-06 22:17:34]

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:40: // events and compositor
animations occur, which is why it's passed in here.
nit: s/compositor/fling/ would be a little more accurate.

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
File content/renderer/input/input_handler_wrapper.cc (right):

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
content/renderer/input/input_handler_wrapper.cc:61: if (input_handler_manager_)
{
Do we need this check since the other methods don't have it?

[alex clarke (OOO till 29th), 2014-11-06 22:32:15]

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:40: // events and compositor
animations occur, which is why it's passed in here.
On 2014/11/06 22:17:34, Sami wrote:
> nit: s/compositor/fling/ would be a little more accurate.

Done.

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
File content/renderer/input/input_handler_wrapper.cc (right):

https://codereview.chromium.org/692483002/diff/160001/content/renderer/input/...
content/renderer/input/input_handler_wrapper.cc:61: if (input_handler_manager_)
{
On 2014/11/06 22:17:34, Sami wrote:
> Do we need this check since the other methods don't have it?

Done.

[Sami, 2014-11-06 22:40:59]

Thank you, lgtm! jdduke and sievers, please review for realz.

[jdduke (slow), 2014-11-07 00:06:16]

rubberstamp lgtm for content/renderer/input.

Could you update the title/description to indicate what this is being hooked up
to (i.e., the RendererScheduler). Thanks.

[jdduke (slow), 2014-11-07 02:54:54]

https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:68: void DidReceiveInputEvent();
If we plumb the event type through here
(DidReceiveInputEvent(blink::WebInputEvent::Type)), we can let the scheduler
make the right decision about how to respond. Per discussion in
crbug.com/429814, I'd prefer we start by restricting prioritization when we see
touch/gesture/wheel events, but not mouse/keyboard. We can always expand
coverage later.

[alex clarke (OOO till 29th), 2014-11-07 16:59:11]

https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
File content/renderer/input/input_handler_manager.h (right):

https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
content/renderer/input/input_handler_manager.h:68: void DidReceiveInputEvent();
On 2014/11/07 02:54:54, jdduke wrote:
> If we plumb the event type through here
> (DidReceiveInputEvent(blink::WebInputEvent::Type)), we can let the scheduler
> make the right decision about how to respond. Per discussion in
> crbug.com/429814, I'd prefer we start by restricting prioritization when we
see
> touch/gesture/wheel events, but not mouse/keyboard. We can always expand
> coverage later.

This seems like a good idea, but it feels like it might be cleaner if done in a
separate patch. WDYT?

[jdduke (slow), 2014-11-07 17:12:50]

On 2014/11/07 16:59:11, alexclarke1 wrote:
>
https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
> File content/renderer/input/input_handler_manager.h (right):
> 
>
https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
> content/renderer/input/input_handler_manager.h:68: void
DidReceiveInputEvent();
> On 2014/11/07 02:54:54, jdduke wrote:
> > If we plumb the event type through here
> > (DidReceiveInputEvent(blink::WebInputEvent::Type)), we can let the scheduler
> > make the right decision about how to respond. Per discussion in
> > crbug.com/429814, I'd prefer we start by restricting prioritization when we
> see
> > touch/gesture/wheel events, but not mouse/keyboard. We can always expand
> > coverage later.
> 
> This seems like a good idea, but it feels like it might be cleaner if done in
a
> separate patch. WDYT?

Hmm, this patch isn't very big, so I'd just as soon do it now. Unless we're a
long way from turning this on?

[alex clarke (OOO till 29th), 2014-11-07 18:08:21]

On 2014/11/07 17:12:50, jdduke wrote:
> On 2014/11/07 16:59:11, alexclarke1 wrote:
> >
>
https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
> > File content/renderer/input/input_handler_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/692483002/diff/180001/content/renderer/input/...
> > content/renderer/input/input_handler_manager.h:68: void
> DidReceiveInputEvent();
> > On 2014/11/07 02:54:54, jdduke wrote:
> > > If we plumb the event type through here
> > > (DidReceiveInputEvent(blink::WebInputEvent::Type)), we can let the
scheduler
> > > make the right decision about how to respond. Per discussion in
> > > crbug.com/429814, I'd prefer we start by restricting prioritization when
we
> > see
> > > touch/gesture/wheel events, but not mouse/keyboard. We can always expand
> > > coverage later.
> > 
> > This seems like a good idea, but it feels like it might be cleaner if done
in
> a
> > separate patch. WDYT?
> 
> Hmm, this patch isn't very big, so I'd just as soon do it now. Unless we're a
> long way from turning this on?

OK I routed the type through.  PTAL

[jdduke (slow), 2014-11-07 18:12:38]

Thanks, still lgtm but I'll defer to skyostil/rmcilroy as to whether we filter
out mouse/keyboard events now or later.

https://codereview.chromium.org/692483002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/692483002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl.cc:95: // TODO(rmcilroy):
Decide whether only a subset of input events should trigger
Can we decide this now?

if (WebInputEvent::isMouseEventType(type) ||
    WebInputEvent::isKeyboardEventType(type)) {
  return;
}

??

[alex clarke (OOO till 29th), 2014-11-07 18:31:55]

https://codereview.chromium.org/692483002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/692483002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl.cc:95: // TODO(rmcilroy):
Decide whether only a subset of input events should trigger
On 2014/11/07 18:12:38, jdduke wrote:
> Can we decide this now?
> 
> if (WebInputEvent::isMouseEventType(type) ||
>     WebInputEvent::isKeyboardEventType(type)) {
>   return;
> }
> 
> ??

Done.

[Sami, 2014-11-10 11:48:41]

Cool, lgtm!

[alex clarke (OOO till 29th), 2014-11-10 11:50:13]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2014-11-10 11:50:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/692483002/220001

[commit-bot: I haz the power, 2014-11-10 11:54:53]

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

[commit-bot: I haz the power, 2014-11-10 11:54:55]

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

[no sievers, 2014-11-10 19:10:43]

lgtm

[alex clarke (OOO till 29th), 2014-11-11 09:04:01]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2014-11-11 09:04:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/692483002/220001

[commit-bot: I haz the power, 2014-11-11 09:52:32]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-11-11 09:53:18]

Patchset 12 (id:??) landed as
https://crrev.com/3722366bff9bf187c151053220968931f3d377c4
Cr-Commit-Position: refs/heads/master@{#303621}