Bug 96465 – [EFL][WK2] TestRunner needs touch events support.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

In order to pass fast/events/touch,
EventSenderProxyEfl needs touch events support.

[Chris Dumez]

*** Bug 96792 has been marked as a duplicate of this bug. ***

[Nikhil Bansal]

Created attachment 165074 [details]
Patch

[Ryuan Choi]

Comment on attachment 165074 [details]
Patch

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

Thank you.

> Tools/ChangeLog:3
> +        [EFL][WK2] TestRunner needs touch events support.

We can update testExpectations in LayoutTests. Am I right?

> Tools/WebKitTestRunner/EventSenderProxy.h:37
> +#if ENABLE(TOUCH_EVENTS)

I think that it looks not needed

> Tools/WebKitTestRunner/EventSenderProxy.h:137
> +    Vector<Ewk_Touch_Point> m_touchPoints;

Isn't it usefull using Eina_List instead of Vector?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:433
> +    for (unsigned i = 0; i < m_touchPoints.size(); i++) {

s/unsigned/size_t and ++i is preferred.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:522
> +    Ewk_Touch_Point* touchPoint = &m_touchPoints[index];
> +    touchPoint->state = EVAS_TOUCH_POINT_UP;

I think that touchPoint is not needed.

[Kangil Han]

Comment on attachment 165074 [details]
Patch

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

> Tools/WebKitTestRunner/EventSenderProxy.h:52
> +typedef Evas_Object* PlatformWKView;

This is commonly used file.
You can use this in EventSenderProxyEfl.cpp

> Tools/WebKitTestRunner/EventSenderProxy.h:107
> +    PlatformWKView getPlatformView() const;

We don't need this.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:451
> +    for (unsigned i = 0; i < m_touchPoints.size(); i++) {

++i

[Nikhil Bansal]

Created attachment 165253 [details]
Patch

[Nikhil Bansal]

(In reply to comment #3)
> (From update of attachment 165074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165074&action=review
> 
> Thank you.
> 
> > Tools/ChangeLog:3
> > +        [EFL][WK2] TestRunner needs touch events support.
> 
> We can update testExpectations in LayoutTests. Am I right?

Yes, I have updated it in new patch. Thank you. 

> > Tools/WebKitTestRunner/EventSenderProxy.h:37
> > +#if ENABLE(TOUCH_EVENTS)
> 
> I think that it looks not needed
>
> > Tools/WebKitTestRunner/EventSenderProxy.h:137
> > +    Vector<Ewk_Touch_Point> m_touchPoints;
> 
> Isn't it usefull using Eina_List instead of Vector?
> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:433
> > +    for (unsigned i = 0; i < m_touchPoints.size(); i++) {
> 
> s/unsigned/size_t and ++i is preferred.
> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:522
> > +    Ewk_Touch_Point* touchPoint = &m_touchPoints[index];
> > +    touchPoint->state = EVAS_TOUCH_POINT_UP;
> 
> I think that touchPoint is not needed.

I have modified the code to use Eina_List instead of Vector. Please review.

[Nikhil Bansal]

(In reply to comment #4)
> (From update of attachment 165074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165074&action=review
> 
> > Tools/WebKitTestRunner/EventSenderProxy.h:52
> > +typedef Evas_Object* PlatformWKView;
> 
> This is commonly used file.
> You can use this in EventSenderProxyEfl.cpp
> 
> > Tools/WebKitTestRunner/EventSenderProxy.h:107
> > +    PlatformWKView getPlatformView() const;
> 
> We don't need this.
> 

I added getPlatformView() to improve readability. I have now removed it from #if ENABLE(TOUCH_EVENTS) so that other apis can also use it.

I think we can add getPlatformWindow() also to refactor code to improve readability (maybe in another patch). 

Please let me know what you think.

> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:451
> > +    for (unsigned i = 0; i < m_touchPoints.size(); i++) {
> 
> ++i

I have made the changes in new patch. Please review. Thank you.

[Chris Dumez]

Comment on attachment 165253 [details]
Patch

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

> Tools/WebKitTestRunner/EventSenderProxy.h:49
> +typedef Evas_Object* PlatformWKView;

Why do we need this typedef if we use this type only if #if PLATFORM(EFL) protected code? We could just use Evas_Object* directly. It is clearer IMHO.

> Tools/WebKitTestRunner/EventSenderProxy.h:104
> +    void sendTouchEvent(const Ewk_Touch_Event_Type&);

Isn't Ewk_Touch_Event_Type just an enumeration? We probably don't need to use a const reference here right?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:297
> +    , m_touchPoints(0)

We should probably free the list and its items in the destructor.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:445
> +    ASSERT(eina_list_count(m_touchPoints));

If you just want to make sure the list is not empty, why not so ASSERT(m_touchPoints) ?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:447
> +    ewk_view_feed_touch_event(getPlatformView(), eventType, m_touchPoints, evas_key_modifier_get(evas_object_evas_get(getPlatformView())));

Maybe we should cache the view to avoid calling getPlatformView() twice?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:449
> +    Eina_List* list = 0;

Can be defined inside the for loop condition

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:456
> +            m_touchPoints = eina_list_remove_list(m_touchPoints, tempList);

You not freeing memory for the item being removed, you're probably leaking memory here.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:465
> +    if (eina_list_count(m_touchPoints)) {

if (m_touchPoints) ? Calling eina_list_count() is probably costly for checking if a list is empty.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:466
> +        Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(eina_list_nth(m_touchPoints, eina_list_count(m_touchPoints)-1));

eina_list_last() ?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:470
> +    Ewk_Touch_Point* touchPoint = new Ewk_Touch_Point;

Can't we pass the initialisation members to Ewk_Touch_Point constructor (Adding one if necessary) ?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:491
> +    static const char* modifierNames[] = { "Shift", "Control", "Alt", "Meta" };

Isn't this duplicated? I'm pretty sure we have the same code for Keyboard modifier. We can probably refactor to share code here?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:525
> +    eina_list_free(m_touchPoints);

You're clearing the list but not its items -> leak.

[Nikhil Bansal]

Created attachment 165261 [details]
Patch

[Nikhil Bansal]

(In reply to comment #8)
> (From update of attachment 165253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165253&action=review
> 
> We should probably free the list and its items in the destructor.
> 

Thanks for reviewing. I have the modified the code as per comments. Please review.

> 
> Can't we pass the initialisation members to Ewk_Touch_Point constructor (Adding one if necessary) ?
>

Currently it is not possible. Shall I add constructor as part of this patch?
 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:491
> > +    static const char* modifierNames[] = { "Shift", "Control", "Alt", "Meta" };
> 
> Isn't this duplicated? I'm pretty sure we have the same code for Keyboard modifier. We can probably refactor to share code here?
> 

I've modified setEvasModifiers(). Please check.

[Chris Dumez]

Comment on attachment 165261 [details]
Patch

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

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:134
> +static void setEvasModifiers(Evas* evas, WKEventModifiers wkModifiers, bool state = true, bool isTouchModifier = false)

This becomes a bit complicated. Since the behavior is slightly different, maybe we should share only the modifier to const char* converting? e.g. 
static inline const char* toEvasModifier(unsigned modifier);

What do you think?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:465
> +        if (!touchPoint)

How is this possible? Shouldn't it simply be an assertion?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:482
> +        if (!touchPoint)

Ditto.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:488
> +    Ewk_Touch_Point* touchPoint = new Ewk_Touch_Point;

I would add the needed constructor. It looks weird to assign the members afterwards.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:505
> +    if (!touchPoint)

assert?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:542
> +        if (data) {

Why is this needed?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:554
> +    if (!touchPoint)

assert?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:565
> +    if (!touchPoint)

assert?

[Mikhail Pozdnyakov]

Comment on attachment 165261 [details]
Patch

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

> Tools/WebKitTestRunner/EventSenderProxy.h:102
> +    Evas_Object* getPlatformView() const;

no point in having 'const' here since it returns pointer which is not const

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:489
> +    if (!touchPoint)

Checking 'new' operator return? have not seen such checking anywhere else in WK code.. Anyway, think, we should not simply return in such error cases even without logging.

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:543
> +            Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(data);

Does Ewk_Touch_Point have destructor to be invoked?

[Nikhil Bansal]

Created attachment 165347 [details]
Patch

[Nikhil Bansal]

(In reply to comment #11)
> (From update of attachment 165261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165261&action=review
> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:134
> > +static void setEvasModifiers(Evas* evas, WKEventModifiers wkModifiers, bool state = true, bool isTouchModifier = false)
> 
> This becomes a bit complicated. Since the behavior is slightly different, maybe we should share only the modifier to const char* converting? e.g. 
> static inline const char* toEvasModifier(unsigned modifier);
> 
> What do you think?
> 
Yes, it gets a little complicated. 
setEvasModifier() is used to set multiple modifiers whereas setTouchModifier() deals with only one modifier at a time.
With setTouchModifier() we can have multiple modifiers enabled at a time
whereas with setEvasModifiers() only one modifier will be enabled.
 
As discussed, I have made the change to use modifierNames in both modifier related APIs.

> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:488
> > +    Ewk_Touch_Point* touchPoint = new Ewk_Touch_Point;
> 
> I would add the needed constructor. It looks weird to assign the members afterwards.
> 

I think ewk_touch.h is public header and it is not possible to add constructor.

I have made other changes as per comments. Please check.

[Nikhil Bansal]

(In reply to comment #12)
> (From update of attachment 165261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165261&action=review
> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:489
> > +    if (!touchPoint)
> 
> Checking 'new' operator return? have not seen such checking anywhere else in WK code.. Anyway, think, we should not simply return in such error cases even without logging.
> 
I have removed the check to make it consistent with code. Please review. Thank you.

> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:543
> > +            Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(data);
> 
> Does Ewk_Touch_Point have destructor to be invoked?
> 
Ewk_Touch_Point does not have a destructor. Memory for touch point is allocated in addTouchPoint().

[Chris Dumez]

Comment on attachment 165347 [details]
Patch

LGTM but please make sure to run the layout tests in debug mode before landing this.

[Mikhail Pozdnyakov]

Comment on attachment 165347 [details]
Patch

Looks good, thanks.

[Nikhil Bansal]

(In reply to comment #16)
> (From update of attachment 165347 [details])
> LGTM but please make sure to run the layout tests in debug mode before landing this.
>
Yes, I tried running touch related layout tests (LayoutTests/fast/events/touch) in debug mode before creating the patch. revision 129339.

[Kenneth Rohde Christiansen]

Comment on attachment 165347 [details]
Patch

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

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:445
> +inline Evas_Object* EventSenderProxy::getPlatformView()

ALWAYS_INLINE? but you need to declare it in the header with implementation, but then again it is really not needed to be inlined for testing and you are kind of cluttering the api with this method.

Why not just call that directly

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458
> +    for (Eina_List* list = m_touchPoints; list; ) {

Why not use the eina iteration methods?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:475
> +    int id = 0;

unsigned?

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:479
> +        Ewk_Touch_Point* touchPoint = static_cast<Ewk_Touch_Point*>(eina_list_data_get(last));
> +        ASSERT(touchPoint);

Why are we actually using eina lists here and not a Vector<OwnPtr<Ewk_Touch_Point> > or similar?

[Gyuyoung Kim]

Comment on attachment 165347 [details]
Patch

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

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458
> +    for (Eina_List* list = m_touchPoints; list; ) {

If you use EINA_LIST_FOREACH() instead of for loop, can't we reduce unnecessary codes ?

See also : http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_touch_event.cpp#L81

[Chris Dumez]

Comment on attachment 165347 [details]
Patch

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

>> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:475
>> +    int id = 0;
> 
> unsigned?

Well, it is defined as an int in Ewk_Touch_Point so it is consistent. Otherwise we should fix Ewk_Touch_Point declaration in ewk_touch.h

>> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:479
>> +        ASSERT(touchPoint);
> 
> Why are we actually using eina lists here and not a Vector<OwnPtr<Ewk_Touch_Point> > or similar?

because ewk_view_feed_touch_event() takes an Eina_List in argument. If we use a Vector internally (tempting I know), then we need to convert before calling Ewk_View API which is bad.

[Nikhil Bansal]

Created attachment 165362 [details]
Patch

[Nikhil Bansal]

(In reply to comment #19)
> (From update of attachment 165347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165347&action=review
> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:445
> > +inline Evas_Object* EventSenderProxy::getPlatformView()
> 
> ALWAYS_INLINE? but you need to declare it in the header with implementation, but then again it is really not needed to be inlined for testing and you are kind of cluttering the api with this method.
> 
> Why not just call that directly
> 
I have modified the code to remove that api. Please check.
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458
> > +    for (Eina_List* list = m_touchPoints; list; ) {
> 
> Why not use the eina iteration methods?
> 
I've modified code to use EINA_LIST_FOREACH_SAFE since there is a possibility of deleting current node. Please check. Thank you.

[Nikhil Bansal]

(In reply to comment #20)
> (From update of attachment 165347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165347&action=review
> 
> > Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:458
> > +    for (Eina_List* list = m_touchPoints; list; ) {
> 
> If you use EINA_LIST_FOREACH() instead of for loop, can't we reduce unnecessary codes ?
> 
> See also : http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_touch_event.cpp#L81

I'm using EINA_LIST_FOREACH_SAFE now. Please check. Thank you.

[Kenneth Rohde Christiansen]

Comment on attachment 165362 [details]
Patch

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

I think this looks fine

> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:462
> +             delete touchPoint;
> +             m_touchPoints = eina_list_remove_list(m_touchPoints, list);

Is this safe when iterating?

[Chris Dumez]

Comment on attachment 165362 [details]
Patch

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

LGTM.

>> Tools/WebKitTestRunner/efl/EventSenderProxyEfl.cpp:462
>> +             m_touchPoints = eina_list_remove_list(m_touchPoints, list);
> 
> Is this safe when iterating?

According to the doc (http://docs.enlightenment.org/auto/eina/group__Eina__List__Group.html#ga5b4d2aac696cd2558aaed03e3929d873), yes:
"Since this macro stores a pointer to the next list node in l_next, deleting the current node and continuing looping is safe."

[WebKit Review Bot]

Comment on attachment 165362 [details]
Patch

Clearing flags on attachment: 165362

Committed r129437: <http://trac.webkit.org/changeset/129437>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.