[Webkit-unassigned] [Bug 66800] [Chromium] Modify WebTouchEvent structure to match WebCore::TouchEvent

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66800


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #104998|review?                     |review-
               Flag|                            |




--- Comment #14 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-08-24 14:00:45 PST ---
(From update of attachment 104998)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.