Add timestamps to synthesized events.

Add timestamps to synthesized events.

With this CL, synthetic gestures add timestamps to the events they generate.
This allows us to have timestamps that are not necessarily aligned to frame
boundaries. The timestamps are converted to native timestamps by the synthetic
gesture targets.

The synthetic gesture controller passes timestamps instead of intervals to the
gestures.

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

[Dominik Grewe, 2014-01-03 17:34:05]

This is my first attempt for adding timestamps to synthesized events. PTAL.

The controller now passes a timestamp to the gesture rather than an interval.

On Android, I can't pass the timestamps directly, because MotionEvent requires
values from uptimeMillis(). To get around that I base all the timestamps on the
value of uptimeMillis when starting a gesture.

On Aura, I'm not sure how to pass timestamps to the ui::Events. The touch events
created via MakeUITouchEventsFromWebTouchEvents automatically pass them on, but
the others don't afaict. There's a ui::Event::set_time_stamp() but that's
protected. Might ask Sadrul for help with this.

[jdduke (slow), 2014-01-03 18:24:30]

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture.cc (right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture.cc:49: return (timestamp -
base::TimeTicks::UnixEpoch()).InSecondsF();
The documentation for WebInputEvent is somewhat misleading, we really want the
*system* time, not the *wall clock* time.  So this should read:

return (timestamp - base::TimeTicks()).InSecondsF();

Then you shouldn't need to convert between wall and system times later on.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:40:
JNIEnv* env, Action action, int pointer_count, double timestamp_in_s) {
As noted later, let's make this arg |long time_in_ms|.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:144: float
total_abs_delta = params_.relative_pointer_speed_in_pixels_s *
DCHECK(timestamp);

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:152: int
total_duration_in_ms = (1000.0f * ComputeAbsoluteDistance()) /
Nit: This may be slightly more readable as: 

1000.f * (ComputeAbsoluteDistance() /
params_.relative_pointer_speed_in_pixels_s);

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:172: return
start_y_0_ + delta == target_y_0_;
Is it possible we'd run into floating point precision issues here? Is this worth
an epsilon check?

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc
(right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:211:
base::TimeTicks* timestamp) const {
DCHECK(timestamp);

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:219:
(1000 * params_.distance.Length()) / params_.speed_in_pixels_s;
Same as above, multiplying the time scale by the distance first is a bit less
intuitive, so let's divide distance by speed and then do the multiply.

https://codereview.chromium.org/119323007/diff/30001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java
(right):

https://codereview.chromium.org/119323007/diff/30001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java:31:
private double mDownTimestampInS;
We shouldn't need two times here.  The provided timestamp to |inject| should be
in system time, so you should only need to store |mDownTimeInMs| as before.

https://codereview.chromium.org/119323007/diff/30001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java:59:
long timeInMs;
Instead of taking a double |timeStampInS|, just have as an argument |long time|,
or |long timeMs|, let the caller do the conversion, and you shouldn't have to
change any of the code you had before (except maybe rename the var).

[Dominik Grewe, 2014-01-06 11:46:06]

Thanks Jared.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture.cc (right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture.cc:49: return (timestamp -
base::TimeTicks::UnixEpoch()).InSecondsF();
On 2014/01/03 18:24:30, jdduke wrote:
> The documentation for WebInputEvent is somewhat misleading, we really want the
> *system* time, not the *wall clock* time.  So this should read:
> 
> return (timestamp - base::TimeTicks()).InSecondsF();
> 
> Then you shouldn't need to convert between wall and system times later on.

Done.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_gesture_target_android.cc
(right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_target_android.cc:40:
JNIEnv* env, Action action, int pointer_count, double timestamp_in_s) {
On 2014/01/03 18:24:30, jdduke wrote:
> As noted later, let's make this arg |long time_in_ms|.

Done.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:144: float
total_abs_delta = params_.relative_pointer_speed_in_pixels_s *
On 2014/01/03 18:24:30, jdduke wrote:
> DCHECK(timestamp);

Done.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:152: int
total_duration_in_ms = (1000.0f * ComputeAbsoluteDistance()) /
On 2014/01/03 18:24:30, jdduke wrote:
> Nit: This may be slightly more readable as: 
> 
> 1000.f * (ComputeAbsoluteDistance() /
> params_.relative_pointer_speed_in_pixels_s);

Done.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:172: return
start_y_0_ + delta == target_y_0_;
On 2014/01/03 18:24:30, jdduke wrote:
> Is it possible we'd run into floating point precision issues here? Is this
worth
> an epsilon check?

I don't think that this could cause problems. In the final step delta is set to
|(+/-)params.total_num_pixels_covered / 2| (I got rid of
ComputeAbsoluteDistance), where |params.total_num_pixels_covered| is an integer,
which is exactly the difference between |start_y_0_| and |target_y_0_| (see
SetupCoordinates).

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc
(right):

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:211:
base::TimeTicks* timestamp) const {
On 2014/01/03 18:24:30, jdduke wrote:
> DCHECK(timestamp);

Done.

https://codereview.chromium.org/119323007/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:219:
(1000 * params_.distance.Length()) / params_.speed_in_pixels_s;
On 2014/01/03 18:24:30, jdduke wrote:
> Same as above, multiplying the time scale by the distance first is a bit less
> intuitive, so let's divide distance by speed and then do the multiply.

Done.

https://codereview.chromium.org/119323007/diff/30001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java
(right):

https://codereview.chromium.org/119323007/diff/30001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java:31:
private double mDownTimestampInS;
On 2014/01/03 18:24:30, jdduke wrote:
> We shouldn't need two times here.  The provided timestamp to |inject| should
be
> in system time, so you should only need to store |mDownTimeInMs| as before.

Done.

https://codereview.chromium.org/119323007/diff/30001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/TouchEventSynthesizer.java:59:
long timeInMs;
On 2014/01/03 18:24:30, jdduke wrote:
> Instead of taking a double |timeStampInS|, just have as an argument |long
time|,
> or |long timeMs|, let the caller do the conversion, and you shouldn't have to
> change any of the code you had before (except maybe rename the var).

Done.

[Dominik Grewe, 2014-01-06 18:46:50]

I've updated smooth scroll as we discussed. Please have a look.
I think it's cleaner than before, even with the special case for the last delta.
Wdyt?

[jdduke (slow), 2014-01-06 18:56:27]

lgtm with nits.  We can do the same pinch refactor either in this or a
subsequent patch, up to you.

https://codereview.chromium.org/119323007/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc
(right):

https://codereview.chromium.org/119323007/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:214: if
(timestamp == stop_scrolling_time_)
Nit: Maybe put a note here about precision issues?

https://codereview.chromium.org/119323007/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:237:
return timestamp > stop_scrolling_time_ ? stop_scrolling_time_ : timestamp;
Nit: std::min(timestamp, stop_scrolling_time_);

[Dominik Grewe, 2014-01-06 19:16:43]

I've updated the pinch code. PTAL.

https://codereview.chromium.org/119323007/diff/140001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc
(right):

https://codereview.chromium.org/119323007/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:214: if
(timestamp == stop_scrolling_time_)
On 2014/01/06 18:56:28, jdduke wrote:
> Nit: Maybe put a note here about precision issues? 

Done.

https://codereview.chromium.org/119323007/diff/140001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:237:
return timestamp > stop_scrolling_time_ ? stop_scrolling_time_ : timestamp;
On 2014/01/06 18:56:28, jdduke wrote:
> Nit: std::min(timestamp, stop_scrolling_time_);

Done.

[jdduke (slow), 2014-01-06 21:41:14]

lgtm with nits.  Thanks!

https://codereview.chromium.org/119323007/diff/230001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_pinch_gesture.cc (right):

https://codereview.chromium.org/119323007/diff/230001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:155: if
(timestamp == stop_time_)
if (HasReachedTarget(distance))

https://codereview.chromium.org/119323007/diff/230001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_pinch_gesture.cc:172: return
timestamp == stop_time_;
Same as with scroll, let's generalize this by:

return timestamp >= stop_time_;

https://codereview.chromium.org/119323007/diff/230001/content/browser/rendere...
File content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc
(right):

https://codereview.chromium.org/119323007/diff/230001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:216: if
(timestamp == stop_scrolling_time_)
Let's re-use the check below:

HasScrolledEntireDistance(timestamp)

https://codereview.chromium.org/119323007/diff/230001/content/browser/rendere...
content/browser/renderer_host/input/synthetic_smooth_scroll_gesture.cc:244:
return timestamp == stop_scrolling_time_;
Let's make this general with

return timestamp >= stop_scrolling_time_;

[Dominik Grewe, 2014-01-07 10:43:24]

Thanks Jared!

I just fixed another small issue: The computation of |total_duration_in_us| for
pinch involved dividing two integers (|total_num_pixels_covered| and
|relative_pointer_speed_in_pixels_s|). One of them should be cast to double
first to get an accurate result, otherwise you might get a duration of 0 for
example, if the speed is greater than the distance. I've added a DCHECK to make
sure the duration is greater than 0.
This problem wasn't raised by the unit tests, because we always make sure that
the final step will get us to the correct final position.

[commit-bot: I haz the power, 2014-01-07 13:19:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/119323007/350001

[commit-bot: I haz the power, 2014-01-07 15:29:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominikg@chromium.org/119323007/350001

[commit-bot: I haz the power, 2014-01-07 15:50:26]

Change committed as 243299