input: Change how synthesized events are dispatched in telemetry tests.

input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=722921
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2856423002
Cr-Commit-Position: refs/heads/master@{#472595}
Committed: https://chromium.googlesource.com/chromium/src/+/233e1b3593cd4a9565e2ffef1a9ec97207d740e2

[sadrul, 2017-05-04 04:40:00]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 04:40:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-05-04 04:40:44]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 04:41:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 04:51:04]

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

[commit-bot: I haz the power, 2017-05-04 04:51:04]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[sadrul, 2017-05-05 05:44:12]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 05:44:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 05:58:16]

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

[commit-bot: I haz the power, 2017-05-05 05:58:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sadrul, 2017-05-15 20:58:50]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 20:59:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-05-15 21:14:52]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 21:15:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 22:07:57]

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

[commit-bot: I haz the power, 2017-05-15 22:07:58]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sadrul, 2017-05-16 03:23:47]

Description was changed from

==========
wip: Change how synthesized events are dispatched.

Synthesized events are still generated and dispatched on begin-frame.

BUG=...
==========

to

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Notable changes:

. SyntheticGestureController would request for the next input flush
  through the SyntheticGestureTarget, which would then request for flush
  to RenderWidgetHostImpl. Change this so SyntheticGestureController can
  directly request for flush to RenderWidgetHostImpl, through a callbac
  installed during construction of SyntheticGestureController.

BUG=...
==========

[sadrul, 2017-05-16 03:26:48]

Description was changed from

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Notable changes:

. SyntheticGestureController would request for the next input flush
  through the SyntheticGestureTarget, which would then request for flush
  to RenderWidgetHostImpl. Change this so SyntheticGestureController can
  directly request for flush to RenderWidgetHostImpl, through a callbac
  installed during construction of SyntheticGestureController.

BUG=...
==========

to

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=...
==========

[sadrul, 2017-05-16 03:45:26]

sadrul@chromium.org changed reviewers:
+ dtapuska@chromium.org

[sadrul, 2017-05-16 03:45:27]

Hi! Mind taking a look at this? This CL is not expected to change the behaviour,
just rearrange some code, so that it makes it easier to dispatch events at an
offset in subsequent CLs. Thanks!

[dtapuska, 2017-05-16 14:39:42]

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_controller.cc (left):

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_controller.cc:42: if
(pending_gesture_result_)
Doesn't removing the pending_gesture_result_ not change the behavior of this
method? Should pending_gesture_result_ be checked in OnBeginFrame() ?

I'm wondering about the case where a gesture has been sent.

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_controller.cc:45:
SyntheticGesture* gesture = pending_gesture_queue_.FrontGesture();
Do we need this variable on the stack? I feel someone might want to use it after
its storage is removed. Looks to only be used in one spot so we should be able
to just get rid of the variable reference.

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:192: virtual void
OnSetNeedsFlushInput() {}
Would it not make sense to make this pure virtual?

[dtapuska, 2017-05-16 14:40:52]

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_controller.cc (left):

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_controller.cc:42: if
(pending_gesture_result_)
On 2017/05/16 14:39:41, dtapuska wrote:
> Doesn't removing the pending_gesture_result_ not change the behavior of this
> method? Should pending_gesture_result_ be checked in OnBeginFrame() ?
> 
> I'm wondering about the case where a gesture has been sent.

Ugh; no this submitted the wrong comment.  I looked over it more; disregard that
comment by my new one was: Can pending_gesture_result_ be removed?

[sadrul, 2017-05-16 17:27:42]

Description was changed from

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=...
==========

to

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[sadrul, 2017-05-16 17:27:53]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 17:28:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-05-16 17:35:06]

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_gesture_controller.cc (left):

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_controller.cc:42: if
(pending_gesture_result_)
On 2017/05/16 14:40:52, dtapuska wrote:
> On 2017/05/16 14:39:41, dtapuska wrote:
> > Doesn't removing the pending_gesture_result_ not change the behavior of this
> > method? Should pending_gesture_result_ be checked in OnBeginFrame() ?
> > 
> > I'm wondering about the case where a gesture has been sent.
> 
> Ugh; no this submitted the wrong comment.  I looked over it more; disregard
that
> comment by my new one was: Can pending_gesture_result_ be removed?

Yep, done.

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_gesture_controller.cc:45:
SyntheticGesture* gesture = pending_gesture_queue_.FrontGesture();
On 2017/05/16 14:39:41, dtapuska wrote:
> Do we need this variable on the stack? I feel someone might want to use it
after
> its storage is removed. Looks to only be used in one spot so we should be able
> to just get rid of the variable reference.

Done.

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2856423002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_base.h:192: virtual void
OnSetNeedsFlushInput() {}
On 2017/05/16 14:39:41, dtapuska wrote:
> Would it not make sense to make this pure virtual?

I initially did that, but added back the default impl because of
RWHVChildFrame/RWHVGuest. But making this pure virtual makes it easier to follow
the code I think. Done.

[dtapuska, 2017-05-16 17:42:31]

dtapuska@chromium.org changed reviewers:
+ kenrb@chromium.org

[dtapuska, 2017-05-16 17:42:32]

lgtm % I wonder if this is now broken for oopif sending web driver events to
them. Previously these caused issues because they didn't create the right
subclass of the gesture target. Anyways adding Ken as he can comment.

[sadrul, 2017-05-16 17:53:41]

Description was changed from

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=722921
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[sadrul, 2017-05-16 17:56:13]

On 2017/05/16 17:42:32, dtapuska wrote:
> lgtm % I wonder if this is now broken for oopif sending web driver events to
> them. Previously these caused issues because they didn't create the right
> subclass of the gesture target. Anyways adding Ken as he can comment.

Do you know if there are tests that currently do this?

[dtapuska, 2017-05-16 17:58:37]

On 2017/05/16 17:56:13, sadrul wrote:
> On 2017/05/16 17:42:32, dtapuska wrote:
> > lgtm % I wonder if this is now broken for oopif sending web driver events to
> > them. Previously these caused issues because they didn't create the right
> > subclass of the gesture target. Anyways adding Ken as he can comment.
> 
> Do you know if there are tests that currently do this?

This code path isn't just for tests. It is for webdriver/chromedriver as well.

See issue: https://bugs.chromium.org/p/chromium/issues/detail?id=720020 that I
reported about OOPIF and touch. I just wonder if with this change more is
broken. I don't know.. I'm not really certain how OOPIF works with this stuff.

[commit-bot: I haz the power, 2017-05-16 20:10:36]

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

[commit-bot: I haz the power, 2017-05-16 20:10:37]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sadrul, 2017-05-17 04:16:07]

On 2017/05/16 17:58:37, dtapuska wrote:
> On 2017/05/16 17:56:13, sadrul wrote:
> > On 2017/05/16 17:42:32, dtapuska wrote:
> > > lgtm % I wonder if this is now broken for oopif sending web driver events
to
> > > them. Previously these caused issues because they didn't create the right
> > > subclass of the gesture target. Anyways adding Ken as he can comment.
> > 
> > Do you know if there are tests that currently do this?
> 
> This code path isn't just for tests. It is for webdriver/chromedriver as well.
> 
> See issue: https://bugs.chromium.org/p/chromium/issues/detail?id=720020 that I
> reported about OOPIF and touch. I just wonder if with this change more is
> broken. I don't know.. I'm not really certain how OOPIF works with this stuff.

Interesting! Thanks for the info. I'll wait for Ken's feedback.

[sadrul, 2017-05-17 18:06:33]

sadrul@chromium.org changed reviewers:
+ piman@chromium.org

[sadrul, 2017-05-17 18:06:33]

kenrb@ ping

+piman@ for content owners

[piman, 2017-05-17 19:01:07]

rs lgtm

[kenrb, 2017-05-17 20:10:37]

lgtm

AFAICT this doesn't break OOPIFs with Chromedriver any more than they are
already broken.

[sadrul, 2017-05-17 20:44:25]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2017-05-17 20:45:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 22:56:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495053865255370,
"parent_rev": "5781ee5811becfe6c87bff86a1e0b507cb5f3b97", "commit_rev":
"233e1b3593cd4a9565e2ffef1a9ec97207d740e2"}

[commit-bot: I haz the power, 2017-05-17 22:56:42]

Description was changed from

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=722921
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
input: Change how synthesized events are dispatched in telemetry tests.

Synthesized events are still generated and dispatched on begin-frame. But
the dispatch is somewhat simplified in this change. Some detail:

. Before this change, SyntheticGestureController would request for the
  next input flush through the SyntheticGestureTarget, which would then
  request for flush to RenderWidgetHostImpl. This CL changes that, so that
  SyntheticGestureController can directly request for flush to
  RenderWidgetHostImpl, through a callback installed during construction
  of SyntheticGestureController.

BUG=722921
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2856423002
Cr-Commit-Position: refs/heads/master@{#472595}
Committed:
https://chromium.googlesource.com/chromium/src/+/233e1b3593cd4a9565e2ffef1a9e...
==========

[commit-bot: I haz the power, 2017-05-17 22:56:43]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/233e1b3593cd4a9565e2ffef1a9e...