[webkit-reviews] review denied: [Bug 98110] [chromium] Make sure the touch-points in the touch-events have the correct state. : [Attachment 166600] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied sadrul at chromium.org's request for
review:
Bug 98110: [chromium] Make sure the touch-points in the touch-events have the
correct state.
https://bugs.webkit.org/show_bug.cgi?id=98110

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166600&action=review


> Source/WebKit/chromium/src/WebInputEventConversion.cpp:339
> +static inline WebTouchPoint::State toWebTouchPointState(const AtomicString&
type)

static and inline are redundant.

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:94
> +    switch (state) {
> +    case WebKit::WebTouchPoint::StateReleased:   return
stateReleased.data();
> +    case WebKit::WebTouchPoint::StatePressed:    return statePressed.data();

> +    case WebKit::WebTouchPoint::StateMoved:	    return stateMoved.data();
> +    case WebKit::WebTouchPoint::StateCancelled:  return
stateCancelled.data();
> +    default: 				    break;
> +    }

This switch statement is not in the proper style.

Why are you creating these CStrings?  You can just return pointers to the
string literals.

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:108
> +	   const WebKit::WebTouchEvent& touch = reinterpret_cast<const
WebKit::WebTouchEvent&>(event);

reinterpret_cast -> static_cast

Please read up on the differences between these two kinds of C++ casts,
especially as they related to multi-inheritance.  Using the wrong cast can
result is subtle, dangerous bugs!