[webkit-reviews] review denied: [Bug 214870] [WPE][Pointer Events] Add support for touch based pointer events : [Attachment 405352] Patch

[bugzilla-daemon at webkit.org]

Carlos Alberto Lopez Perez <clopez at igalia.com> has denied Marco Felsch
<m.felsch at pengutronix.de>'s request for review:
Bug 214870: [WPE][Pointer Events] Add support for touch based pointer events
https://bugs.webkit.org/show_bug.cgi?id=214870

Attachment 405352: Patch

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




--- Comment #6 from Carlos Alberto Lopez Perez <clopez at igalia.com> ---
Comment on attachment 405352
  --> https://bugs.webkit.org/attachment.cgi?id=405352
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