Set the unique_event_id when converting from TouchEvent to WebTouchEvent

Set the unique_event_id when converting from TouchEvent to WebTouchEvent

In order for us to map and track between the touch events and their ACKs,
we need to set unique_event_id to WebTouchEvent when converting from TouchEvent.

This patch is part of a series:

patch #1: https://codereview.chromium.org/1008573002/
patch #2: This CL

BUG=466711
Committed: https://crrev.com/2574b1b13231fc8348389716ac5630d9f853a1a5
Cr-Commit-Position: refs/heads/master@{#327520}

[lanwei, 2015-03-13 00:38:22]

lanwei@chromium.org changed reviewers:
+ jdduke@chromium.org, tdresser@chromium.org

[tdresser, 2015-03-13 13:58:09]

It would probably make sense to set the unique_event_id in the ack params in
this patch as well.

[tdresser, 2015-03-13 15:22:00]

On 2015/03/13 13:58:09, tdresser wrote:
> It would probably make sense to set the unique_event_id in the ack params in
> this patch as well.

Lan pointed out that the ack params are assigned on the Chrome side.

We might as well bundle this in with the other blink change though.

[jdduke (slow), 2015-03-13 15:32:00]

https://codereview.chromium.org/999423003/diff/1/content/common/input_messages.h
File content/common/input_messages.h (right):

https://codereview.chromium.org/999423003/diff/1/content/common/input_message...
content/common/input_messages.h:114: // TODO(jdduke): Use Optional<T> type to
avoid heap alloc, crbug.com/375002.
So, for some time I've been meaning to make the ack a proper structure, with
different fields for different event types.

i.e., overscroll would be a field for scrollupdate/wheel, id would be a field
for touch events, etc... similar to how WebGestureEvent behaves. Not sure if it
makes sense to do it now, but it would ease the pain of adding event
type-specific ack metadata in the future.

[lanwei, 2015-03-13 21:12:17]

Patchset #2 (id:20001) has been deleted

[lanwei, 2015-03-13 21:44:24]

Patchset #2 (id:40001) has been deleted

[jdduke (slow), 2015-03-13 21:48:27]

https://codereview.chromium.org/999423003/diff/60001/content/common/input_mes...
File content/common/input_messages.h (right):

https://codereview.chromium.org/999423003/diff/60001/content/common/input_mes...
content/common/input_messages.h:116: IPC_STRUCT_MEMBER(uint32,
unique_touch_event_id)
I wonder how silly it would be simply pass back the original WebInputEvent...
that would give us the type and the id and all that fun data...

https://codereview.chromium.org/999423003/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/motion_event.h:50: virtual uint32 GetUniqueId()
const = 0;
I don't think we want both an id and a unique id...

[tdresser, 2015-03-16 12:24:13]

https://codereview.chromium.org/999423003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/999423003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:353:
unique_touch_event_id= touch.uniqueTouchEventId;
add space before =

https://codereview.chromium.org/999423003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:382:
unique_touch_event_id= touch.uniqueTouchEventId;
add space before =

https://codereview.chromium.org/999423003/diff/60001/content/common/input_mes...
File content/common/input_messages.h (right):

https://codereview.chromium.org/999423003/diff/60001/content/common/input_mes...
content/common/input_messages.h:116: IPC_STRUCT_MEMBER(uint32,
unique_touch_event_id)
On 2015/03/13 21:48:27, jdduke wrote:
> I wonder how silly it would be simply pass back the original WebInputEvent...
> that would give us the type and the id and all that fun data...

I've been under the impression that we want to keep this struct _real_ small, to
reduce IPC overhead. Do you think that's not worthwhile?

What advantages would there be to passing the whole struct?
Getting the cancelable bit and the type would allow for cleaner code in the
TouchEventQueue I suppose.

https://codereview.chromium.org/999423003/diff/60001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/999423003/diff/60001/ui/events/event.cc#newco...
ui/events/event.cc:117: return id++;
We should check if id has wrapped around, and set it back to 1 if it has.

We should also add a check somewhere (possibly on the renderer side) to ensure
that touch events never have a unique id of 0.

If we are using uint32, we should change this method to return a uint32.

https://codereview.chromium.org/999423003/diff/60001/ui/events/gesture_detect...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/60001/ui/events/gesture_detect...
ui/events/gesture_detection/motion_event.h:50: virtual uint32 GetUniqueId()
const = 0;
On 2015/03/13 21:48:27, jdduke wrote:
> I don't think we want both an id and a unique id...

We require the id to be stable across multiple events (it's the pointer id of
the first pointer), whereas the unique id is separate per event. We should
probably change the naming here though.

[lanwei, 2015-03-16 19:06:07]

https://codereview.chromium.org/999423003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/999423003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:353:
unique_touch_event_id= touch.uniqueTouchEventId;
On 2015/03/16 12:24:12, tdresser wrote:
> add space before =

Done.

https://codereview.chromium.org/999423003/diff/60001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:382:
unique_touch_event_id= touch.uniqueTouchEventId;
On 2015/03/16 12:24:12, tdresser wrote:
> add space before =

Done.

https://codereview.chromium.org/999423003/diff/60001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/999423003/diff/60001/ui/events/event.cc#newco...
ui/events/event.cc:117: return id++;
On 2015/03/16 12:24:12, tdresser wrote:
> We should check if id has wrapped around, and set it back to 1 if it has.
> 
> We should also add a check somewhere (possibly on the renderer side) to ensure
> that touch events never have a unique id of 0.
> 
> If we are using uint32, we should change this method to return a uint32.

Done.

[lanwei, 2015-03-16 20:41:41]

Patchset #3 (id:80001) has been deleted

[jdduke (slow), 2015-03-16 21:33:45]

Sorry, I'm not intentionally delaying review here. It's just that this is a good
amount of additional wiring, introducing a stateful event stream change that we
want to be really sure about before we land it.

https://codereview.chromium.org/999423003/diff/100001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/999423003/diff/100001/ui/events/event.h#newco...
ui/events/event.h:523: uint64 unique_touch_event_id() const { return
unique_touch_event_id_; }
Why the extra "touch" name here? Isn't that implicit for the ui::TouchEvent.

[lanwei, 2015-04-07 17:41:16]

Patchset #4 (id:120001) has been deleted

[lanwei, 2015-04-07 17:54:24]

Patchset #4 (id:140001) has been deleted

[lanwei, 2015-04-07 18:06:36]

Patchset #4 (id:160001) has been deleted

[lanwei, 2015-04-07 21:41:08]

Patchset #4 (id:180001) has been deleted

[lanwei, 2015-04-07 21:52:58]

Patchset #4 (id:200001) has been deleted

[lanwei, 2015-04-07 22:11:20]

Patchset #4 (id:220001) has been deleted

[lanwei, 2015-04-07 22:29:43]

Patchset #4 (id:240001) has been deleted

[tdresser, 2015-04-08 12:24:26]

Sorry for requesting that rename so late.

It doesn't feel like this has adequate test coverage - are there some additional
tests you can add?

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:256:
LOG(ERROR) << "   PressTouchPoint   " << touch_event_.uniqueTouchEventId;
These logs should be removed.

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:866:
LOG(ERROR) << "   InputRouterImplTest.AckedTouchEventState   ";
Ditto.

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:124: DCHECK_NE(id,
0UL);
We can remove that DCHECK.

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:77: return 0;
Will this cause problems for devtools touch emulation?

https://codereview.chromium.org/999423003/diff/280001/content/common/input/sy...
File content/common/input/synthetic_web_input_event_builders.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/common/input/sy...
content/common/input/synthetic_web_input_event_builders.cc:15: uint64
get_next_event_id() {
Can we factor this method out into a helper somewhere? Perhaps something in the
events_base target would work?

https://codereview.chromium.org/999423003/diff/280001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/280001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:50: virtual uint64 GetUniqueId()
const = 0;
I would prefer we call this the GetUniqueEventId, and clarify in the comment how
its different from GetId().

[lanwei, 2015-04-08 20:48:56]

Patchset #6 (id:300001) has been deleted

[lanwei, 2015-04-09 04:14:29]

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:256:
LOG(ERROR) << "   PressTouchPoint   " << touch_event_.uniqueTouchEventId;
On 2015/04/08 12:24:26, tdresser wrote:
> These logs should be removed.

Done.

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl_unittest.cc:866:
LOG(ERROR) << "   InputRouterImplTest.AckedTouchEventState   ";
On 2015/04/08 12:24:26, tdresser wrote:
> Ditto.

Done.

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.cc:124: DCHECK_NE(id,
0UL);
On 2015/04/08 12:24:26, tdresser wrote:
> We can remove that DCHECK.

Done.

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:77: return 0;
On 2015/04/08 12:24:26, tdresser wrote:
> Will this cause problems for devtools touch emulation?

Done.

https://codereview.chromium.org/999423003/diff/280001/content/common/input/sy...
File content/common/input/synthetic_web_input_event_builders.cc (right):

https://codereview.chromium.org/999423003/diff/280001/content/common/input/sy...
content/common/input/synthetic_web_input_event_builders.cc:15: uint64
get_next_event_id() {
On 2015/04/08 12:24:26, tdresser wrote:
> Can we factor this method out into a helper somewhere? Perhaps something in
the
> events_base target would work?

Done.

https://codereview.chromium.org/999423003/diff/280001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/280001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:50: virtual uint64 GetUniqueId()
const = 0;
On 2015/04/08 12:24:26, tdresser wrote:
> I would prefer we call this the GetUniqueEventId, and clarify in the comment
how
> its different from GetId().

Done.

[lanwei, 2015-04-09 05:18:01]

Patchset #6 (id:320001) has been deleted

[tdresser, 2015-04-09 12:33:06]

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.cc:519: uint64
uniqueEventId,
unique_touch_event_id

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.h (right):

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.h:148: uint64
uniqueEventId,
unique_event_id

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.h:148: uint64
uniqueEventId,
I'm not clear on when you're calling it unique_touch_event_id, and when you're
calling it unique_event_id.

I think in this case, it should probably be unique_touch_event_id.

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:79: return
unique_event_id_;
Did you figure out why there were no test failures when you returned 0 here
before?

https://codereview.chromium.org/999423003/diff/340001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/340001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:53: // event.
This comment isn't quite right, because on Android, we start with a motion
event. 

I'd just say this is a unique identifier for this motion event.

[jdduke (slow), 2015-04-09 15:40:35]

https://codereview.chromium.org/999423003/diff/100001/content/common/input_me...
File content/common/input_messages.h (right):

https://codereview.chromium.org/999423003/diff/100001/content/common/input_me...
content/common/input_messages.h:121: IPC::WebInputEventPointer /* event */,
So, one thought that might make things simpler is this.

We add a |optional_event_id| token in the sent message, and we return that in
the ack. So we populate that for touches, and not for other things, so there's
no burden on the sender of the ack to distinguish between touches.

On the sender side (InputRouter), we can simply populate that value with the
WebTouchEvent's unique id. THoughts?

https://codereview.chromium.org/999423003/diff/100001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.h (right):

https://codereview.chromium.org/999423003/diff/100001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.h:34: void OnSyncTouchEventAck(const
uint64 unique_touch_event_id,
I don't think we need the "touch_" addition here.

https://codereview.chromium.org/999423003/diff/340001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/340001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:50: virtual int GetId() const = 0;
Hmm, so why don't we just have a GetPointerId() accessor like we have GetX(),
GetY(), that just returns GetPointerId(0)?

[jdduke (slow), 2015-04-09 15:41:46]

On 2015/04/09 15:40:35, jdduke wrote:
>
https://codereview.chromium.org/999423003/diff/100001/content/common/input_me...
> File content/common/input_messages.h (right):
> 
>
https://codereview.chromium.org/999423003/diff/100001/content/common/input_me...
> content/common/input_messages.h:121: IPC::WebInputEventPointer /* event */,
> So, one thought that might make things simpler is this.
> 
> We add a |optional_event_id| token in the sent message, and we return that in
> the ack. So we populate that for touches, and not for other things, so there's
> no burden on the sender of the ack to distinguish between touches.
> 
> On the sender side (InputRouter), we can simply populate that value with the
> WebTouchEvent's unique id. THoughts?
> 
>
https://codereview.chromium.org/999423003/diff/100001/ui/events/gestures/gest...
> File ui/events/gestures/gesture_provider_aura.h (right):
> 
>
https://codereview.chromium.org/999423003/diff/100001/ui/events/gestures/gest...
> ui/events/gestures/gesture_provider_aura.h:34: void OnSyncTouchEventAck(const
> uint64 unique_touch_event_id,
> I don't think we need the "touch_" addition here.

Oops, please ignore the two above comments, looks like they were drafts from a
while ago and I published without checking.

[lanwei, 2015-04-15 21:35:59]

Patchset #7 (id:360001) has been deleted

[lanwei, 2015-04-16 16:04:56]

Patchset #7 (id:380001) has been deleted

[lanwei, 2015-04-16 16:24:26]

Patchset #7 (id:400001) has been deleted

[lanwei, 2015-04-16 17:09:07]

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_impl.h (right):

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
content/browser/renderer_host/input/input_router_impl.h:148: uint64
uniqueEventId,
On 2015/04/09 12:33:06, tdresser wrote:
> I'm not clear on when you're calling it unique_touch_event_id, and when you're
> calling it unique_event_id.
> 
> I think in this case, it should probably be unique_touch_event_id.

Done.

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/340001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:79: return
unique_event_id_;
On 2015/04/09 12:33:06, tdresser wrote:
> Did you figure out why there were no test failures when you returned 0 here
> before?

The unit test does not use this type, it uses WebTouchEvent directly.

https://codereview.chromium.org/999423003/diff/340001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/340001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:53: // event.
On 2015/04/09 12:33:06, tdresser wrote:
> This comment isn't quite right, because on Android, we start with a motion
> event. 
> 
> I'd just say this is a unique identifier for this motion event.

Done.

[tdresser, 2015-04-16 17:33:34]

LGTM with nits (but wait for Jared's LGTM)

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:124: if (scroll_event_sent_) {
No reason to add braces here.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_buffer.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_buffer.cc:134: LOG(ERROR) << " 
ResampleMotionEvent pointer_count  " << pointer_count;
Remove log.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (left):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:30:
event.set_id(1);
Shouldn't we still be able to verify that the pointer ids work correctly?

[jdduke (slow), 2015-04-17 20:30:01]

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:68:
unique_event_id_(ui::GetNextTouchEventId()) {
Shouldn't this ID come from event.uniqueTouchEventId?

https://codereview.chromium.org/999423003/diff/420001/content/common/input/we...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/999423003/diff/420001/content/common/input/we...
content/common/input/web_input_event_traits.cc:116:
static_cast<uint32_t>(event.uniqueTouchEventId));
I think we have some 64-bit printf-style helpers for formatting, but I don't
seem to recall where they are.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:110: scoped_ptr<MotionEvent> Clone()
const;
So what's the right thing to do if/when we Clone the event? Do we just copy the
unique id?

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:49: action_index_(0),
unique_event_id_ needs to be initialized in all of these constructors

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:259: action_index_ =
other.action_index_;
Don't we need to copy the unique_event_id_?

https://codereview.chromium.org/999423003/diff/420001/ui/events/test/motion_e...
File ui/events/test/motion_event_test_utils.h (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/test/motion_e...
ui/events/test/motion_event_test_utils.h:54: void SetPointerId(int id);
Shouldn't this be |SetPointerId(size_t index, int id)|? Otherwise maybe call
this |SetPrimaryPointerId(int id)|?

[lanwei, 2015-04-20 19:58:59]

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:68:
unique_event_id_(ui::GetNextTouchEventId()) {
On 2015/04/17 20:30:01, jdduke wrote:
> Shouldn't this ID come from event.uniqueTouchEventId?

This WebTouchEvent does not have a valid unique_event_id, so we need to generate
one for MotionEventWeb.

https://codereview.chromium.org/999423003/diff/420001/content/common/input/we...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/999423003/diff/420001/content/common/input/we...
content/common/input/web_input_event_traits.cc:116:
static_cast<uint32_t>(event.uniqueTouchEventId));
On 2015/04/17 20:30:01, jdduke wrote:
> I think we have some 64-bit printf-style helpers for formatting, but I don't
> seem to recall where they are.

I found "PRIu64", is this the one you have seen before?

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:124: if (scroll_event_sent_) {
On 2015/04/16 17:33:34, tdresser wrote:
> No reason to add braces here.

Done.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:110: scoped_ptr<MotionEvent> Clone()
const;
On 2015/04/17 20:30:01, jdduke wrote:
> So what's the right thing to do if/when we Clone the event? Do we just copy
the
> unique id?

We can just copy the unique_event_id when we clone the MotionEvent.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_buffer.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_buffer.cc:134: LOG(ERROR) << " 
ResampleMotionEvent pointer_count  " << pointer_count;
On 2015/04/16 17:33:34, tdresser wrote:
> Remove log.

Done.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:49: action_index_(0),
On 2015/04/17 20:30:01, jdduke wrote:
> unique_event_id_ needs to be initialized in all of these constructors

The MotionEventGeneric's unique_event_id is from TouchEvent, so we do not
generate it in its constructor.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:259: action_index_ =
other.action_index_;
On 2015/04/17 20:30:01, jdduke wrote:
> Don't we need to copy the unique_event_id_?

Done.

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (left):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:30:
event.set_id(1);
On 2015/04/16 17:33:34, tdresser wrote:
> Shouldn't we still be able to verify that the pointer ids work correctly?

Done.

https://codereview.chromium.org/999423003/diff/420001/ui/events/test/motion_e...
File ui/events/test/motion_event_test_utils.h (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/test/motion_e...
ui/events/test/motion_event_test_utils.h:54: void SetPointerId(int id);
On 2015/04/17 20:30:01, jdduke wrote:
> Shouldn't this be |SetPointerId(size_t index, int id)|? Otherwise maybe call
> this |SetPrimaryPointerId(int id)|?

Done.

[tdresser, 2015-04-20 20:12:09]

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:68:
unique_event_id_(ui::GetNextTouchEventId()) {
On 2015/04/20 19:58:59, lanwei wrote:
> On 2015/04/17 20:30:01, jdduke wrote:
> > Shouldn't this ID come from event.uniqueTouchEventId?
> 
> This WebTouchEvent does not have a valid unique_event_id, so we need to
generate
> one for MotionEventWeb.

Shouldn't it have a valid unique_event_id?

[tdresser, 2015-04-20 20:26:20]

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/420001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:68:
unique_event_id_(ui::GetNextTouchEventId()) {
On 2015/04/20 20:12:09, tdresser wrote:
> On 2015/04/20 19:58:59, lanwei wrote:
> > On 2015/04/17 20:30:01, jdduke wrote:
> > > Shouldn't this ID come from event.uniqueTouchEventId?
> > 
> > This WebTouchEvent does not have a valid unique_event_id, so we need to
> generate
> > one for MotionEventWeb.
> 
> Shouldn't it have a valid unique_event_id?

Lan pointed out that this isn't the case with synthesized events like touch
cancels.

[jdduke (slow), 2015-04-21 14:56:51]

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:49: action_index_(0),
On 2015/04/20 19:58:59, lanwei wrote:
> On 2015/04/17 20:30:01, jdduke wrote:
> > unique_event_id_ needs to be initialized in all of these constructors
> 
> The MotionEventGeneric's unique_event_id is from TouchEvent, so we do not
> generate it in its constructor.

I don't fully understand this comment, what do you mean it's from TouchEvent?
Even if we don't create a new ID here, don't we have to initialize the member
variable to 0?

https://codereview.chromium.org/999423003/diff/440001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/440001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:68:
unique_event_id_(ui::GetNextTouchEventId()) {
The only place we use MotionEventWeb is in touch_emulator.cc. I'd be OK if we
moved unique ID generation there (at the start of
TouchEmulator::HandleEmulatedTouchEvent()|, then we can simply copy the id from
the source WebTouchEvent here.

https://codereview.chromium.org/999423003/diff/440001/ui/events/test/motion_e...
File ui/events/test/motion_event_test_utils.cc (right):

https://codereview.chromium.org/999423003/diff/440001/ui/events/test/motion_e...
ui/events/test/motion_event_test_utils.cc:177: << "\n ID: " <<
event.GetPointerId() << "\n Action: " << event.GetAction();
This should probably now be |GetUniquEventId|, and we should log the pointer ids
below in the per pointer properties (by tool/pressure/orientation).

[lanwei, 2015-04-21 20:35:29]

Patchset #9 (id:460001) has been deleted

[lanwei, 2015-04-21 20:44:56]

Patchset #9 (id:480001) has been deleted

[lanwei, 2015-04-21 21:32:45]

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.cc (right):

https://codereview.chromium.org/999423003/diff/420001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:49: action_index_(0),
On 2015/04/21 14:56:50, jdduke wrote:
> On 2015/04/20 19:58:59, lanwei wrote:
> > On 2015/04/17 20:30:01, jdduke wrote:
> > > unique_event_id_ needs to be initialized in all of these constructors
> > 
> > The MotionEventGeneric's unique_event_id is from TouchEvent, so we do not
> > generate it in its constructor.
> 
> I don't fully understand this comment, what do you mean it's from TouchEvent?
> Even if we don't create a new ID here, don't we have to initialize the member
> variable to 0?

Sorry, I was wrong, I thought we do not pass MotionEventGeneric objects to the
CreateWebTouchEventFromMotionEvent function, so we do not care if it has a
unique_event_id value. We should initialize the unique_event_id in all the
constructors.

https://codereview.chromium.org/999423003/diff/440001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/440001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:68:
unique_event_id_(ui::GetNextTouchEventId()) {
On 2015/04/21 14:56:50, jdduke wrote:
> The only place we use MotionEventWeb is in touch_emulator.cc. I'd be OK if we
> moved unique ID generation there (at the start of
> TouchEmulator::HandleEmulatedTouchEvent()|, then we can simply copy the id
from
> the source WebTouchEvent here.

Done.

https://codereview.chromium.org/999423003/diff/440001/ui/events/test/motion_e...
File ui/events/test/motion_event_test_utils.cc (right):

https://codereview.chromium.org/999423003/diff/440001/ui/events/test/motion_e...
ui/events/test/motion_event_test_utils.cc:177: << "\n ID: " <<
event.GetPointerId() << "\n Action: " << event.GetAction();
On 2015/04/21 14:56:51, jdduke wrote:
> This should probably now be |GetUniquEventId|, and we should log the pointer
ids
> below in the per pointer properties (by tool/pressure/orientation).

Done.

[jdduke (slow), 2015-04-21 22:40:16]

Thanks, just a couple more nits! Should we add sadrul@ to get his thoughts?

https://codereview.chromium.org/999423003/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/999423003/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:349:
touch_event_.uniqueTouchEventId = ui::GetNextTouchEventId();
Can we move this logic into |HandleEmulatedTouchEvent()| and populate the ID at
the beginning of the function? That should also cover the FillTouchEventAndPoint
case.

https://codereview.chromium.org/999423003/diff/500001/ui/events/base_event_ut...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/base_event_ut...
ui/events/base_event_utils.cc:10: // Set the first touch event ID to 1 because
we set id to 0 for other types
Should we be asserting that we're on the UI thread? Will that mess with the
tests?

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:49: 
Nit: delete this extra newline.

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.cc (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:224:
unique_event_id_(ui::GetNextTouchEventId()),
I can't say I love using globals, but this is probably the simplest thing to do
I guess.

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:16: EXPECT_EQ(1U,
event.GetPointerCount());
Maybe add a |DCHECK_NE(0U, event.GetUniqueEventId());|?

[tdresser, 2015-04-22 12:59:00]

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.cc (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:224:
unique_event_id_(ui::GetNextTouchEventId()),
On 2015/04/21 22:40:16, jdduke wrote:
> I can't say I love using globals, but this is probably the simplest thing to
do
> I guess.

I agree, but I think that ensuring that these ids are unique across all forms of
input is worth it. I'm worries about cases where, for example, you're touching
the screen and using touch emulation at the same time, and you end up with
collisions.

[lanwei, 2015-04-22 19:56:39]

Patchset #10 (id:520001) has been deleted

[lanwei, 2015-04-22 20:51:29]

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

[lanwei, 2015-04-22 20:51:31]

sadrul@chromium.org: Please review changes in 

ui/events/

Thank you.

https://codereview.chromium.org/999423003/diff/500001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/999423003/diff/500001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:349:
touch_event_.uniqueTouchEventId = ui::GetNextTouchEventId();
On 2015/04/21 22:40:16, jdduke wrote:
> Can we move this logic into |HandleEmulatedTouchEvent()| and populate the ID
at
> the beginning of the function? That should also cover the
FillTouchEventAndPoint
> case.

I put setting uniqueTouchEventId into these two functions so we can actually
change touch_event_'s uniqueTouchEventId, but I think this does not matter
because we do not need its uniqueTouchEventId except when we use it create
MotionEventWeb object.

https://codereview.chromium.org/999423003/diff/500001/ui/events/base_event_ut...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/base_event_ut...
ui/events/base_event_utils.cc:10: // Set the first touch event ID to 1 because
we set id to 0 for other types
On 2015/04/21 22:40:16, jdduke wrote:
> Should we be asserting that we're on the UI thread? Will that mess with the
> tests?

Done.

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event.h:49: 
On 2015/04/21 22:40:16, jdduke wrote:
> Nit: delete this extra newline.

Done.

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic.cc (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic.cc:224:
unique_event_id_(ui::GetNextTouchEventId()),
On 2015/04/22 12:59:00, tdresser wrote:
> On 2015/04/21 22:40:16, jdduke wrote:
> > I can't say I love using globals, but this is probably the simplest thing to
> do
> > I guess.
> 
> I agree, but I think that ensuring that these ids are unique across all forms
of
> input is worth it. I'm worries about cases where, for example, you're touching
> the screen and using touch emulation at the same time, and you end up with
> collisions.

I also agree, in many cases such as for different platforms it does not matter
if they have same unique_event_id, but it is better to keep this way Tim gave an
good example.

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (right):

https://codereview.chromium.org/999423003/diff/500001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:16: EXPECT_EQ(1U,
event.GetPointerCount());
On 2015/04/21 22:40:16, jdduke wrote:
> Maybe add a |DCHECK_NE(0U, event.GetUniqueEventId());|?

Done.

[sadrul, 2015-04-22 21:00:13]

Consider using base::AtomicSequenceNumber and base::StaticAtomicSequenceNumber
instead of introducing ui::GetNextTouchEventId

[jdduke (slow), 2015-04-22 21:06:04]

On 2015/04/22 21:00:13, sadrul wrote:
> Consider using base::AtomicSequenceNumber and base::StaticAtomicSequenceNumber
> instead of introducing ui::GetNextTouchEventId

I had a suspicion we had something like that lying around, but didn't find it
after a cursor glance. Thanks.

[jdduke (slow), 2015-04-22 21:07:22]

On 2015/04/22 21:06:04, jdduke wrote:
> On 2015/04/22 21:00:13, sadrul wrote:
> > Consider using base::AtomicSequenceNumber and
base::StaticAtomicSequenceNumber
> > instead of introducing ui::GetNextTouchEventId
> 
> I had a suspicion we had something like that lying around, but didn't find it
> after a cursor glance. Thanks.

*cursory*

[tdresser, 2015-04-23 15:49:56]

On 2015/04/22 21:07:22, jdduke wrote:
> On 2015/04/22 21:06:04, jdduke wrote:
> > On 2015/04/22 21:00:13, sadrul wrote:
> > > Consider using base::AtomicSequenceNumber and
> base::StaticAtomicSequenceNumber
> > > instead of introducing ui::GetNextTouchEventId
> > 
> > I had a suspicion we had something like that lying around, but didn't find
it
> > after a cursor glance. Thanks.
> 
> *cursory*

We'll still need a wrapper around StaticAtomicSequenceNumber, to give us an
"invalid" value, which is never generated by the id generator.

If we need a wrapper anyways, and don't need to rely on the accesses being
atomic, I'm not clear on why StaticAtomicSequenceNumber is superior. What's the
reasoning here?

[jdduke (slow), 2015-04-23 15:52:20]

On 2015/04/23 15:49:56, tdresser wrote:
> On 2015/04/22 21:07:22, jdduke wrote:
> > On 2015/04/22 21:06:04, jdduke wrote:
> > > On 2015/04/22 21:00:13, sadrul wrote:
> > > > Consider using base::AtomicSequenceNumber and
> > base::StaticAtomicSequenceNumber
> > > > instead of introducing ui::GetNextTouchEventId
> > > 
> > > I had a suspicion we had something like that lying around, but didn't find
> it
> > > after a cursor glance. Thanks.
> > 
> > *cursory*
> 
> We'll still need a wrapper around StaticAtomicSequenceNumber, to give us an
> "invalid" value, which is never generated by the id generator.
> 
> If we need a wrapper anyways, and don't need to rely on the accesses being
> atomic, I'm not clear on why StaticAtomicSequenceNumber is superior. What's
the
> reasoning here?

Hmm, perhasps consistency? There appears to be precedent for massaging the
result of StaticAtomicSequenceNumber, e.g., here with animation IDs:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/anima....

[tdresser, 2015-04-23 15:56:59]

On 2015/04/23 15:52:20, jdduke wrote:
> On 2015/04/23 15:49:56, tdresser wrote:
> > On 2015/04/22 21:07:22, jdduke wrote:
> > > On 2015/04/22 21:06:04, jdduke wrote:
> > > > On 2015/04/22 21:00:13, sadrul wrote:
> > > > > Consider using base::AtomicSequenceNumber and
> > > base::StaticAtomicSequenceNumber
> > > > > instead of introducing ui::GetNextTouchEventId
> > > > 
> > > > I had a suspicion we had something like that lying around, but didn't
find
> > it
> > > > after a cursor glance. Thanks.
> > > 
> > > *cursory*
> > 
> > We'll still need a wrapper around StaticAtomicSequenceNumber, to give us an
> > "invalid" value, which is never generated by the id generator.
> > 
> > If we need a wrapper anyways, and don't need to rely on the accesses being
> > atomic, I'm not clear on why StaticAtomicSequenceNumber is superior. What's
> the
> > reasoning here?
> 
> Hmm, perhasps consistency? There appears to be precedent for massaging the
> result of StaticAtomicSequenceNumber, e.g., here with animation IDs:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/anima....

Isn't that broken if you wrap around? 
We're going to be calling this from a variety of places, so we'll want to ensure
that all call sites are massaging the output in the same way.

[jdduke (slow), 2015-04-23 16:09:48]

On 2015/04/23 15:56:59, tdresser wrote:
> On 2015/04/23 15:52:20, jdduke wrote:
> > On 2015/04/23 15:49:56, tdresser wrote:
> > > On 2015/04/22 21:07:22, jdduke wrote:
> > > > On 2015/04/22 21:06:04, jdduke wrote:
> > > > > On 2015/04/22 21:00:13, sadrul wrote:
> > > > > > Consider using base::AtomicSequenceNumber and
> > > > base::StaticAtomicSequenceNumber
> > > > > > instead of introducing ui::GetNextTouchEventId
> > > > > 
> > > > > I had a suspicion we had something like that lying around, but didn't
> find
> > > it
> > > > > after a cursor glance. Thanks.
> > > > 
> > > > *cursory*
> > > 
> > > We'll still need a wrapper around StaticAtomicSequenceNumber, to give us
an
> > > "invalid" value, which is never generated by the id generator.
> > > 
> > > If we need a wrapper anyways, and don't need to rely on the accesses being
> > > atomic, I'm not clear on why StaticAtomicSequenceNumber is superior.
What's
> > the
> > > reasoning here?
> > 
> > Hmm, perhasps consistency? There appears to be precedent for massaging the
> > result of StaticAtomicSequenceNumber, e.g., here with animation IDs:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/anima....
> 
> Isn't that broken if you wrap around? 
> We're going to be calling this from a variety of places, so we'll want to
ensure
> that all call sites are massaging the output in the same way.

Depends how they're used. I think if we just treat 0 as the magic invalid value,
it doesn't really matter that we're nonmonotonic. We really just need
"reasonably adjacent" touch events to have different values (i.e., it doesn't
matter that we'll get negative IDs, as long as we skip 0). I don't think we need
to worry about collisions.

[jdduke (slow), 2015-04-23 16:14:13]

On 2015/04/23 16:09:48, jdduke wrote:
> Depends how they're used. I think if we just treat 0 as the magic invalid
value,
> it doesn't really matter that we're nonmonotonic. We really just need
> "reasonably adjacent" touch events to have different values (i.e., it doesn't
> matter that we'll get negative IDs, as long as we skip 0). I don't think we
need
> to worry about collisions.

Wait, are you worried about wrapping for 32 bit values period (signed OR
unsigned), or are you worried about getting negative IDs? I was speaking more
toward the latter.

[sadrul, 2015-04-23 16:56:23]

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:15: #include
"ui/events/base_event_utils.h"
Not needed

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:229:
event.uniqueTouchEventId = ui::GetNextTouchEventId();
Should this happen in WebTouchEventTraits::ResetTypeAndTouchStates() instead?

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
ui/events/base_event_utils.h:15: 
Why not add this in event_utils.h instead?

https://codereview.chromium.org/999423003/diff/560001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/999423003/diff/560001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:128:
set_unique_event_id(touch.unique_event_id());
For multi-touch motion-events, the 'unique id' of the motion event is going to
change during its lifetime. Is that safe?

[tdresser, 2015-04-23 17:13:20]

jdduke: I was just pointing out that
https://code.google.com/p/chromium/codesearch#chromium/src/cc/animation/anima...
doesn't handle wrap around correctly, as if you wrap around you can end up
returning 0.

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
ui/events/base_event_utils.h:15: 
On 2015/04/23 16:56:23, sadrul wrote:
> Why not add this in event_utils.h instead?

Is event_utils.h included on Android?

[sadrul, 2015-04-23 17:16:28]

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
ui/events/base_event_utils.h:15: 
On 2015/04/23 17:13:19, tdresser wrote:
> On 2015/04/23 16:56:23, sadrul wrote:
> > Why not add this in event_utils.h instead?
> 
> Is event_utils.h included on Android?

If it's not, can it be?

[tdresser, 2015-04-23 17:27:21]

On 2015/04/23 17:16:28, sadrul wrote:
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> File ui/events/base_event_utils.h (right):
> 
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> ui/events/base_event_utils.h:15: 
> On 2015/04/23 17:13:19, tdresser wrote:
> > On 2015/04/23 16:56:23, sadrul wrote:
> > > Why not add this in event_utils.h instead?
> > 
> > Is event_utils.h included on Android?
> 
> If it's not, can it be?

A lot of event_utils.h deals with types that we don't use on Android, like
NativeEvent. It's almost entirely Aura specific logic.

[sadrul, 2015-04-23 17:31:08]

On 2015/04/23 17:27:21, tdresser wrote:
> On 2015/04/23 17:16:28, sadrul wrote:
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > File ui/events/base_event_utils.h (right):
> > 
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > ui/events/base_event_utils.h:15: 
> > On 2015/04/23 17:13:19, tdresser wrote:
> > > On 2015/04/23 16:56:23, sadrul wrote:
> > > > Why not add this in event_utils.h instead?
> > > 
> > > Is event_utils.h included on Android?
> > 
> > If it's not, can it be?
> 
> A lot of event_utils.h deals with types that we don't use on Android, like
> NativeEvent. It's almost entirely Aura specific logic.

NativeEvent is defined for android as jobject.

event_utils.cc file contains platform-independent code, and so this can go in
there. Have you tried it?

[tdresser, 2015-04-23 17:40:36]

On 2015/04/23 17:31:08, sadrul wrote:
> On 2015/04/23 17:27:21, tdresser wrote:
> > On 2015/04/23 17:16:28, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > > File ui/events/base_event_utils.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > > ui/events/base_event_utils.h:15: 
> > > On 2015/04/23 17:13:19, tdresser wrote:
> > > > On 2015/04/23 16:56:23, sadrul wrote:
> > > > > Why not add this in event_utils.h instead?
> > > > 
> > > > Is event_utils.h included on Android?
> > > 
> > > If it's not, can it be?
> > 
> > A lot of event_utils.h deals with types that we don't use on Android, like
> > NativeEvent. It's almost entirely Aura specific logic.
> 
> NativeEvent is defined for android as jobject.
> 
> event_utils.cc file contains platform-independent code, and so this can go in
> there. Have you tried it?

Oh, interesting.
I haven't tried it, but as of https://codereview.chromium.org/241213002, we
didn't include the events target on Android, which seems like a trend we should
continue. I suppose it's possible something has started pulling in the events
target on Android since then.

[sadrul, 2015-04-23 21:01:35]

On 2015/04/23 17:40:36, tdresser wrote:
> On 2015/04/23 17:31:08, sadrul wrote:
> > On 2015/04/23 17:27:21, tdresser wrote:
> > > On 2015/04/23 17:16:28, sadrul wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > > > File ui/events/base_event_utils.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > > > ui/events/base_event_utils.h:15: 
> > > > On 2015/04/23 17:13:19, tdresser wrote:
> > > > > On 2015/04/23 16:56:23, sadrul wrote:
> > > > > > Why not add this in event_utils.h instead?
> > > > > 
> > > > > Is event_utils.h included on Android?
> > > > 
> > > > If it's not, can it be?
> > > 
> > > A lot of event_utils.h deals with types that we don't use on Android, like
> > > NativeEvent. It's almost entirely Aura specific logic.
> > 
> > NativeEvent is defined for android as jobject.
> > 
> > event_utils.cc file contains platform-independent code, and so this can go
in
> > there. Have you tried it?
> 
> Oh, interesting.
> I haven't tried it, but as of https://codereview.chromium.org/241213002, we
> didn't include the events target on Android, which seems like a trend we
should
> continue. I suppose it's possible something has started pulling in the events
> target on Android since then.

Ah I see. They are in separate build targets. Hrm ...

If events target is already being used on android, then this new function should
be there. But it doesn't make sense to pull in all of events just for this. So,
if 'events' target is currently not compiled on android, then having this
base_event_utils is ... unfortunate (because it's not really clear if future
such functions should go to base_event_utils or event_utils), but acceptable.

[tdresser, 2015-04-24 12:48:11]

On 2015/04/23 21:01:35, sadrul wrote:
> On 2015/04/23 17:40:36, tdresser wrote:
> > On 2015/04/23 17:31:08, sadrul wrote:
> > > On 2015/04/23 17:27:21, tdresser wrote:
> > > > On 2015/04/23 17:16:28, sadrul wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > > > > File ui/events/base_event_utils.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
> > > > > ui/events/base_event_utils.h:15: 
> > > > > On 2015/04/23 17:13:19, tdresser wrote:
> > > > > > On 2015/04/23 16:56:23, sadrul wrote:
> > > > > > > Why not add this in event_utils.h instead?
> > > > > > 
> > > > > > Is event_utils.h included on Android?
> > > > > 
> > > > > If it's not, can it be?
> > > > 
> > > > A lot of event_utils.h deals with types that we don't use on Android,
like
> > > > NativeEvent. It's almost entirely Aura specific logic.
> > > 
> > > NativeEvent is defined for android as jobject.
> > > 
> > > event_utils.cc file contains platform-independent code, and so this can go
> in
> > > there. Have you tried it?
> > 
> > Oh, interesting.
> > I haven't tried it, but as of https://codereview.chromium.org/241213002, we
> > didn't include the events target on Android, which seems like a trend we
> should
> > continue. I suppose it's possible something has started pulling in the
events
> > target on Android since then.
> 
> Ah I see. They are in separate build targets. Hrm ...
> 
> If events target is already being used on android, then this new function
should
> be there. But it doesn't make sense to pull in all of events just for this.
So,
> if 'events' target is currently not compiled on android, then having this
> base_event_utils is ... unfortunate (because it's not really clear if future
> such functions should go to base_event_utils or event_utils), but acceptable.

Lan, can you add a comment to event_utils.h and base_event_utils.h indicating
when each should be used? I think that the determining factor is whether or not
the function is used on Android.

[lanwei, 2015-04-24 20:21:09]

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:15: #include
"ui/events/base_event_utils.h"
On 2015/04/23 16:56:23, sadrul wrote:
> Not needed

Done.

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:229:
event.uniqueTouchEventId = ui::GetNextTouchEventId();
On 2015/04/23 16:56:23, sadrul wrote:
> Should this happen in WebTouchEventTraits::ResetTypeAndTouchStates() instead?

Tim pointed out that ResetTypeAndTouchStates may be called multiple times and
HandleEmulatedTouchEvent will only be called once, so it is safe to put it here.
We add this setting uniqueTouchEventId here is because this MotionEventWeb
object is generated from WebTouchEvent object which does not have a valid
uniqueTouchEventId. Normally WebTouchEvent are generated from MotionEvent which
are from TouchEvent, which will have a valid uniqueTouchEventId.

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
File ui/events/base_event_utils.h (right):

https://codereview.chromium.org/999423003/diff/560001/ui/events/base_event_ut...
ui/events/base_event_utils.h:15: 
On 2015/04/23 17:16:28, sadrul wrote:
> On 2015/04/23 17:13:19, tdresser wrote:
> > On 2015/04/23 16:56:23, sadrul wrote:
> > > Why not add this in event_utils.h instead?
> > 
> > Is event_utils.h included on Android?
> 
> If it's not, can it be?

I add a comment to clarify that event_utils.h will not be included in Android,
and base_event_utils.h can be used in all the platforms.

https://codereview.chromium.org/999423003/diff/560001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/999423003/diff/560001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:128:
set_unique_event_id(touch.unique_event_id());
On 2015/04/23 16:56:23, sadrul wrote:
> For multi-touch motion-events, the 'unique id' of the motion event is going to
> change during its lifetime. Is that safe?

For the unique_event_id, we only use it when the gesture recognizer valid the
touch events' acks are associated with the right touch event. Also in our
ongoing CL( Issue 997283002), we use it to check if the ack is from async
touchmove, so it should be safe and we want it to be different every time when
we dispatch them. We have a PointerId which is identical for every touch point.

[tdresser, 2015-04-27 14:02:24]

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/999423003/diff/560001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:229:
event.uniqueTouchEventId = ui::GetNextTouchEventId();
On 2015/04/24 20:21:09, lanwei wrote:
> On 2015/04/23 16:56:23, sadrul wrote:
> > Should this happen in WebTouchEventTraits::ResetTypeAndTouchStates()
instead?
> 
> Tim pointed out that ResetTypeAndTouchStates may be called multiple times and
> HandleEmulatedTouchEvent will only be called once, so it is safe to put it
here.
> We add this setting uniqueTouchEventId here is because this MotionEventWeb
> object is generated from WebTouchEvent object which does not have a valid
> uniqueTouchEventId. Normally WebTouchEvent are generated from MotionEvent
which
> are from TouchEvent, which will have a valid uniqueTouchEventId.

Currently we only call ResetTypeAndTouchStates once per event, but I can picture
us introducing bugs by later calling ResetTypeAndTouchStates multiple times. It
seems unlikely that we'd switch to calling HandleEmulatedTouchEvent multiple
times per event.

We could move this to ResetTypeAndTouchStates, and DCHECK that the
uniqueTouchEventId is unset before generating a new one, but this seems more
straight forward to me.

[jdduke (slow), 2015-04-27 17:11:07]

ui/events/gesture_detection and content/browser/renderer_host/input lgtm.

In the description, can you also link to the CL that will actually use this
value when ack'ing to the TouchDispositionGestureFilter?

https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.h (right):

https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.h:61: uint32
GetUniqueEventId() const override;
I'd be OK if this was just |GetUniqueId()|, the "event" part seems a little
redundant, but it's not a big deal, either way.

https://codereview.chromium.org/999423003/diff/580001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (right):

https://codereview.chromium.org/999423003/diff/580001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:82:
MotionEventGeneric event(MotionEvent::ACTION_DOWN,
Can you make sure the event id is cloned for Clone(), and that it's different
for Cancel()?

[tdresser, 2015-04-27 17:13:46]

https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.h (right):

https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.h:61: uint32
GetUniqueEventId() const override;
On 2015/04/27 17:11:07, jdduke wrote:
> I'd be OK if this was just |GetUniqueId()|, the "event" part seems a little
> redundant, but it's not a big deal, either way.

I like GetUniqueEventId(), as it clarifies that the id is unique per event, not
unique per pointer.

[sadrul, 2015-04-27 19:00:43]

lgtm

https://codereview.chromium.org/999423003/diff/580001/ui/events/base_event_ut...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/999423003/diff/580001/ui/events/base_event_ut...
ui/events/base_event_utils.cc:18: if (id==0)
spaces around ==

[lanwei, 2015-04-28 16:14:50]

On 2015/04/27 17:11:07, jdduke wrote:
> ui/events/gesture_detection and content/browser/renderer_host/input lgtm.
> 
> In the description, can you also link to the CL that will actually use this
> value when ack'ing to the TouchDispositionGestureFilter?

I will add the link the CL, once I have this CL ready for review.

> 
>
https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
> File content/browser/renderer_host/input/motion_event_android.h (right):
> 
>
https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
> content/browser/renderer_host/input/motion_event_android.h:61: uint32
> GetUniqueEventId() const override;
> I'd be OK if this was just |GetUniqueId()|, the "event" part seems a little
> redundant, but it's not a big deal, either way.
> 
>
https://codereview.chromium.org/999423003/diff/580001/ui/events/gesture_detec...
> File ui/events/gesture_detection/motion_event_generic_unittest.cc (right):
> 
>
https://codereview.chromium.org/999423003/diff/580001/ui/events/gesture_detec...
> ui/events/gesture_detection/motion_event_generic_unittest.cc:82:
> MotionEventGeneric event(MotionEvent::ACTION_DOWN,
> Can you make sure the event id is cloned for Clone(), and that it's different
> for Cancel()?

[lanwei, 2015-04-28 16:24:34]

lanwei@chromium.org changed reviewers:
+ sky@chromium.org

[lanwei, 2015-04-28 16:24:35]

sky@chromium.org: Please review changes in 

Could you please take a look at components/html_viewer/touch_handler.cc?

We do not use set_id method any more.

Thank you.

https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android.h (right):

https://codereview.chromium.org/999423003/diff/580001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android.h:61: uint32
GetUniqueEventId() const override;
On 2015/04/27 17:13:46, tdresser wrote:
> On 2015/04/27 17:11:07, jdduke wrote:
> > I'd be OK if this was just |GetUniqueId()|, the "event" part seems a little
> > redundant, but it's not a big deal, either way.
> 
> I like GetUniqueEventId(), as it clarifies that the id is unique per event,
not
> unique per pointer.

I agree that GetUniqueEventId is clearer.

https://codereview.chromium.org/999423003/diff/580001/ui/events/base_event_ut...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/999423003/diff/580001/ui/events/base_event_ut...
ui/events/base_event_utils.cc:18: if (id==0)
On 2015/04/27 19:00:43, sadrul wrote:
> spaces around ==

Done.

https://codereview.chromium.org/999423003/diff/580001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_generic_unittest.cc (right):

https://codereview.chromium.org/999423003/diff/580001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_generic_unittest.cc:82:
MotionEventGeneric event(MotionEvent::ACTION_DOWN,
On 2015/04/27 17:11:07, jdduke wrote:
> Can you make sure the event id is cloned for Clone(), and that it's different
> for Cancel()?

Done.

[jdduke (slow), 2015-04-28 16:26:58]

https://codereview.chromium.org/999423003/diff/600001/content/common/input/sy...
File content/common/input/synthetic_web_input_event_builders.cc (right):

https://codereview.chromium.org/999423003/diff/600001/content/common/input/sy...
content/common/input/synthetic_web_input_event_builders.cc:176: WebTouchPoint&
point = touches[touchesLength];
At some point, we'll probably need to assign a new ID each time a point is
manipulated (PressPoint/MovePoint/etc...). That doesn't necessarily need to
happen, but something to keep in mind when tests start relying on valid/unique
IDs.

[lanwei, 2015-04-28 16:30:59]

On 2015/04/28 16:26:58, jdduke wrote:
>
https://codereview.chromium.org/999423003/diff/600001/content/common/input/sy...
> File content/common/input/synthetic_web_input_event_builders.cc (right):
> 
>
https://codereview.chromium.org/999423003/diff/600001/content/common/input/sy...
> content/common/input/synthetic_web_input_event_builders.cc:176: WebTouchPoint&
> point = touches[touchesLength];
> At some point, we'll probably need to assign a new ID each time a point is
> manipulated (PressPoint/MovePoint/etc...). That doesn't necessarily need to
> happen, but something to keep in mind when tests start relying on valid/unique
> IDs.

Thanks for reminding me this, I am award of all the unittests using the same
event, so I am generating new event_id for each new touch event, keep them in a
queue, and also use them when we send acks.

[sky, 2015-04-28 17:00:19]

https://codereview.chromium.org/999423003/diff/600001/components/html_viewer/...
File components/html_viewer/touch_handler.cc (left):

https://codereview.chromium.org/999423003/diff/600001/components/html_viewer/...
components/html_viewer/touch_handler.cc:73:
current_motion_event_->set_id(current_motion_event_->GetId() + 1);
current_motion_event_ lives for quite a while (it's reset on cancel or the last
up). Seems to me the way you have things now you're assuming a new event is
created for each distinct underlying event. That doesn't match the use case
here. Maybe I'm wrong though?

[sky, 2015-04-28 21:27:59]

LGTM

[lanwei, 2015-04-29 17:36:12]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2015-04-29 17:36:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, jdduke@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/999423003/#ps620001
(title: " ")

[commit-bot: I haz the power, 2015-04-29 17:37:13]

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

[commit-bot: I haz the power, 2015-04-29 17:43:01]

Committed patchset #14 (id:620001)

[commit-bot: I haz the power, 2015-04-29 17:44:37]

Patchset 14 (id:??) landed as
https://crrev.com/2574b1b13231fc8348389716ac5630d9f853a1a5
Cr-Commit-Position: refs/heads/master@{#327520}