The chromium gesture recognizer overloads PlatformGestureEvents with additional semantics that ought to be separated into a separate kind of event per discussion in https://bugs.webkit.org/show_bug.cgi?id=65044.
Created attachment 104384 [details] Patch, not to be committed. Looking for feedback.
Attachment 104384 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/page/EventHandler.cpp', u'S..." exit_code: 1 Source/WebCore/platform/SyntheticGestureEvent.h:79: Extra space between const and FloatSize& [whitespace/declaration] [3] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 105058 [details] Patch
Benjamin: please have a look.
Benjamin: please have a look at the SyntheticGestureEvent related portions. Does this address your objections? Adam: once Benjamin is satisfied: I believe this maintains layering separation. And please have a look at the chromium specific portions once Benjamin is satisfied. Thanks.
Comment on attachment 105058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105058&action=review > Source/WebCore/page/EventHandler.cpp:2247 > + // fp.move(synthetic.delta()); > + // IntPoint point(roundedIntPoint(fp)); We usually frown upon commented out code. > Source/WebCore/page/EventHandler.cpp:2249 > + fprintf(stderr, "EventHandler::handleSyntheticEvent ScrollUpdateType\n\tdelta %f, %f\n", synthetic.delta().width(), synthetic.delta().height()); WebKit has a LOG macro if you'd like to log things. > Source/WebCore/platform/ScrollAnimator.cpp:45 > +// remove > +#include <stdio.h> ?
Comment on attachment 105058 [details] Patch !@* I always forget to proof-read something.
Created attachment 105070 [details] Patch
Comment on attachment 105070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105070&action=review > LayoutTests/ChangeLog:5 > + Remove a layout test obsoleted by removing > + synthetic gesture events from PlatformGestureEvent. > + https://bugs.webkit.org/show_bug.cgi?id=66492 What's wrong with this test? Is there another test for touchdown + touchup = synthetic click? > Source/WebCore/page/EventHandler.cpp:2208 > +// TODO(rjkroege): revert thsi to something more like the previous version To remove :) > Source/WebCore/platform/SyntheticGestureEvent.h:53 > + GestureBeginType, > + GestureChangeType, > + GestueEndType, I suggest to keep those out until the you add the implementation. > Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:111 > + // FIXME: make certain that screen positions are valid. > + gestures->append(SyntheticGestureEvent(SyntheticGestureEvent::TapEndType, FloatPoint(firstTouchPosition()), FloatPoint(firstTouchPosition()), m_lastTouchTime, FloatSize(), m_shiftKey, m_ctrlKey, m_altKey, m_metaKey)); Look like the screen position are not valid here, and that should not be too difficult to fix. I suggest you fix that now instead of getting weird bugs later. > Source/WebKit/chromium/public/WebInputEvent.h:113 > + > + // FIXME: insert WebSyntheticGestureEvent I am not familiar with Chromium, don't you need this to get the feature to work?
Just a few comment. The patch looks good. Lars, could you apply this patch locally and update https://bugs.webkit.org/show_bug.cgi?id=65545 to use SyntheticGestureEvent::TapEndType?
Created attachment 105205 [details] Patch
Comment on attachment 105070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105070&action=review >> LayoutTests/ChangeLog:5 >> + https://bugs.webkit.org/show_bug.cgi?id=66492 > > What's wrong with this test? Is there another test for touchdown + touchup = synthetic click? This test verified that PlatformGestureEvents with TapType would cause delivery of the appropriate mousemove, mousedown, mouseup sequence needed for a click. This patch removes TapType so this particular layout test doesn't apply. The delivery of synthetic clicks is still exercised by fast/events/touch/touch-gesture-click.html. >> Source/WebCore/page/EventHandler.cpp:2208 >> +// TODO(rjkroege): revert thsi to something more like the previous version > > To remove :) done, easily. :-) >> Source/WebCore/platform/SyntheticGestureEvent.h:53 >> + GestueEndType, > > I suggest to keep those out until the you add the implementation. done. >> Source/WebCore/platform/chromium/GestureRecognizerChromium.cpp:111 >> + gestures->append(SyntheticGestureEvent(SyntheticGestureEvent::TapEndType, FloatPoint(firstTouchPosition()), FloatPoint(firstTouchPosition()), m_lastTouchTime, FloatSize(), m_shiftKey, m_ctrlKey, m_altKey, m_metaKey)); > > Look like the screen position are not valid here, and that should not be too difficult to fix. I suggest you fix that now instead of getting weird bugs later. Thanks for the catch. Fixed. >> Source/WebKit/chromium/public/WebInputEvent.h:113 >> + // FIXME: insert WebSyntheticGestureEvent > > I am not familiar with Chromium, don't you need this to get the feature to work? No. Currently: SyntheticGestureEvents get manufactured by the PlatformGestureRecognizer instance by Chromium WebKit and sent into WebCore. This FIXME records a future enhancement to the Chromium WebKit API to permit delivering SyntheticGestureEvents from the Chromium embedder.
Comment on attachment 105205 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105205&action=review Look good! > Source/WebCore/page/EventHandler.cpp:2235 > + PlatformMouseEvent fakeMouseMove(roundedIntPoint(synthetic.position()), roundedIntPoint(synthetic.globalPosition()), NoButton, MouseEventMoved, /* clickCount */ 1, synthetic.shiftKey(), synthetic.ctrlKey(), synthetic.altKey(), synthetic.metaKey(), synthetic.timestamp()); > + PlatformMouseEvent fakeMouseDown(roundedIntPoint(synthetic.position()), roundedIntPoint(synthetic.globalPosition()), LeftButton, MouseEventPressed, /* clickCount */ 1, synthetic.shiftKey(), synthetic.ctrlKey(), synthetic.altKey(), synthetic.metaKey(), synthetic.timestamp()); > + PlatformMouseEvent fakeMouseUp(roundedIntPoint(synthetic.position()), roundedIntPoint(synthetic.globalPosition()), LeftButton, MouseEventReleased, /* clickCount */ 1, synthetic.shiftKey(), synthetic.ctrlKey(), synthetic.altKey(), synthetic.metaKey(), synthetic.timestamp()); It would be a bit cleaner to have local variables for roundedIntPoint(synthetic.position()) and roundedIntPoint(synthetic.globalPosition()) instead of repeating the call. > Source/WebCore/page/EventHandler.cpp:2249 > + // FIXME: Implement additional synthetic gestures. This comment does not convey enough information in my opinion. You should mention examples of what is missing (or not have the comment).
Created attachment 105494 [details] Patch
Comments addressed in new patch.
Comment on attachment 105494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105494&action=review I think we should avoid overloading the term "synthetic event". That already means events created from document.createEvent() via script. > Source/WebKit/chromium/src/WebInputEventConversion.cpp:-132 > - m_shiftKey = (e.modifiers & WebInputEvent::ShiftKey); why do these lines get deleted?
(In reply to comment #16) > (From update of attachment 105494 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105494&action=review > > I think we should avoid overloading the term "synthetic event". That already means events created from document.createEvent() via script. > > > Source/WebKit/chromium/src/WebInputEventConversion.cpp:-132 > > - m_shiftKey = (e.modifiers & WebInputEvent::ShiftKey); > > why do these lines get deleted? Because I removed the fields from PlatformGestureEvent that I had added in https://bugs.webkit.org/show_bug.cgi?id=65044
After extensive off-line discussion, it has been decided that it is unnecessary to continue with this patch.