VR: Use more accurate timestamps for input events.

VR: Use more accurate timestamps for input events.

This change pulls timestamps from the controller data, asking it when the
values we poll were last updated.

BUG=732830
Review-Url: https://codereview.chromium.org/2941293002
Cr-Commit-Position: refs/heads/master@{#480125}
Committed: https://chromium.googlesource.com/chromium/src/+/33648c554cafddb23cf3140a1b915ebd3f50b379

[mthiesse, 2017-06-16 16:01:48]

mthiesse@chromium.org changed reviewers:
+ asimjour@chromium.org, cjgrant@chromium.org

[mthiesse, 2017-06-16 16:01:49]

PTAL

[cjgrant, 2017-06-16 16:19:01]

lgtm with a few comments.

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller.cc (right):

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller.cc:51: constexpr float
microsPerNano = 1000.0;
Why use a float?  The types used with this are integer.

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:831: timestamp =
controller_->GetLastOrientationTimestamp();
Is it possible this could pass non-increasing timestamps to blink?  Eg, we pass
a move then a down, but the down is older than the move?  Will blink choke on
that?

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:842: mouse_event->click_count =
1;
Carried over from previous code, but this line is weird.  Do we need to set
click_count always?  I only dug as far as the arg not being documented in the
blink header.

[mthiesse, 2017-06-16 17:24:11]

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_controller.cc (right):

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_controller.cc:51: constexpr float
microsPerNano = 1000.0;
On 2017/06/16 16:19:00, cjgrant wrote:
> Why use a float?  The types used with this are integer.

Done.

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:831: timestamp =
controller_->GetLastOrientationTimestamp();
On 2017/06/16 16:19:00, cjgrant wrote:
> Is it possible this could pass non-increasing timestamps to blink?  Eg, we
pass
> a move then a down, but the down is older than the move?  Will blink choke on
> that?

I tested it for a while and nothing blew up. I think for all blink knows these
could be events from different devices with different arbitrary delays.

https://codereview.chromium.org/2941293002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:842: mouse_event->click_count =
1;
On 2017/06/16 16:19:00, cjgrant wrote:
> Carried over from previous code, but this line is weird.  Do we need to set
> click_count always?  I only dug as far as the arg not being documented in the
> blink header.

It's used to handle double and triple-click behaviour. There's no harm in always
having it set to 1, but we might consider supporting double-click in the future.

[mthiesse, 2017-06-16 17:25:38]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-06-16 17:25:39]

The patchset sent to the CQ was uploaded after l-g-t-m from cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2941293002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2017-06-16 17:25:59]

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-06-16 19:26:48]

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

[commit-bot: I haz the power, 2017-06-16 19:26:49]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[mthiesse, 2017-06-16 19:33:31]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-06-16 19:33:43]

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-06-16 19:39:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497641611682690,
"parent_rev": "b0d04772edfcaf49e8bfc909f8ed1596e8d46184", "commit_rev":
"33648c554cafddb23cf3140a1b915ebd3f50b379"}

[commit-bot: I haz the power, 2017-06-16 19:39:52]

Description was changed from

==========
VR: Use more accurate timestamps for input events.

This change pulls timestamps from the controller data, asking it when the
values we poll were last updated.

BUG=732830
==========

to

==========
VR: Use more accurate timestamps for input events.

This change pulls timestamps from the controller data, asking it when the
values we poll were last updated.

BUG=732830

Review-Url: https://codereview.chromium.org/2941293002
Cr-Commit-Position: refs/heads/master@{#480125}
Committed:
https://chromium.googlesource.com/chromium/src/+/33648c554cafddb23cf3140a1b91...
==========

[commit-bot: I haz the power, 2017-06-16 19:39:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/33648c554cafddb23cf3140a1b91...