Cleaning up PointerIdManager and add id re-mapping

Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEventFactory
- Add id re-mapping concept to have guaranteed unique pointer ids among
  touch, stylus, and mouse pointer types.
- Set mouse id to 1
- Set the mouse pointer type in event sender

BUG=546658
Committed: https://crrev.com/06d3f72718785fea41c1e4deffe330afb5e24841
Cr-Commit-Position: refs/heads/master@{#362779}

[Navid Zolghadr, 2015-11-10 17:48:50]

nzolghadr@chromium.org changed reviewers:
+ mustaq@chromium.org, rbyers@chromium.org

[Navid Zolghadr, 2015-11-10 17:48:50]

Hey guys,

I have uploaded this review for the cleanup of PointerIdManager and also adding
the re-mapping of ids. I couldn't find a way to add tests for this class. Let me
know if you have links/pointers on adding tests to the classes in core/event
folder.

Thanks,
Navid

https://codereview.chromium.org/1426643008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerIdManager.cpp (right):

https://codereview.chromium.org/1426643008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerIdManager.cpp:14: // We do not
handle the overflow of currentId as it should be very rare
Is this okay not to handle the overflow case?

[Navid Zolghadr, 2015-11-11 17:20:53]

ptal

[Rick Byers, 2015-11-14 18:24:07]

Sorry for the delay (been busy with BlinkOn etc.).  Mustaq knows this code best
so should do the detailed review.  I took a quick skim and it seems reasonable
to me.

Please add some comments to the declaration of PointerIdManager capturing some
of the design goals we had here.  Eg:
- Behave the same as Edge, even for things not in the spec (like mouse ID == 1)

[Rick Byers, 2015-11-14 18:26:27]

On 2015/11/14 18:24:07, Rick Byers (slow until Nov 23) wrote:
> Sorry for the delay (been busy with BlinkOn etc.).  Mustaq knows this code
best
> so should do the detailed review.  I took a quick skim and it seems reasonable
> to me.
> 
> Please add some comments to the declaration of PointerIdManager capturing some
> of the design goals we had here.  Eg:
> - Behave the same as Edge, even for things not in the spec (like mouse ID ==
1)

Also describe why pointer ID remapping is valuable.

On Mobile IE, does the pointerId match the touch.identifier?  If so then we
should probably do that too (I can imagine scenarios where it might be
convenient to explicitly correlate the two).  But that can come in a follow-up
CL.

[Navid Zolghadr, 2015-11-14 18:34:51]

I will add the comments in the next patch. There are also some layout tests that
need updates which I need to talk to Mustaq about them. I will add them all in
the next patch.

My main concern was whether we agree on this schema of remapping or not. The
only thing that this mapping keeps is the mouse_id=1 (which if I'm not mistaken
and recall correctly it was for Safari you said?). Then it pretty much assigned
increasing values to both touch ids and stylus very similar to what you showed
on your iPhone in your app. Right?

I haven't looked at the pointer ids on Mobile IE. I can do it on Monday. But we
might not be able to satisfy both keeping touch ids as pointer ids and
mouse_id=1.

[Rick Byers, 2015-11-14 19:11:28]

On 2015/11/14 18:34:51, Navid Zolghadr wrote:
> I will add the comments in the next patch. There are also some layout tests
that
> need updates which I need to talk to Mustaq about them. I will add them all in
> the next patch.
> 
> My main concern was whether we agree on this schema of remapping or not. The
> only thing that this mapping keeps is the mouse_id=1 (which if I'm not
mistaken
> and recall correctly it was for Safari you said?).

If remapping is necessary to match the behavior of Edge, then yes I think it's
worth it (and hopefully we can simplify things elsewhere by ripping out the
touch ID mapping higher up in chromium).  It's Edge that has mouse_id=1.  For
pointer events, it's MS Edge that's the reference platform to match.

> Then it pretty much assigned
> increasing values to both touch ids and stylus very similar to what you showed
> on your iPhone in your app. Right?

Yeah but that's for TouchEvents, really a separate topic here.  Safari does not
support pointer events.  For touch events, it's mobile Safari that's the
reference platform to match :-)

> 
> I haven't looked at the pointer ids on Mobile IE. I can do it on Monday. But
we
> might not be able to satisfy both keeping touch ids as pointer ids and
> mouse_id=1.

Yep, understood.  Let's understand what the other browsers do - I suspect we can
both match Safari's behavior with touch events and Edge's behavior with pointer
events while making touch id == pointer ID, but I might be wrong...

[Navid Zolghadr, 2015-11-16 19:24:44]

I tested with the Surface. It matches the current implementation more or less.
The pointer events that mouse fires always have id=1. Those that are coming from
touch or pen will have an increasing number. However, the number only resets
when I restart the Surface and the count seems to be increased every time I
touch the surface regardless of whether it is in the browser or not. So the
minimum number I could get the browser to print as pointer id while touching the
div in the test html was 6. I guess logging it and opening IE took a few
touches.

I couldn't test with Safari on the iPad I found in the library. It just doesn't
print any events as oppose to the other platforms I tested with (i.e. Windows
10/IE, Chrome OS/Chrome, Linux/Chrome). Is there any trick to it?

Having what Surface does for the pointer event ids which looks the same as what
I did here in this patch, I'm not sure how we can match that as well as touch
ids that will be reset to 0 every time all fingers are off the screen (like what
Chrome on Chrome OS or maybe Safari on mobile does).

[mustaq, 2015-11-20 20:13:53]

Sorry for taking so long. The id mapping looks good but I have a few comments
around caller's maintaining of the mapped id pair.

https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerIdManager.h (right):

https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerIdManager.h:35: void remove(const
GeneratedPointer);
Two remove()s is confusing. just pass a ref to PointerIdManager and rawId. See
my comment in EventHandler.

https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3670: const
PointerIdManager::MappedId mappedId =
m_pointerIdManager.add(PointerIdManager::GeneratedPointer(pointerType,
pointerId));
This looks good. One concern is that EventHandler now has to worry too much
about PE ids w/o really doing anything with it. The state maintained through
m_pointerIdManager is unavoidable, but at least GeneratedPointer could be hidden
inside PointerEvent.

What if you just pass a PointerIdManager reference to PointerEvent::create()
methods so that the id mapping invisible to EventHandler?

Am I right that in that case, users of PointerEvent would know nothing about
mapped ids, and therefore, would pass only raw int/unsigned ids to
PointerIdManager methods? PointerEvent will be the only user of mapped ids then.

[Navid Zolghadr, 2015-11-20 20:32:22]

https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3670: const
PointerIdManager::MappedId mappedId =
m_pointerIdManager.add(PointerIdManager::GeneratedPointer(pointerType,
pointerId));
On 2015/11/20 20:13:53, mustaq wrote:
> This looks good. One concern is that EventHandler now has to worry too much
> about PE ids w/o really doing anything with it. The state maintained through
> m_pointerIdManager is unavoidable, but at least GeneratedPointer could be
hidden
> inside PointerEvent.
> 
> What if you just pass a PointerIdManager reference to PointerEvent::create()
> methods so that the id mapping invisible to EventHandler?
> 
> Am I right that in that case, users of PointerEvent would know nothing about
> mapped ids, and therefore, would pass only raw int/unsigned ids to
> PointerIdManager methods? PointerEvent will be the only user of mapped ids
then.

Then maybe in this case we can change PointerIdManager to PointerEventManager
and the new class will play the role of a factory class for PointerEvent class.
So all the create methods from PointerEvent will be moved to PointerEventManager
and then this guarantees nobody can create PointerEvent directly and everybody
goes through PointerEventManager and this class makes sure all the ids and
everything is unique. Is this a better approach then?

[mustaq, 2015-11-20 21:18:56]

On 2015/11/20 20:32:22, Navid Zolghadr wrote:
>
https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1426643008/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:3670: const
> PointerIdManager::MappedId mappedId =
> m_pointerIdManager.add(PointerIdManager::GeneratedPointer(pointerType,
> pointerId));
> On 2015/11/20 20:13:53, mustaq wrote:
> > This looks good. One concern is that EventHandler now has to worry too much
> > about PE ids w/o really doing anything with it. The state maintained through
> > m_pointerIdManager is unavoidable, but at least GeneratedPointer could be
> hidden
> > inside PointerEvent.
> > 
> > What if you just pass a PointerIdManager reference to PointerEvent::create()
> > methods so that the id mapping invisible to EventHandler?
> > 
> > Am I right that in that case, users of PointerEvent would know nothing about
> > mapped ids, and therefore, would pass only raw int/unsigned ids to
> > PointerIdManager methods? PointerEvent will be the only user of mapped ids
> then.
> 
> Then maybe in this case we can change PointerIdManager to PointerEventManager
> and the new class will play the role of a factory class for PointerEvent
class.
> So all the create methods from PointerEvent will be moved to
PointerEventManager
> and then this guarantees nobody can create PointerEvent directly and everybody
> goes through PointerEventManager and this class makes sure all the ids and
> everything is unique. Is this a better approach then?

Sounds good, thanks.

Maybe it deserves a better name. This is not a event manager. Something around
PointerEventFactory?

[Navid Zolghadr, 2015-11-24 18:51:03]

I pushed a new patchset applying all the comments and the new design of using
PointerEventFactory instead of PointerIdManager. 

There are bot errors for this patch but I can't figure what the problem is. I
can't find anything specific in stdio of the bots. Any idea what is causing this
problem?

[mustaq, 2015-11-24 18:58:17]

On 2015/11/24 18:51:03, Navid Zolghadr wrote:
> I pushed a new patchset applying all the comments and the new design of using
> PointerEventFactory instead of PointerIdManager. 
> 
> There are bot errors for this patch but I can't figure what the problem is. I
> can't find anything specific in stdio of the bots. Any idea what is causing
this
> problem?

The bots failed to apply your patch for mouse-pointer-event-properties.html:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

It was caused by a recent change of mine, sorry.
https://codereview.chromium.org/1458643002

[Navid Zolghadr, 2015-11-24 19:45:33]

Description was changed from

==========
Cleaning up PointerIdManager and add id re-mapping

- Add getPrimaryId and getType APIs
- Add id re-mapping concept to have a guaranteed unique pointer id among
  touch, stylus, and mouse.
- Set mouse id to 1

BUG=546658
==========

to

==========
Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEvevntFactory
- Add id re-mapping concept to have a guaranteed unique pointer id among touch,
stylus, and mouse.
- Set mouse id to 1

BUG=546658
==========

[Rick Byers, 2015-11-24 20:08:18]

On 2015/11/16 19:24:44, Navid Zolghadr wrote:
> I tested with the Surface. It matches the current implementation more or less.
> The pointer events that mouse fires always have id=1. Those that are coming
from
> touch or pen will have an increasing number. However, the number only resets
> when I restart the Surface and the count seems to be increased every time I
> touch the surface regardless of whether it is in the browser or not. So the
> minimum number I could get the browser to print as pointer id while touching
the
> div in the test html was 6. I guess logging it and opening IE took a few
> touches.
> 
> I couldn't test with Safari on the iPad I found in the library. It just
doesn't
> print any events as oppose to the other platforms I tested with (i.e. Windows
> 10/IE, Chrome OS/Chrome, Linux/Chrome). Is there any trick to it?

Safari doesn't support pointer events.  But it does support touch events, which
have their own ID (eg. use rbyers.net/eventTest.html).

> Having what Surface does for the pointer event ids which looks the same as
what
> I did here in this patch, I'm not sure how we can match that as well as touch
> ids that will be reset to 0 every time all fingers are off the screen (like
what
> Chrome on Chrome OS or maybe Safari on mobile does).

Right I'm suggesting changing having our touch IDs reset to 0 every time. 
Safari does NOT do this, it's monotonically increasing just like what you're
doing for pointer events.  So if we're doing this work for pointer, we perhaps
might as well share the same logic and do the same for touch so that we
(finally) match Safari in this regard...

[Navid Zolghadr, 2015-11-24 20:27:42]

On 2015/11/24 20:08:18, Rick Byers wrote:
> On 2015/11/16 19:24:44, Navid Zolghadr wrote:
> > I tested with the Surface. It matches the current implementation more or
less.
> > The pointer events that mouse fires always have id=1. Those that are coming
> from
> > touch or pen will have an increasing number. However, the number only resets
> > when I restart the Surface and the count seems to be increased every time I
> > touch the surface regardless of whether it is in the browser or not. So the
> > minimum number I could get the browser to print as pointer id while touching
> the
> > div in the test html was 6. I guess logging it and opening IE took a few
> > touches.
> > 
> > I couldn't test with Safari on the iPad I found in the library. It just
> doesn't
> > print any events as oppose to the other platforms I tested with (i.e.
Windows
> > 10/IE, Chrome OS/Chrome, Linux/Chrome). Is there any trick to it?
> 
> Safari doesn't support pointer events.  But it does support touch events,
which
> have their own ID (eg. use rbyers.net/eventTest.html).
> 
> > Having what Surface does for the pointer event ids which looks the same as
> what
> > I did here in this patch, I'm not sure how we can match that as well as
touch
> > ids that will be reset to 0 every time all fingers are off the screen (like
> what
> > Chrome on Chrome OS or maybe Safari on mobile does).
> 
> Right I'm suggesting changing having our touch IDs reset to 0 every time. 
> Safari does NOT do this, it's monotonically increasing just like what you're
> doing for pointer events.  So if we're doing this work for pointer, we perhaps
> might as well share the same logic and do the same for touch so that we
> (finally) match Safari in this regard...

I assume you don't want the touch ID change to be part of this very change. So
we should be able to get to it afterwards. Right?

[mustaq, 2015-11-25 21:43:40]

EventHandler is much cleaner now, thanks to your simple create() interface.
There are a few comments on simplifying the factory internals.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:33: // Mouse id is
1 to behave the same as MS Edge for compatibility reasons.
Please add an empty line above the comment to clarify its "target".

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:44:
pointerEventInit.setPointerType(pointerTypeNameForWebPointPointerType(WebPointerProperties::PointerType::Mouse));
Please use mouseEvent.pointerProperties().pointerType() instead of hardcoding
::Mouse here. Blink receives stylus events as PlatformMouseEvents in ChromeOS.

Same for the id above: use s_mouseId only if type==Mouse.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:56: 
Too many empty lines?

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:187: bool
PointerEventFactory::isPrimary(PointerEventFactory::MappedId mappedId) const
Let's nuke isPrimary(), getPrimaryId() and getType(), it doesn't make sense to
complicate the code only for a predicted use case. As we have discussed offline,
we can bring them back if needed. If we ever do that, I don't believe the mapped
id will need to be exposed; instead the functions will take (type,rawId) as
parameter/return value. I will comment on the bug accordingly.

As a result, you can get rid of reverseMapping. Also, m_ids won't need to be an
array, just a (type,rawId)=>generatedId mapping. The special checks for
type=Mouse will be limited to your add() and remove(), so the code will be much
cleaner.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:55: typedef
std::pair<WebPointerProperties::PointerType, int> GeneratedPointer;
GeneratedPointer confused me several times here.  s/GeneratedPointer/IncomingId?

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right):

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:24: class
PlatformTouchPointBuilder : public PlatformTouchPoint {
Please move the inner class def to the end of the parent class so that parent
class members are all together.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:29: unsigned
m_expectedMouseId;
private? const?

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:49: int uid,
bool isPrimary)
Please clarify the names id vs uid. I would prefer rawId vs
generatedId/uniqueId.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:58:
PassRefPtrWillBeRawPtr<PointerEvent>
PointerEventFactoryTest::createAndCheckMouseEvent(int uid, bool isPrimary)
- Include pointerType in parameter, and add a test below for PlatformMouseEvents
of type Pen.
- Make the name uid same as suggested for the above function.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:89: 
Can remove the last empty line in the function. Or at least be consistent.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:103:
m_pointerEventFactory.clear();
Great coverage of possible cases, thanks.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:123:
RefPtrWillBeRawPtr<PointerEvent> pointerEvent6 =
createAndCheckTouchEvent(WebPointerProperties::PointerType::Touch, 0,
m_mappedIdStart+1, true);
Shouldn't we expect m_mappedIdStart+2 here?

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:142:
createAndCheckTouchEvent(WebPointerProperties::PointerType::Touch, 100,
m_mappedIdStart+5, true);
Please do a similar test for a new "primary" of type Pen. Remember to create
those PEs through both PlatformMouse & PlatformTouch events.

[Navid Zolghadr, 2015-11-25 22:22:59]

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:33: // Mouse id is
1 to behave the same as MS Edge for compatibility reasons.
On 2015/11/25 21:43:39, mustaq wrote:
> Please add an empty line above the comment to clarify its "target".

Done.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:44:
pointerEventInit.setPointerType(pointerTypeNameForWebPointPointerType(WebPointerProperties::PointerType::Mouse));
On 2015/11/25 21:43:39, mustaq wrote:
> Please use mouseEvent.pointerProperties().pointerType() instead of hardcoding
> ::Mouse here. Blink receives stylus events as PlatformMouseEvents in ChromeOS.
> 
> Same for the id above: use s_mouseId only if type==Mouse.

I guess we need to further look into the stylus as mouse events and see how we
want to treat them. If they come as MouseEvents I guess they will not have ids.
So we consider that there will be only one stylus at a time just like mouse?
Maybe we can look into that together tomorrow.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:56: 
On 2015/11/25 21:43:39, mustaq wrote:
> Too many empty lines?

Done.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:187: bool
PointerEventFactory::isPrimary(PointerEventFactory::MappedId mappedId) const
On 2015/11/25 21:43:39, mustaq wrote:
> Let's nuke isPrimary(), getPrimaryId() and getType(), it doesn't make sense to
> complicate the code only for a predicted use case. As we have discussed
offline,
> we can bring them back if needed. If we ever do that, I don't believe the
mapped
> id will need to be exposed; instead the functions will take (type,rawId) as
> parameter/return value. I will comment on the bug accordingly.
> 
> As a result, you can get rid of reverseMapping. Also, m_ids won't need to be
an
> array, just a (type,rawId)=>generatedId mapping. The special checks for
> type=Mouse will be limited to your add() and remove(), so the code will be
much
> cleaner.
> 
> 

Reverse mapping will be use particurarly for the remove api. I'll be looking
into changing m_ids to one hashmap instead of an array to see what I can do
there.

Functions like isPrimary is used in the create functions of this class to find
out whether the generate id is primary to set the attribute of pointerEvent. But
surely I can get rid of getType and getPrimaryId to make the class simpler.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:55: typedef
std::pair<WebPointerProperties::PointerType, int> GeneratedPointer;
On 2015/11/25 21:43:39, mustaq wrote:
> GeneratedPointer confused me several times here. 
s/GeneratedPointer/IncomingId?

Sure. I'll do that.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right):

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:49: int uid,
bool isPrimary)
On 2015/11/25 21:43:40, mustaq wrote:
> Please clarify the names id vs uid. I would prefer rawId vs
> generatedId/uniqueId.

Done.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:58:
PassRefPtrWillBeRawPtr<PointerEvent>
PointerEventFactoryTest::createAndCheckMouseEvent(int uid, bool isPrimary)
On 2015/11/25 21:43:40, mustaq wrote:
> - Include pointerType in parameter, and add a test below for
PlatformMouseEvents
> of type Pen.
> - Make the name uid same as suggested for the above function.

Done.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:89: 
On 2015/11/25 21:43:40, mustaq wrote:
> Can remove the last empty line in the function. Or at least be consistent.

I was copying it from the other test class. I thought there has to be an empty
line at the end of TEST_F functions. That's why I have it at the end of all
those here :).

I guess the other test class was just adding it not on purpose then. So I just
remove it from all the functions here.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:123:
RefPtrWillBeRawPtr<PointerEvent> pointerEvent6 =
createAndCheckTouchEvent(WebPointerProperties::PointerType::Touch, 0,
m_mappedIdStart+1, true);
On 2015/11/25 21:43:40, mustaq wrote:
> Shouldn't we expect m_mappedIdStart+2 here?

I'll add a comment to explain what happened here. I actually did not remove the
new pointer event in the previous line. Just removed an obsolete (already
removed) pointer event again and just wanted to make sure it doesn't affect the
behavior.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:142:
createAndCheckTouchEvent(WebPointerProperties::PointerType::Touch, 100,
m_mappedIdStart+5, true);
On 2015/11/25 21:43:40, mustaq wrote:
> Please do a similar test for a new "primary" of type Pen. Remember to create
> those PEs through both PlatformMouse & PlatformTouch events.

Done.

[Navid Zolghadr, 2015-11-26 20:15:10]

I missed 2 of the previous comments before.

Also to be able to see what we really do on Chrome OS with pen events and
whether they have ids or not I need to be able to test this patch on it. Is that
possible? Do you have a wiki or something to explain how to load this on Chrome
OS?

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right):

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:24: class
PlatformTouchPointBuilder : public PlatformTouchPoint {
On 2015/11/25 21:43:40, mustaq wrote:
> Please move the inner class def to the end of the parent class so that parent
> class members are all together.

Sure.

https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:29: unsigned
m_expectedMouseId;
On 2015/11/25 21:43:40, mustaq wrote:
> private? const?

None of them is possible. As initializing the variables in SetUp function needs
them to be non-const. Also if I make anything private they cannot be accessed
inside TEST_F macros.

[Rick Byers, 2015-11-27 02:04:46]

On 2015/11/24 20:27:42, Navid Zolghadr wrote:
> On 2015/11/24 20:08:18, Rick Byers wrote:
> > On 2015/11/16 19:24:44, Navid Zolghadr wrote:
> > > I tested with the Surface. It matches the current implementation more or
> less.
> > > The pointer events that mouse fires always have id=1. Those that are
coming
> > from
> > > touch or pen will have an increasing number. However, the number only
resets
> > > when I restart the Surface and the count seems to be increased every time
I
> > > touch the surface regardless of whether it is in the browser or not. So
the
> > > minimum number I could get the browser to print as pointer id while
touching
> > the
> > > div in the test html was 6. I guess logging it and opening IE took a few
> > > touches.
> > > 
> > > I couldn't test with Safari on the iPad I found in the library. It just
> > doesn't
> > > print any events as oppose to the other platforms I tested with (i.e.
> Windows
> > > 10/IE, Chrome OS/Chrome, Linux/Chrome). Is there any trick to it?
> > 
> > Safari doesn't support pointer events.  But it does support touch events,
> which
> > have their own ID (eg. use rbyers.net/eventTest.html).
> > 
> > > Having what Surface does for the pointer event ids which looks the same as
> > what
> > > I did here in this patch, I'm not sure how we can match that as well as
> touch
> > > ids that will be reset to 0 every time all fingers are off the screen
(like
> > what
> > > Chrome on Chrome OS or maybe Safari on mobile does).
> > 
> > Right I'm suggesting changing having our touch IDs reset to 0 every time. 
> > Safari does NOT do this, it's monotonically increasing just like what you're
> > doing for pointer events.  So if we're doing this work for pointer, we
perhaps
> > might as well share the same logic and do the same for touch so that we
> > (finally) match Safari in this regard...
> 
> I assume you don't want the touch ID change to be part of this very change. So
> we should be able to get to it afterwards. Right?

Yeah probably best to keep it to a separate CL (unless it means you'd be wasting
any time doing throwaway work / testing).

[Rick Byers, 2015-11-27 02:10:33]

On 2015/11/26 20:15:10, Navid Zolghadr wrote:
> I missed 2 of the previous comments before.
> 
> Also to be able to see what we really do on Chrome OS with pen events and
> whether they have ids or not I need to be able to test this patch on it. Is
that
> possible? Do you have a wiki or something to explain how to load this on
Chrome
> OS?

It's been awhile since I've done it, but I used to use this process all the
time:
http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro...

That said IIRC adlr@ told me that at least with the Wacom device we have, the
hardware (or driver) appears to support only a single stylus at a time.  I guess
you could try getting a second device and plugging both in.  But Personally I
think it's fine to limit our support to a single stylus pointer for now, we can
always add multi-stylus support later if desired.  Or if there's evidence that
the code is wired up for WebMouseEvent.id then it wouldn't hurt to just try
using that and verifying it works properly in the single pointer case.

>
https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp
(right):
> 
>
https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:24: class
> PlatformTouchPointBuilder : public PlatformTouchPoint {
> On 2015/11/25 21:43:40, mustaq wrote:
> > Please move the inner class def to the end of the parent class so that
parent
> > class members are all together.
> 
> Sure.
> 
>
https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:29: unsigned
> m_expectedMouseId;
> On 2015/11/25 21:43:40, mustaq wrote:
> > private? const?
> 
> None of them is possible. As initializing the variables in SetUp function
needs
> them to be non-const. Also if I make anything private they cannot be accessed
> inside TEST_F macros.

[mustaq, 2015-11-27 16:51:49]

On 2015/11/27 02:10:33, Rick Byers wrote:
> On 2015/11/26 20:15:10, Navid Zolghadr wrote:
> > I missed 2 of the previous comments before.
> > 
> > Also to be able to see what we really do on Chrome OS with pen events and
> > whether they have ids or not I need to be able to test this patch on it. Is
> that
> > possible? Do you have a wiki or something to explain how to load this on
> Chrome
> > OS?
> 
> It's been awhile since I've done it, but I used to use this process all the
> time:
>
http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro...
> 
> That said IIRC adlr@ told me that at least with the Wacom device we have, the
> hardware (or driver) appears to support only a single stylus at a time.  I
guess
> you could try getting a second device and plugging both in.  But Personally I
> think it's fine to limit our support to a single stylus pointer for now, we
can
> always add multi-stylus support later if desired.  Or if there's evidence that
> the code is wired up for WebMouseEvent.id then it wouldn't hurt to just try
> using that and verifying it works properly in the single pointer case.
> 
> >
>
https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:24: class
> > PlatformTouchPointBuilder : public PlatformTouchPoint {
> > On 2015/11/25 21:43:40, mustaq wrote:
> > > Please move the inner class def to the end of the parent class so that
> parent
> > > class members are all together.
> > 
> > Sure.
> > 
> >
>
https://codereview.chromium.org/1426643008/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:29:
unsigned
> > m_expectedMouseId;
> > On 2015/11/25 21:43:40, mustaq wrote:
> > > private? const?
> > 
> > None of them is possible. As initializing the variables in SetUp function
> needs
> > them to be non-const. Also if I make anything private they cannot be
accessed
> > inside TEST_F macros.

Agreed. Let's go ahead here when the PointerEventFactory interface is general
enough to let us switch to whichever id mapping works out for us. For id-mapping
in the factory, let's stick to the assumption that there is only one stylus, so:
  id=1 for mouse, id=2 for stylus, id=3... for touches.

[Navid Zolghadr, 2015-12-01 19:07:29]

ptal

I applied all the comments and tested again with pen on Chromebook. To the
extent that is related to this class everything worked well. Although I saw a
few other problems with parameters beings passed to the APIs of this class which
I'll file bugs for them separately and address them in different CLs.

[mustaq, 2015-12-02 15:53:46]

Have a few quick nits.

https://codereview.chromium.org/1426643008/diff/140001/components/test_runner...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1426643008/diff/140001/components/test_runner...
components/test_runner/event_sender.cc:96: e->pointerType =
blink::WebPointerProperties::PointerType::Mouse;
Please add a line in CL description about this fix.

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:47:
pointerEventInit.setPointerId(pointerId);
In all create*() methods, the 4 lines dealing with MappedId is exactly the same,
looks like sort of an "atomic" id-related setter op. I suggest packing them in a
helper function, to help avoiding missed steps in future changes.

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:129: :
m_currentId(PointerEventFactory::s_mouseId+1)
Perhaps we should set m_currentId in clear().

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right):

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:50: :
PlatformTouchPoint()
This call could be implicit I guess?

[Navid Zolghadr, 2015-12-02 16:24:40]

Description was changed from

==========
Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEvevntFactory
- Add id re-mapping concept to have a guaranteed unique pointer id among touch,
stylus, and mouse.
- Set mouse id to 1

BUG=546658
==========

to

==========
Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEventFactory
- Add id re-mapping concept to have guaranteed unique pointer ids among
  touch, stylus, and mouse pointer types.
- Set mouse id to 1
- Set the mouse pointer type in event sender

BUG=546658
==========

[Navid Zolghadr, 2015-12-02 16:49:08]

ptal

https://codereview.chromium.org/1426643008/diff/140001/components/test_runner...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1426643008/diff/140001/components/test_runner...
components/test_runner/event_sender.cc:96: e->pointerType =
blink::WebPointerProperties::PointerType::Mouse;
On 2015/12/02 15:53:46, mustaq wrote:
> Please add a line in CL description about this fix.

Done.

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:47:
pointerEventInit.setPointerId(pointerId);
On 2015/12/02 15:53:46, mustaq wrote:
> In all create*() methods, the 4 lines dealing with MappedId is exactly the
same,
> looks like sort of an "atomic" id-related setter op. I suggest packing them in
a
> helper function, to help avoiding missed steps in future changes.

Done.

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:129: :
m_currentId(PointerEventFactory::s_mouseId+1)
On 2015/12/02 15:53:46, mustaq wrote:
> Perhaps we should set m_currentId in clear().

Done.

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right):

https://codereview.chromium.org/1426643008/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:50: :
PlatformTouchPoint()
On 2015/12/02 15:53:46, mustaq wrote:
> This call could be implicit I guess?

Done.

[mustaq, 2015-12-02 18:35:59]

Thanks for the clean interface. LGTM. Please close the bug after CQing.

[Rick Byers, 2015-12-02 18:55:59]

LGTM (for OWNERS)

[Navid Zolghadr, 2015-12-02 19:48:30]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2015-12-02 19:48:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/1426643008/#ps200001
(title: "Fix EXPECT_EQ compilation error on Android")

[commit-bot: I haz the power, 2015-12-02 19:48:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1426643008/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1426643008/200001

[commit-bot: I haz the power, 2015-12-02 20:07:01]

Description was changed from

==========
Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEventFactory
- Add id re-mapping concept to have guaranteed unique pointer ids among
  touch, stylus, and mouse pointer types.
- Set mouse id to 1
- Set the mouse pointer type in event sender

BUG=546658
==========

to

==========
Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEventFactory
- Add id re-mapping concept to have guaranteed unique pointer ids among
  touch, stylus, and mouse pointer types.
- Set mouse id to 1
- Set the mouse pointer type in event sender

BUG=546658
==========

[commit-bot: I haz the power, 2015-12-02 20:07:02]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-12-02 20:08:22]

Description was changed from

==========
Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEventFactory
- Add id re-mapping concept to have guaranteed unique pointer ids among
  touch, stylus, and mouse pointer types.
- Set mouse id to 1
- Set the mouse pointer type in event sender

BUG=546658
==========

to

==========
Cleaning up PointerIdManager and changing it to PointerEventFactory

- Move some create methods of PointerEvent to PointerEventFactory
- Add id re-mapping concept to have guaranteed unique pointer ids among
  touch, stylus, and mouse pointer types.
- Set mouse id to 1
- Set the mouse pointer type in event sender

BUG=546658

Committed: https://crrev.com/06d3f72718785fea41c1e4deffe330afb5e24841
Cr-Commit-Position: refs/heads/master@{#362779}
==========

[commit-bot: I haz the power, 2015-12-02 20:08:23]

Patchset 11 (id:??) landed as
https://crrev.com/06d3f72718785fea41c1e4deffe330afb5e24841
Cr-Commit-Position: refs/heads/master@{#362779}