Bug 35691 – [Chromium] Adding new platform types for touch events and touch points.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Garret Kelly]

The Chromium WebKit needs the requisite platform types to allow for touch events to be transferred into WebKit, which this feature adds. This is the first of several patches to incorporate touch event support into Chromium.

[Garret Kelly]

Created attachment 49921 [details]
Initial patch

[Garret Kelly]

Created attachment 49923 [details]
Removed extraneous include

[Darin Fisher (:fishd, Google)]

Comment on attachment 49923 [details]
Removed extraneous include

> +++ 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.

[Garret Kelly]

(In reply to comment #3)
> (From update of attachment 49923 [details])
> > +++ b/WebKit/chromium/public/WebInputEvent.h
> >  
> > +// WebTouchEvent --------------------------------------------------------------
> > +
> > +class WebTouchEvent : public WebInputEvent {
> > +public:
> > +    static const int touchPointCountCap = 4;
> 
> nit: CountCap -> LengthCap for consistency with WebKeyboardEvent

Done.

> 
> > +
> > +    int modifiers;
> > +    int touchPointCount;
> > +    WebTouchPoint touchPoints[touchPointCountCap];
> 
> Is touchPointCount necessary?  Perhaps you could use
> touchPoints[i].state == WebTouchPoint::Undefined as
> an array terminator instead?

That'd require using up a WebTouchPoint entry in the array just to indicate the total length. For the sake of space it seems better to stick with the touchPointCount.

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

Removed.

> 
> > +++ b/WebKit/chromium/public/WebTouchPoint.h
> ...
> > +#include <string.h>
> 
> ^^^ this include seems unnecessary

Entirely. I'm baffled as to why I put that there. Removed. :)

> 
> > +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.

Very sensible. Done. Also renamed Undefined -> StateUndefined.

Thank you very much for the comments. :)

[Garret Kelly]

Created attachment 49942 [details]
Review fixes

[Darin Fisher (:fishd, Google)]

What it the use case for StateUndefined if not to terminate the array?  Can you have a WebTouchPoint that is undefined and have the other fields contain meaningful data?  It is OK to have the terminator be optional.  All fields could contain data, or some of them could contain StateUndefined to indicate that they are unused.  Maybe that is less clear though.

[Garret Kelly]

Created attachment 49958 [details]
Second-round review fixes

[Darin Fisher (:fishd, Google)]

Comment on attachment 49958 [details]
Second-round review fixes

> +class WebTouchPoint {
> +public:
> +    WebTouchPoint()
> +        : id(0)
> +        , state(StateUndefined)
> +        , screenPosition(0, 0)
> +        , position(0, 0) { }

minor nit: no need to initialize a WebPoint explicitly.  just let
the default constructor do its thing.

otherwise, r=me

[Garret Kelly]

Created attachment 49959 [details]
Third-round review fixes

[WebKit Commit Bot]

Comment on attachment 49959 [details]
Third-round review fixes

Clearing flags on attachment: 49959

Committed r55507: <http://trac.webkit.org/changeset/55507>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.