[Webkit-unassigned] [Bug 214870] [WPE][Pointer Events] Add support for touch based pointer events

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=214870

Chris Lord <clord at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |clord at igalia.com

--- Comment #5 from Chris Lord <clord 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

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210616/47b82ee4/attachment.htm>