Bug 214870 – [WPE][Pointer Events] Add support for touch based pointer events

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Marco Felsch]

[WPE][Pointer Events] Add support for touch based pointer events

[Marco Felsch]

Created attachment 405348 [details]
Patch

[Marco Felsch]

Damn, I missed a few #ifdef. The new version will fix this.

[Marco Felsch]

Created attachment 405352 [details]
Patch

[Marco Felsch]

Hi,

I know I have to rework at least the drop of POINTER_EVENTS and runtime check but can you give me some feedback. Is this RFC completely wrong?

[Chris Lord]

Comment on attachment 405352 [details]
Patch

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

I'm not a reviewer, so unfortunately this will just be a drive-by review. For what it's worth, this overall looks good to me, modulo the things I've commented on.

> Source/WebCore/dom/PointerEvent.cpp:96
> +#if ENABLE(TOUCH_EVENTS) && PLATFORM(WPE)

These constructors should probably go in a separate dom/wpe/PointerEventWPE.cpp if they're truly WPE-specific, to mimic how this has been done for iOS. In the case that multiple platforms start using this, they can be moved at that point I would suppose.

> Source/WebCore/dom/PointerEvent.cpp:99
> +    // FIXME:

nit, no need for the newline after this FIXME. Also, s/according the/according to the/

> Source/WebCore/dom/PointerEvent.h:-84
> -#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS_FAMILY)

nit; I think nested #if's tend to be avoided in new code. This can be left alone and a separate #if ENABLE(TOUCH_EVENTS) && PLATFORM(WPE) could be added afterwards.

> Source/WebCore/dom/PointerEvent.h:131
> +    // We have contact with the touch surface for most events except when we've released the touch or canceled it.

This comment is misaligned. Also, while I personally prefer your indentation, I'd have left the below as it was originally, to make it easier to see that this is just a code move.

> Source/WebCore/dom/PointerEvent.h:-128
> -#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS_FAMILY)

Same nit as before re nested #ifs.

> Source/WebCore/page/EventHandler.cpp:4082
> +static const AtomString& pointerEventNameForTouchPointState(PlatformTouchPoint::State state)

This is only used when ENABLE(POINTER_EVENTS) && PLATFORM(WPE) - this should be declared in a block with matching conditionals to where it gets used.

> Source/WebCore/page/EventHandler.cpp:4094
> +        // TouchStationary state is not converted to touch events, so fall through to assert.

I couldn't easily follow the logic below in EventHandler::handleTouchEvent, but it didn't immediately look like a TouchStationary event wouldn't be passed to this function - could you explain how this gets handled so that this ought to never happen?

> Source/WebCore/page/PointerCaptureController.cpp:193
> +static Ref<PointerEvent> createPointerEvent(const String& type, const PlatformTouchEvent& platformTouchEvent, unsigned index, bool isPrimary, WindowProxy& view, const Touch* touch)

I think it would make for a slightly cleaner look elsewhere to reorder these so that the unused-on-WPE parameters come last and have defaults so that they can be ommitted when calling this function.

> Source/WebCore/page/PointerCaptureController.cpp:336
> +        dispatchEventForTouch(*touch->target(), PlatformTouchEvent() /* Don't care */, 0 /* Don't care */, isPrimary, view, type.string(), touch);

I don't think these /* Don't care */ comments are necessary.

> Source/WebCore/page/PointerCaptureController.h:-62
> -#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS_FAMILY)

Same nit here as before regarding nested #ifs.

[Carlos Alberto Lopez Perez]

Comment on attachment 405352 [details]
Patch

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

Thanks! Patch looks good overall :)

But there are some issues that need to be fixed before landing:

1. If you can please upload a new version of it rebased and ensuring the the layout tests run on WPE (unskipping them, for that you have to put this line "pointerevents/ios [ Pass ]" in the file LayoutTests/platform/wpe/TestExpectations) and if they all pass, then a bonus point would be renaming that directory from ios to touch and also renaming the related lines on LayoutTests/platform/wpe/TestExpectations and LayoutTests/TestExpectations)

2. Please take a look at the previous comments from Chris and try to address them

I'm setting r- to make clear a new version of the patch is needed in order to continue with the review process.

> Source/WebCore/ChangeLog:9
> +        No new tests needed because we can reuse the tests from:
> +        LayoutTests/pointerevents/ios/

Those tests are skipped on the main TestExpectation file.
If they work for WPE as well, then this patch should unskip them on the WPE TestExpectation file.
It can be also a good idea to rename the directory to something like LayoutTests/pointerevents/touch as that seems more appropriate if they work in WPE

>> Source/WebCore/page/PointerCaptureController.cpp:336
>> +        dispatchEventForTouch(*touch->target(), PlatformTouchEvent() /* Don't care */, 0 /* Don't care */, isPrimary, view, type.string(), touch);
> 
> I don't think these /* Don't care */ comments are necessary.

Agree

[Carlos Alberto Lopez Perez]

I was puzzled to realize that we were not running any tests for pointerevents, all of them were skipped on all platforms. So on r287196 I unskiped them and now on WPE (and GTK) we are running some of those tests.

Regarding the tests of pointerevents/ios :

 1. Those are crashing and I tested this patch and it didn't fixed them. They continue to crash

 2. I'm not sure this pointerevents/ios tests can run on other platforms than iOS. So forget my previous suggestion of renaming this to pointerevents/touch

Since those tests are skipped by default (they crash) and you want to test if your patch fixes them you have to add this line:

pointerevents/ios [ Pass ]

At the bottom of the file LayoutTests/platform/wpe/TestExpectations

And then you run the tests like this:

$ Tools/Scripts/run-webkit-tests --debug-rwt-logging --release --wpe pointerevents/ios

If your patch fixes one of those tests but not all, then you can white-list that specific test by adding a line like

pointerevents/ios/touch-action-none.html [ Pass ]


You can try to run also the tests imported from WPT by telling run-webkit-tests to run the tests from imported/w3c/web-platform-tests/pointerevents

But note that we have the bad habit of having for those imported tests expectations that contain FAIL lines. That means that if your patch fixes something there, it may appear as a failure on run-webkit-tests. You have to check the text diff and see if your patch is removing "expected failures" and adding "expected passes". If that is the case then the patch is good, you have to update the -expected.txt for that test inside the LayoutTest directory with the new -actual.txt generated. Then run-webkit-tests will stop complaining and will give the result as a pass.

If you have any doubt feel free to ask.

[Bastian Krause]

Comment on attachment 405352 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:4094
>> +        // TouchStationary state is not converted to touch events, so fall through to assert.
> 
> I couldn't easily follow the logic below in EventHandler::handleTouchEvent, but it didn't immediately look like a TouchStationary event wouldn't be passed to this function - could you explain how this gets handled so that this ought to never happen?

AFAICS this follows the same logic as eventNameForTouchPointState() does, which follows the same assumption. handleTouchPointerEvent() also contains the same note about TouchStationary, so I think this applies here as well.

>> Source/WebCore/page/PointerCaptureController.cpp:193
>> +static Ref<PointerEvent> createPointerEvent(const String& type, const PlatformTouchEvent& platformTouchEvent, unsigned index, bool isPrimary, WindowProxy& view, const Touch* touch)
> 
> I think it would make for a slightly cleaner look elsewhere to reorder these so that the unused-on-WPE parameters come last and have defaults so that they can be ommitted when calling this function.

I can't see the advantage here, this is a static function that is called from platform-unspecific code, so where would these defaults help exactly?

[Bastian Krause]

Note: I'm taking over for Marco, I've revisited all feedback, did various manual testing, added some improvements and had a look at the tests. I'll create a PR on GitHub with an updated version next week.

[Bastian Krause]

Pull request: https://github.com/WebKit/WebKit/pull/9688

[EWS]

Committed 263969@main (84e4a4ae1e89): <https://commits.webkit.org/263969@main>

Reviewed commits have been landed. Closing PR #9688 and removing active labels.