Touch.identifier should be long, not unsigned long

Touch.identifier should be long, not unsigned long

BUG=464708
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196881

[ramya.v, 2015-06-08 11:28:47]

PTAL!

Thanks

[ramya.v, 2015-06-09 05:28:42]

ramya.v@samsung.com changed reviewers:
+ philipj@opera.com, rbyers@chromium.org

[ramya.v, 2015-06-09 05:28:42]

PTAL!

Thanks

[Rick Byers, 2015-06-09 16:04:37]

rbyers@chromium.org changed reviewers:
- philipj@opera.com

[Rick Byers, 2015-06-09 16:04:37]

Thanks!  I'll review this (moving Philip to cc).

https://codereview.chromium.org/1170703003/diff/40001/Source/core/dom/Touch.h
File Source/core/dom/Touch.h (right):

https://codereview.chromium.org/1170703003/diff/40001/Source/core/dom/Touch.h...
Source/core/dom/Touch.h:48: long identifier, const FloatPoint& screenPos, const
FloatPoint& pagePos,
On some platforms (64-bit linux I think) C++ 'long' is 64-bits.  But in IDL
'long' is always 32 bits.  So when using 'long' in IDL, please use 'int' in C++.

https://codereview.chromium.org/1170703003/diff/40001/Source/core/dom/Touch.h...
Source/core/dom/Touch.h:48: long identifier, const FloatPoint& screenPos, const
FloatPoint& pagePos,
Please check all callers of this method for signed/unsigned mismatch too.  Eg.
the main place in EventHandler::handleTouchEvent passes an unsigned here (I
think that should generate a warning - at least in some build configs?). 
Happily it looks like we can also just change PlatformTouchPoint::id to be 'int'
as well.  The API from Chromium (WebTouchPoint) is already using 'int' so this
will also fix the signed->unsigned conversion that was happening from there.

[Rick Byers, 2015-06-09 16:06:04]

BTW this would have potential compat implications if we ever actually used large
values.  But unlike Safari (IIRC), chromium always uses small integers on all
platforms - so I don't think we need to worry about getting negative values in
practice.

[Rick Byers, 2015-06-09 16:07:56]

Also /cc mustaq since this should be (at least very slightly) helpful for
pointer events.  PointerEvent.pointerId is also defined to be a 'long', so we
can directly copy the PlatformTouchPoint ID without a signed/unsigned mismatch.

[ramya.v, 2015-06-10 07:13:45]

Updated as per review comments.

PTAL!

https://codereview.chromium.org/1170703003/diff/40001/Source/core/dom/Touch.h
File Source/core/dom/Touch.h (right):

https://codereview.chromium.org/1170703003/diff/40001/Source/core/dom/Touch.h...
Source/core/dom/Touch.h:48: long identifier, const FloatPoint& screenPos, const
FloatPoint& pagePos,
On 2015/06/09 16:04:37, Rick Byers wrote:
> On some platforms (64-bit linux I think) C++ 'long' is 64-bits.  But in IDL
> 'long' is always 32 bits.  So when using 'long' in IDL, please use 'int' in
C++.

Done.

https://codereview.chromium.org/1170703003/diff/40001/Source/core/dom/Touch.h...
Source/core/dom/Touch.h:48: long identifier, const FloatPoint& screenPos, const
FloatPoint& pagePos,
On 2015/06/09 16:04:37, Rick Byers wrote:
> Please check all callers of this method for signed/unsigned mismatch too.  Eg.
> the main place in EventHandler::handleTouchEvent passes an unsigned here (I
> think that should generate a warning - at least in some build configs?). 
> Happily it looks like we can also just change PlatformTouchPoint::id to be
'int'
> as well.  The API from Chromium (WebTouchPoint) is already using 'int' so this
> will also fix the signed->unsigned conversion that was happening from there.

Done.

[Rick Byers, 2015-06-10 15:34:21]

LGTM

https://codereview.chromium.org/1170703003/diff/60001/Source/core/inspector/I...
File Source/core/inspector/InspectorInputAgent.cpp (right):

https://codereview.chromium.org/1170703003/diff/60001/Source/core/inspector/I...
Source/core/inspector/InspectorInputAgent.cpp:53:
SyntheticInspectorTouchPoint(int id, State state, const blink::IntPoint&
screenPos, const blink::IntPoint& pos, int radiusX, int radiusY, double
rotationAngle, double force)
Good catch!

[Rick Byers, 2015-06-10 15:35:47]

rbyers@chromium.org changed reviewers:
+ pdr@chromium.org

[Rick Byers, 2015-06-10 15:35:59]

+pdr for Source/platform OWNERS

[pdr., 2015-06-10 17:02:23]

On 2015/06/10 at 15:35:59, rbyers wrote:
> +pdr for Source/platform OWNERS

LGTM

[pdr., 2015-06-10 17:02:26]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-06-10 17:03:14]

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

[commit-bot: I haz the power, 2015-06-10 17:25:39]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196881