Bug 98110 – [chromium] Make sure the touch-points in the touch-events have the correct state.

• Component: New Bugs
• Status: RESOLVED FIXED

[Sadrul Habib Chowdhury]

[chromium] Make sure the touch-points in the touch-events have the correct state.

[Sadrul Habib Chowdhury]

Created attachment 166600 [details]
Patch

[Adam Barth]

Comment on attachment 166600 [details]
Patch

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!

[Sadrul Habib Chowdhury]

Comment on attachment 166600 [details]
Patch

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.

Fixed (also fixed the other cases of the same issue in this file).

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:94
>> +    }
> 
> This switch statement is not in the proper style.
> 
> Why are you creating these CStrings?  You can just return pointers to the string literals.

Fixed.

>> 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!

Whoops. My bad. Fixed. Thanks!

[Sadrul Habib Chowdhury]

Created attachment 166622 [details]
Patch

[Adam Barth]

Comment on attachment 166622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166622&action=review

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:80
> +const char* pointState(WebKit::WebTouchPoint::State state)

This function should probably be static

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:99
> +void printTouchList(const WebKit::WebTouchPoint* points, int length)

ditto

> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:105
> +void printEventDetails(const WebKit::WebInputEvent& event)

ditto

[Sadrul Habib Chowdhury]

Created attachment 166703 [details]
Patch

[Sadrul Habib Chowdhury]

Comment on attachment 166622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166622&action=review

>> Tools/DumpRenderTree/chromium/TestWebPlugin.cpp:80
>> +const char* pointState(WebKit::WebTouchPoint::State state)
> 
> This function should probably be static

My bad. Fixed all cases.

[Adam Barth]

Comment on attachment 166703 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 166703 [details]
Patch

Clearing flags on attachment: 166703

Committed r130197: <http://trac.webkit.org/changeset/130197>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.