Bug 66492 – [chromium] Separate platform gesture events from synthetic gestures

• Component: UI Events
• Status: RESOLVED WONTFIX

[Robert Kroeger]

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.

[Robert Kroeger]

Created attachment 104384 [details]
Patch, not to be committed. Looking for feedback.

[WebKit Review Bot]

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.

[Robert Kroeger]

Created attachment 105058 [details]
Patch

[Robert Kroeger]

Benjamin: please have a look.

[Robert Kroeger]

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.

[Adam Barth]

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>

?

[Robert Kroeger]

Comment on attachment 105058 [details]
Patch

!@* I always forget to proof-read something.

[Robert Kroeger]

Created attachment 105070 [details]
Patch

[Benjamin Poulain]

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?

[Benjamin Poulain]

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?

[Robert Kroeger]

Created attachment 105205 [details]
Patch

[Robert Kroeger]

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.

[Benjamin Poulain]

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).

[Robert Kroeger]

Created attachment 105494 [details]
Patch

[Robert Kroeger]

Comments addressed in new patch.

[Darin Fisher (:fishd, Google)]

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?

[Robert Kroeger]

(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

[Robert Kroeger]

(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

[Robert Kroeger]

After extensive off-line discussion, it has been decided that it is unnecessary to continue with this patch.