Bug 174674 - [WPE] Implement click counting in mouse events
Summary: [WPE] Implement click counting in mouse events
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on: 173419
Blocks: 189664 wpe-webdriver
  Show dependency treegraph
 
Reported: 2017-07-20 07:14 PDT by Claudio Saavedra
Modified: 2020-10-20 18:44 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.38 KB, patch)
2017-07-27 04:08 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (7.05 KB, patch)
2017-07-28 07:06 PDT, Claudio Saavedra
clopez: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2017-07-20 07:14:09 PDT
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.
Comment 1 Claudio Saavedra 2017-07-27 04:08:03 PDT
Created attachment 316544 [details]
Patch
Comment 2 Zan Dobersek 2017-07-27 23:42:49 PDT
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.
Comment 3 Claudio Saavedra 2017-07-28 05:03:17 PDT
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.
Comment 4 Claudio Saavedra 2017-07-28 07:06:01 PDT
Created attachment 316637 [details]
Patch
Comment 5 Darin Adler 2017-07-30 14:19:45 PDT
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.
Comment 6 Carlos Alberto Lopez Perez 2017-10-10 12:13:41 PDT
Comment on attachment 316637 [details]
Patch

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