Bug 86720 – [EFL][DRT] Implement touch event

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

Implement touch event.

[Kangil Han]

Created attachment 142453 [details]
Patch

[Ryuan Choi]

Comment on attachment 142453 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:654
> +static JSValueRef touchStartCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)

Almost code of touchStartCallback, touchMoveCallback, touchCancelCallback, touchEndCallback are similar.

Can we share codes?

> Tools/DumpRenderTree/efl/EventSender.cpp:659
> +        Ewk_Touch_Point* event = new Ewk_Touch_Point;

you looks missing 'delete'

> Tools/DumpRenderTree/efl/EventSender.cpp:668
> +        if (touchPointList.at(i).state == TouchPressed)
> +            event->state = EWK_TOUCH_POINT_PRESSED;
> +        else
> +            event->state = EWK_TOUCH_POINT_STATIONARY;

EWK_TOUCH_XXX and TouchXXX will be same and AssertMatchingEnums.cpp assure that.

IMO, just casting will be usefull if we can share codes.

[Kangil Han]

Created attachment 142459 [details]
Patch

[Kangil Han]

(In reply to comment #2)
> (From update of attachment 142453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142453&action=review
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:654
> > +static JSValueRef touchStartCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> 
> Almost code of touchStartCallback, touchMoveCallback, touchCancelCallback, touchEndCallback are similar.
> 
> Can we share codes?

Done!

> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:659
> > +        Ewk_Touch_Point* event = new Ewk_Touch_Point;
> 
> you looks missing 'delete'

List will be freed with 'EINA_LIST_FREE' in 'PlatformTouchEventEfl.cpp'

> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:668
> > +        if (touchPointList.at(i).state == TouchPressed)
> > +            event->state = EWK_TOUCH_POINT_PRESSED;
> > +        else
> > +            event->state = EWK_TOUCH_POINT_STATIONARY;
> 
> EWK_TOUCH_XXX and TouchXXX will be same and AssertMatchingEnums.cpp assure that.
> 
> IMO, just casting will be usefull if we can share codes.

Done!

[Ryuan Choi]

> > 
> > > Tools/DumpRenderTree/efl/EventSender.cpp:659
> > > +        Ewk_Touch_Point* event = new Ewk_Touch_Point;
> > 
> > you looks missing 'delete'
> 
> List will be freed with 'EINA_LIST_FREE' in 'PlatformTouchEventEfl.cpp'

No, data itself should be freed by yourself.

And PlatformTouchEventEfl.cpp should not use EINA_LIST_FREE because of freeing user-provided list.

IMO, you'd better to change EINA_LIST_FREE to EINA_LIST_FOREACH in PlatformTouchEventEfl.cpp and call EINA_LIST_FREE to free events in EventSender.cpp.

[Kangil Han]

Created attachment 142603 [details]
Patch

[Kangil Han]

(In reply to comment #5)
> > > 
> > > > Tools/DumpRenderTree/efl/EventSender.cpp:659
> > > > +        Ewk_Touch_Point* event = new Ewk_Touch_Point;
> > > 
> > > you looks missing 'delete'
> > 
> > List will be freed with 'EINA_LIST_FREE' in 'PlatformTouchEventEfl.cpp'
> 
> No, data itself should be freed by yourself.
> 
> And PlatformTouchEventEfl.cpp should not use EINA_LIST_FREE because of freeing user-provided list.
> 
> IMO, you'd better to change EINA_LIST_FREE to EINA_LIST_FOREACH in PlatformTouchEventEfl.cpp and call EINA_LIST_FREE to free events in EventSender.cpp.

Done!

[Ryuan Choi]

Comment on attachment 142603 [details]
Patch

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

Looks quite better than before.

> Tools/DumpRenderTree/efl/EventSender.cpp:155
> +    TouchEventInfo(Ewk_Touch_Point_Type state, WebCore::IntPoint point, unsigned id)

nitpick, const WebCore::IntPoint& ?

> Tools/DumpRenderTree/efl/EventSender.cpp:167
> +static Vector<TouchEventInfo> touchPointList;

Should we need DEFINE_STATIC_LOCAL?

[Kangil Han]

Created attachment 142617 [details]
Patch

[Kangil Han]

(In reply to comment #8)
> (From update of attachment 142603 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142603&action=review
> 
> Looks quite better than before.
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:155
> > +    TouchEventInfo(Ewk_Touch_Point_Type state, WebCore::IntPoint point, unsigned id)
> 
> nitpick, const WebCore::IntPoint& ?
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:167
> > +static Vector<TouchEventInfo> touchPointList;
> 
> Should we need DEFINE_STATIC_LOCAL?

Done!

[Kangil Han]

Created attachment 142620 [details]
Patch

[Kangil Han]

Try again!

[Ryuan Choi]

(In reply to comment #12)
> Try again!

looks fine to me.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 142620 [details]
Patch

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

Have you run all layout tests and made sure this does not cause any regression?

> Source/WebCore/platform/efl/PlatformTouchEventEfl.cpp:39
> +    : PlatformEvent(type, !!(metaState & ShiftKey), !!(metaState & CtrlKey), !!(metaState & AltKey), !!(metaState & MetaKey), currentTime())

The documentation for ewk_frame_feed_touch_event is not being updated, and metaState is still marked as deprecated and with no description of what value it is supposed to have. Please document it properly (and optionally change its API to accept separate parameters for each meaningful modifier).

> Tools/DumpRenderTree/efl/EventSender.cpp:155
> +    TouchEventInfo(Ewk_Touch_Point_Type state, const WebCore::IntPoint& point, unsigned id)

Nit: most of us are used to passing any "id" field as the first parameter to functions.

> Tools/DumpRenderTree/efl/EventSender.cpp:167
> +static int touchModifiers;

Why isn't it a PlatformEvent::Modifiers?

> Tools/DumpRenderTree/efl/EventSender.cpp:643
> +    for (unsigned i = 0; i < touchPointList().size(); i++) {
> +        if (touchPointList().at(i).state == EWK_TOUCH_POINT_RELEASED) {
> +            touchPointList().remove(i);
> +            i--;

You could probably remove this back and forth of the value of `i' by using iterators.

> Tools/DumpRenderTree/efl/EventSender.cpp:651
> +    if (argumentCount < 2)

!= 2 is safer.

> Tools/DumpRenderTree/efl/EventSender.cpp:659
> +    WebCore::IntPoint position(x, y);

Nit: could be const. Why is it called position instead of point?

> Tools/DumpRenderTree/efl/EventSender.cpp:665
> +    unsigned lowestId = 0;
> +    for (unsigned i = 0; i < touchPointList().size(); i++) {
> +        if (touchPointList().at(i).id == lowestId)
> +            lowestId++;
> +    }

I don't understand this. Why not
  const unsigned id = touchPointList().isEmpty() ? 0 : touchPointList().last().id + 1;
?

> Tools/DumpRenderTree/efl/EventSender.cpp:676
> +    sendTouchEvent(EWK_TOUCH_START);
> +    postTouchEvent();

Since these two are always called together, why not merge them?

> Tools/DumpRenderTree/efl/EventSender.cpp:682
> +    if (argumentCount < 3)

!= is safer.

> Tools/DumpRenderTree/efl/EventSender.cpp:712
> +    if (argumentCount < 1)

!= 1 is safer.

> Tools/DumpRenderTree/efl/EventSender.cpp:733
> +    if (argumentCount < 1)

!= 1 is safer.

> Tools/DumpRenderTree/efl/EventSender.cpp:761
> +    if (argumentCount < 2)

!= 2 is safer.

> Tools/DumpRenderTree/efl/EventSender.cpp:766
> +    int mask = 0;

Why not a PlatformEvent::Modifiers?

[Gyuyoung Kim]

Comment on attachment 142620 [details]
Patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:640
> +    for (unsigned i = 0; i < touchPointList().size(); i++) {

Though, as far as I know, there is no difference from the point of view of functionality, generally, it looks WebKit prefers to use ++i instead of i++.

> Tools/DumpRenderTree/efl/EventSender.cpp:643
> +            i--;

I don't understand why you add this line. If we modify loop counter both  *for(;;)* condition and loop body, I think this may make problem.

> Tools/DumpRenderTree/efl/EventSender.cpp:765
> +    bool enable = JSValueToBoolean(context, arguments[1]);

It seems to me this is better to move 765 line to 777 line.

[Kangil Han]

Created attachment 142659 [details]
Patch

[Kangil Han]

gyuyoung, rakuco: Thanks for review!
FOA, checked no regression and done fixing for reviewed point! :)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 142659 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_frame.h:853
> + * @param metaState touch event modifiers

This still isn't helpful -- if I am an API user and check this documentation, I still won't know what this means and what values I can pass.

> Tools/DumpRenderTree/efl/EventSender.cpp:649
> +    postTouchEvent();

I think it makes sense to just merge both functions.

> Tools/DumpRenderTree/efl/EventSender.cpp:668
> +    unsigned lowestId = 0;
> +    for (unsigned i = 0; i < touchPointList().size(); ++i) {
> +        if (touchPointList().at(i).id == lowestId)
> +            ++lowestId;
> +    }

My question about using last() has not been answered.

[Kangil Han]

Created attachment 142891 [details]
Patch

[Kangil Han]

(In reply to comment #18)
> (From update of attachment 142659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142659&action=review
> 
> > Source/WebKit/efl/ewk/ewk_frame.h:853
> > + * @param metaState touch event modifiers
> 
> This still isn't helpful -- if I am an API user and check this documentation, I still won't know what this means and what values I can pass.
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:649
> > +    postTouchEvent();
> 
> I think it makes sense to just merge both functions.
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:668
> > +    unsigned lowestId = 0;
> > +    for (unsigned i = 0; i < touchPointList().size(); ++i) {
> > +        if (touchPointList().at(i).id == lowestId)
> > +            ++lowestId;
> > +    }
> 
> My question about using last() has not been answered.

All done, thanks! :)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 142891 [details]
Patch

Thank you for working on this. I still don't like the way metaState is used or documented (AltKey and the other values do not make any sense to an API user, as they are not part of any public header).

I suggest using the modifier values defined in Ecore_Input.h (ECORE_EVENT_MODIFIER_SHIFT etc). This approach does require more code but is definitely more portable and useful for users:

 - In Source/cmake/FindEFL.cmake, look for ecore-input.
 - Add the dependency to the "Requires" line in Source/WebKit/efl/ewk/ewebkit.pc.in
 - Make ewk_frame_feed_touch_event() take an unsigned int instead of an int for metaState, and rename it to modifiers.
   - Properly document that users are expected to pass ORed values of the ECORE_EVENT_MODIFIER macros in Ecore_Input.h, such as ECORE_EVENT_MODIFIER_ALT or ECORE_EVENT_MODIFIER_SHIFT.
 - You can then parse the value in either ewk_frame_feed_touch_event() or relay it to PlatformTouchEventEfl.
 - Adjust EventSender accordingly.

[Kangil Han]

(In reply to comment #21)
> (From update of attachment 142891 [details])
> Thank you for working on this. I still don't like the way metaState is used or documented (AltKey and the other values do not make any sense to an API user, as they are not part of any public header).
> 

FOA, thanks for your review and I agree with your point.
The only reason why I was holding metaState argument is that I just wanted to keep a API form to prevent further confusion.
But, if we should modify API, then I put my two cents on your #14 comment.
  - "Please document it properly (and optionally change its API to accept separate parameters for each meaningful modifier)."
If you are okay with this, then I will remove 'metaState' and add 4 boolean type arguments(bool altKey, bool ctrlKey, bool metaKey, bool shiftKey).

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #22)
> But, if we should modify API, then I put my two cents on your #14 comment.
>   - "Please document it properly (and optionally change its API to accept separate parameters for each meaningful modifier)."
> If you are okay with this, then I will remove 'metaState' and add 4 boolean type arguments(bool altKey, bool ctrlKey, bool metaKey, bool shiftKey).

While that works in theory, it doesn't match the usual behavior when specifying key modifiers in other toolkits and also in the EFL themselves (see Ecore_Event_Key or Evas_Event_Key_Down, for example).

[Kangil Han]

Created attachment 142994 [details]
Patch

[Kangil Han]

(In reply to comment #21)
> (From update of attachment 142891 [details])
> Thank you for working on this. I still don't like the way metaState is used or documented (AltKey and the other values do not make any sense to an API user, as they are not part of any public header).
> 
> I suggest using the modifier values defined in Ecore_Input.h (ECORE_EVENT_MODIFIER_SHIFT etc). This approach does require more code but is definitely more portable and useful for users:
> 
>  - In Source/cmake/FindEFL.cmake, look for ecore-input.
>  - Add the dependency to the "Requires" line in Source/WebKit/efl/ewk/ewebkit.pc.in
>  - Make ewk_frame_feed_touch_event() take an unsigned int instead of an int for metaState, and rename it to modifiers.
>    - Properly document that users are expected to pass ORed values of the ECORE_EVENT_MODIFIER macros in Ecore_Input.h, such as ECORE_EVENT_MODIFIER_ALT or ECORE_EVENT_MODIFIER_SHIFT.
>  - You can then parse the value in either ewk_frame_feed_touch_event() or relay it to PlatformTouchEventEfl.
>  - Adjust EventSender accordingly.

All done, thanks!

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 142994 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_frame.cpp:983
> +    WebCore::PlatformEvent::Modifiers touchModifiers =static_cast<WebCore::PlatformEvent::Modifiers>(0);

Nit: Missing a space after the '=', and the cast is not needed.

> Source/WebKit/efl/ewk/ewk_frame.cpp:985
> +        touchModifiers = static_cast<WebCore::PlatformEvent::Modifiers>(touchModifiers | WebCore::PlatformEvent::AltKey);

The cast is unneeded.

> Source/WebKit/efl/ewk/ewk_frame.cpp:987
> +        touchModifiers = static_cast<WebCore::PlatformEvent::Modifiers>(touchModifiers | WebCore::PlatformEvent::CtrlKey);

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:989
> +        touchModifiers = static_cast<WebCore::PlatformEvent::Modifiers>(touchModifiers | WebCore::PlatformEvent::ShiftKey);

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:991
> +        touchModifiers = static_cast<WebCore::PlatformEvent::Modifiers>(touchModifiers | WebCore::PlatformEvent::MetaKey);

Ditto.

> Source/WebKit/efl/ewk/ewk_frame.cpp:993
> +    WebCore::PlatformTouchEvent touchEvent(points, WebCore::IntPoint(x, y), type, touchModifiers);

Since you already have a PlatformEvent::Modifiers here, you can simplify the PlatformTouchEvent constructor by accepting Modifiers instead of an int as an argument, similarly to one of PlatformEvent's constructor.

> Tools/DumpRenderTree/efl/EventSender.cpp:754
> +    unsigned mask = static_cast<WebCore::PlatformEvent::Modifiers>(0);

The cast is not needed.

[Kangil Han]

> > Source/WebKit/efl/ewk/ewk_frame.cpp:985
> > +        touchModifiers = static_cast<WebCore::PlatformEvent::Modifiers>(touchModifiers | WebCore::PlatformEvent::AltKey);
> 
> The cast is unneeded.
> 

w/o casting I always meet this error:
error: invalid conversion from ‘int’ to ‘WebCore::PlatformEvent::Modifiers’ [-fpermissive]
Any idea on this?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #27)
> > > Source/WebKit/efl/ewk/ewk_frame.cpp:985
> > > +        touchModifiers = static_cast<WebCore::PlatformEvent::Modifiers>(touchModifiers | WebCore::PlatformEvent::AltKey);
> > 
> > The cast is unneeded.
> 
> w/o casting I always meet this error:
> error: invalid conversion from ‘int’ to ‘WebCore::PlatformEvent::Modifiers’ [-fpermissive]
> Any idea on this?

Sorry for overlooking that. Your `touchModifiers' variable should be an int (or an unsigned) so that you can do bitwise operations like the ones you do. For a more detailed explanation, please see <http://www.parashift.com/c++-faq/newbie.html#faq-29.20>.

[Kangil Han]

Created attachment 143159 [details]
Patch

[Kangil Han]

> Sorry for overlooking that. Your `touchModifiers' variable should be an int (or an unsigned) so that you can do bitwise operations like the ones you do. For a more detailed explanation, please see <http://www.parashift.com/c++-faq/newbie.html#faq-29.20>.

Thanks for the link and all done!

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 143159 [details]
Patch

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

> Source/WebCore/platform/efl/PlatformTouchEventEfl.cpp:39
>  PlatformTouchEvent::PlatformTouchEvent(Eina_List* points, const IntPoint pos, PlatformEvent::Type type, int metaState)
> -    : PlatformEvent(type, false, false, false, false, currentTime())
> +    : PlatformEvent(type, !!(metaState & ShiftKey), !!(metaState & CtrlKey), !!(metaState & AltKey), !!(metaState & MetaKey), currentTime())

We're almost there. As I mentioned a few comments ago, there is no point in doing these bitwise checks again since you already created a proper value in ewk_view_feed_touch_event(). You can simplify this constructor to:

  PlatforTouchEvent::PlatformTouchEvent(const Eina_List* points, const IntPoint pos, PlatformEvent::Type type, PlatformEvent::Modifiers modifiers)
      : PlatformEvent(type, modifiers, currentTime())

> Source/WebKit/efl/ewk/ewk_frame.cpp:983
> +    int touchModifiers = 0;

Since PlatformEvent's m_modifiers is an unsigned and you use an unsigned in EventSender, it'd be good to use an unsigned here as well for consistency.

[Kangil Han]

Created attachment 143176 [details]
Patch

[Kangil Han]

(In reply to comment #31)
> (From update of attachment 143159 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143159&action=review
> 
> > Source/WebCore/platform/efl/PlatformTouchEventEfl.cpp:39
> >  PlatformTouchEvent::PlatformTouchEvent(Eina_List* points, const IntPoint pos, PlatformEvent::Type type, int metaState)
> > -    : PlatformEvent(type, false, false, false, false, currentTime())
> > +    : PlatformEvent(type, !!(metaState & ShiftKey), !!(metaState & CtrlKey), !!(metaState & AltKey), !!(metaState & MetaKey), currentTime())
> 
> We're almost there. As I mentioned a few comments ago, there is no point in doing these bitwise checks again since you already created a proper value in ewk_view_feed_touch_event(). You can simplify this constructor to:
> 
>   PlatforTouchEvent::PlatformTouchEvent(const Eina_List* points, const IntPoint pos, PlatformEvent::Type type, PlatformEvent::Modifiers modifiers)
>       : PlatformEvent(type, modifiers, currentTime())
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:983
> > +    int touchModifiers = 0;
> 
> Since PlatformEvent's m_modifiers is an unsigned and you use an unsigned in EventSender, it'd be good to use an unsigned here as well for consistency.

All done, thanks!

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 143176 [details]
Patch

At last! Informal r+, thanks for all the hard work :)

[Kangil Han]

(In reply to comment #34)
> (From update of attachment 143176 [details])
> At last! Informal r+, thanks for all the hard work :)

Many thanks!

[Hajime Morrita]

Comment on attachment 143176 [details]
Patch

rubberstamping based on the informal r+...

[Kangil Han]

(In reply to comment #36)
> (From update of attachment 143176 [details])
> rubberstamping based on the informal r+...

morrita: Thanks!!

[WebKit Review Bot]

Comment on attachment 143176 [details]
Patch

Clearing flags on attachment: 143176

Committed r118103: <http://trac.webkit.org/changeset/118103>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.