[webkit-reviews] review denied: [Bug 35691] [Chromium] Adding new platform types for touch events and touch points. : [Attachment 49923] Removed extraneous include

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Garret Kelly
(Google) <gdk at chromium.org>'s request for review:
Bug 35691: [Chromium] Adding new platform types for touch events and touch
points.
https://bugs.webkit.org/show_bug.cgi?id=35691

Attachment 49923: Removed extraneous include
https://bugs.webkit.org/attachment.cgi?id=49923&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebKit/chromium/public/WebInputEvent.h
>  
> +// WebTouchEvent
--------------------------------------------------------------
> +
> +class WebTouchEvent : public WebInputEvent {
> +public:
> +    static const int touchPointCountCap = 4;

nit: CountCap -> LengthCap for consistency with WebKeyboardEvent


> +
> +    int modifiers;
> +    int touchPointCount;
> +    WebTouchPoint touchPoints[touchPointCountCap];

Is touchPointCount necessary?  Perhaps you could use
touchPoints[i].state == WebTouchPoint::Undefined as
an array terminator instead?

WebInputEvent already defines a modifiers member var,
so I'm surprised to see it repeated here.


> +++ b/WebKit/chromium/public/WebTouchPoint.h
...
> +#include <string.h>

^^^ this include seems unnecessary


> +namespace WebKit {
> +
> +class WebTouchPoint {
> +public:
> +    WebTouchPoint()
> +	   : id(0)
> +	   , state(Undefined)
> +	   , screenPosition(0, 0)
> +	   , position(0, 0) { }
> +
> +    enum State {
> +	   Undefined,
> +	   TouchReleased,
> +	   TouchPressed,
> +	   TouchMoved,
> +	   TouchStationary,
> +	   TouchCancelled,

How about StateReleased, StatePressed, etc. to better
match the name of the enum?

Since these enum values will primarily be written w/
the WebTouchPoint prefix, it seems unnecessary to have
the word "Touch" in the names of the enum values.