Bug 32484

Someone is working on this bug?

Created attachment 92783 [details] Test case Test case

Created attachment 92787 [details] Patch proposed patch

Comment on attachment 92787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92787&action=review > Source/WebCore/ChangeLog:8 > + [Qt] Add support for scale and rotation properties to TouchEvent. Scale property Why double Qt here? You don't need to paste again the title of the bug. > Source/WebCore/ChangeLog:9 > + gives the multiplier the user has pinched in the gesture and rotation gives the "Scale property gives the value of the multiplier used when the user pinched." Something like that.

Could you look benjamin? I've heard some really really unsure rumors that you are working on Touch related stuff :D.

Created attachment 92788 [details] Patch Proposed patch. update changelog.

Created attachment 92867 [details] patch Proposed patch. add test.

Comment on attachment 92867 [details] patch Attachment 92867 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8648718 New failing tests: fast/events/touch/touch-scale-rotation.html

Created attachment 92899 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 92906 [details] patch Proposed patch. skip test from chromium.

Comment on attachment 92906 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=92906&action=review > LayoutTests/ChangeLog:5 > + [Qt] Add support for scale and transformation properties to TouchEvent This change is not Qt specific is it?

(In reply to comment #11) > (From update of attachment 92906 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92906&action=review > > > LayoutTests/ChangeLog:5 > > + [Qt] Add support for scale and transformation properties to TouchEvent > > This change is not Qt specific is it? No, but the initial implementation is done in Qt side. I will update the patch Changelog.

Created attachment 92986 [details] patch Proposed patch.

Comment on attachment 92986 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=92986&action=review > LayoutTests/ChangeLog:15 > + * fast/events/touch/touch-scale-rotation-expected.txt: Added. > + * fast/events/touch/touch-scale-rotation.html: Added. This only tests the forward/valid case. You should also test the values for the cases: -more than 2 fingers -only one finger -boundaries limits (like two very close fingers) > Source/WebCore/platform/PlatformTouchEvent.h:67 > + , m_scale(1) > + , m_rotationAngle(0) You should use explicit floats for the literals, otherwise you might get a warning for the conversion int->float. > Source/WebCore/platform/PlatformTouchEvent.h:102 > + float m_scale; > + float m_rotationAngle; Don't you need to also modify the WebKit 2 parts to serialize those? > Source/WebCore/platform/qt/PlatformTouchEventQt.cpp:33 > +static qreal handleRotationTouchEvent(const QList<QTouchEvent::TouchPoint>& points) I don't like handleRotationTouchEvent() and handleScaleTouchEvent() because they rely on startPos(). This only works if the framework above only accept one gesture at the time: -pan -lift your finger -pinch -lift your fingers -etc. In reality, most devs want to support the gesture following eachothers: -pan -keep fingers on screen -pinch -lift just one finger -continue panning. So the computation of handleRotationTouchEvent() and handleScaleTouchEvent() need to be done from the last gesture started. Gesture recognizer generally also have threshold, which needs to be taken into account. > Source/WebCore/platform/qt/PlatformTouchEventQt.cpp:57 > + / QLineF(firstPoint.startPos(), lastPoint.startPos()).length(); Splitting this line like this is a sign you should use temporary variables to make this operation clearer :)

Robert, are you interested in supporting this feature in Chromium when it lands? If not, we should #ifdef the IDL, otherwise you would expose something that never returns meaningful values.

(In reply to comment #15) > Robert, are you interested in supporting this feature in Chromium when it lands? If not, we should #ifdef the IDL, otherwise you would expose something that never returns meaningful values. Benjamin: thanks for the heads-up. Please (at a minimum) #ifdef the IDL. It will be quite difficult to correctly modify the Chrome gesture recognizer to add the correct scale and transform to the Touch event. Also: maybe these fields should have some sort of prefix given their exclusion from http://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html? Aside: I feel that the "right" thing is what Simon said at the start: scale and transformation don't belong in the TouchEvent. Instead, we should have higher-level semantic gesture events yes?

(In reply to comment #16) > Aside: I feel that the "right" thing is what Simon said at the start: scale and transformation don't belong in the TouchEvent. Instead, we should have higher-level semantic gesture events yes? I don't really like those properties either. But I see how they make it easier to write JavaScript on mobile. iOS is really the de facto standard there, with many apps designed just for it. I understand why people want this feature.

(In reply to comment #17) > (In reply to comment #16) > > Aside: I feel that the "right" thing is what Simon said at the start: scale and transformation don't belong in the TouchEvent. Instead, we should have higher-level semantic gesture events yes? > > I don't really like those properties either. But I see how they make it easier to write JavaScript on mobile. > > iOS is really the de facto standard there, with many apps designed just for it. I understand why people want this feature. I agree with your reasoning. The patch is worthwhile on this basis. I added https://bugs.webkit.org/show_bug.cgi?id=61550 to track a matching update for chrome.

=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.