Summary: | [chromium] Make sure the touch-points in the touch-events have the correct state. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sadrul Habib Chowdhury <sadrul> | ||||||||
Component: | New Bugs | Assignee: | Sadrul Habib Chowdhury <sadrul> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, rjkroege, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Sadrul Habib Chowdhury
2012-10-01 21:27:43 PDT
Created attachment 166600 [details]
Patch
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! 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! Created attachment 166622 [details]
Patch
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 Created attachment 166703 [details]
Patch
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. Comment on attachment 166703 [details]
Patch
Thanks!
Comment on attachment 166703 [details] Patch Clearing flags on attachment: 166703 Committed r130197: <http://trac.webkit.org/changeset/130197> All reviewed patches have been landed. Closing bug. |