Bug 174674 – [WPE] Implement click counting in mouse events

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Claudio Saavedra]

WPE does not provide single-, double-, etc. click information, but this can be implemented in WebEventFactory. This missing information currently breaks fast/events/click-count.html.

[Claudio Saavedra]

Created attachment 316544 [details]
Patch

[Zan Dobersek]

Comment on attachment 316544 [details]
Patch

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

> Source/WebKit/Shared/wpe/WebEventFactory.cpp:89
> +    static struct wpe_input_pointer_event m_lastClickEvent;

This should be zero-initialized:
  `static struct wpe_input_pointer_event m_lastClickEvent = { 0, };`

Might be the zero-init is guaranteed because the variable is static, but it's better to be explicit.

> Source/WebKit/Shared/wpe/WebEventFactory.cpp:99
> +    if (event->time - m_lastClickEvent.time < 1 && event->x == m_lastClickEvent.x && event->y == m_lastClickEvent.y && event->button == m_lastClickEvent.button)
> +        m_clickCount++;
> +    else
> +        m_clickCount = 1;

Difference in time between the two events should be somehow made configurable in the future. I don't know exactly what time unit is used for the input event time, and it could differ from platform to platform anyway.

Should the click count be upwards-limited? As it stands it can grow uncapped.

[Claudio Saavedra]

Comment on attachment 316544 [details]
Patch

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

>> Source/WebKit/Shared/wpe/WebEventFactory.cpp:99
>> +        m_clickCount = 1;
> 
> Difference in time between the two events should be somehow made configurable in the future. I don't know exactly what time unit is used for the input event time, and it could differ from platform to platform anyway.
> 
> Should the click count be upwards-limited? As it stands it can grow uncapped.

It only grows if it's happening in the same position and quickly enough to be counted as consecutive clicks, otherwise it is reset back to 1. WebKit uses this count to decide when it's a double or a triple click, and then again single, double, etc. As is I don't think it's a problem if it keeps growing..

As for the time between events, it seems to be a timestamp, so it's seconds. We can make it configurable at some point, not sure where would that go.

[Claudio Saavedra]

Created attachment 316637 [details]
Patch

[Darin Adler]

Comment on attachment 316637 [details]
Patch

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

> Source/WebKit/Shared/wpe/WebEventFactory.cpp:90
> +    static struct wpe_input_pointer_event m_lastClickEvent = { };
> +    static unsigned m_clickCount = 1;

A function-private global variable should not have the prefix "m_" like this.

The design pattern here is dangerous. It requires that we ask this question about each event once. If you call the function a second time or call it out of order then it does the wrong thing. I don’t see code elsewhere that guarantees or relies that this will be called exactly once for each event.

> Source/WebKit/Shared/wpe/WebEventFactory.cpp:96
> +    if (event->time - m_lastClickEvent.time < 1 && event->x == m_lastClickEvent.x && event->y == m_lastClickEvent.y && event->button == m_lastClickEvent.button)

Typically, requiring that x and y being exactly the same is too strict a rule for double clicking. What is the unit for "time"? Typically 1 second is too long a time to define close enough in time to be a double click.

[Carlos Alberto Lopez Perez]

Comment on attachment 316637 [details]
Patch

Setting r- due to Darin comments, please address them and upload a new patch.

[Diego Pino]

Pull request: https://github.com/webkit/webkit/pull/15555

[EWS]

Committed 265808@main (0482f3984fab): <https://commits.webkit.org/265808@main>

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