[webkit-reviews] review denied: [Bug 66800] [Chromium] Modify WebTouchEvent structure to match WebCore::TouchEvent : [Attachment 104998] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied
borenet at gmail.com's request for review:
Bug 66800: [Chromium] Modify WebTouchEvent structure to match
WebCore::TouchEvent
https://bugs.webkit.org/show_bug.cgi?id=66800

Attachment 104998: Patch
https://bugs.webkit.org/attachment.cgi?id=104998&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104998&action=review


> Source/WebKit/chromium/public/WebInputEvent.h:347
> +    static const unsigned int touchesLengthCap = 8;

nit: we use 'unsigned' instead of 'unsigned int' in the WebKit API (just like
the rest of WebKit).

> Source/WebKit/chromium/public/WebInputEvent.h:349
> +    unsigned int touchesLength;

it seems like it would be helpful to define the difference between
touches, changedTouches and targetTouches.

> Source/WebKit/chromium/public/WebTouchPoint.h:42
> +	   : id(0)

What values can "id" have if not one of the pre-defined Finger enum values?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:53
> +#define MILLIS_PER_SECOND 1000.0

please use a C++ constant

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:269
> +    for (unsigned int i = 0; i < event.touchesLength; ++i)

nit: 'unsigned int' -> 'unsigned'

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:395
> +void AddTouchPoints(TouchList* touches, WebTouchPoint* touchPoints,

nit: it is more common place in WebKit to just use the 'static' keyword here.

can touches be declared as const?  'const TouchList*'?
or, can it be a 'const TouchList&' instead?  it doesn't look like this function

mutates the TouchList at all, so it should not be passed as a non-const
pointer/
reference.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:396
> +		       unsigned int* touchPointsLength, WebCore::IntPoint
offset) {

nit: no need for WebCore:: prefix

it is usually better to list out parameters last and in parameters first.  i
would
therefore move offset before touchPoints.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:404
> +	       touch->pageX() - offset.x(),

what is the offset used for?  does this represent scroll offset of the frame? 
if so,
does this work properly with iframes?  we normally need to use one of the
conversion
routines like convertToContainingWindow to map from widget coordinates and
viewport
coordinates.

> Source/WebKit/chromium/src/WebInputEventConversion.h:130
> +    WebTouchEventBuilder(const WebCore::Widget*, const
WebCore::TouchEvent&);

no need for WebCore:: prefix