Bug 108915 – [EFL][WK2] Add WK2 C API to handle touch events.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Eunmi Lee]

Add WKViewHandleTouchEvent() API for handling touch event in order to use C WK2 API and not C++ internals.

[Eunmi Lee]

Created attachment 186596 [details]
Patch

[WebKit Review Bot]

Attachment 186596 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/NativeWebTouchEvent.h', u'Source/WebKit2/Shared/efl/NativeWebTouchEventEfl.cpp', u'Source/WebKit2/Shared/efl/WebEventFactory.cpp', u'Source/WebKit2/Shared/efl/WebEventFactory.h', u'Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h', u'Source/WebKit2/UIProcess/API/C/efl/WKView.cpp', u'Source/WebKit2/UIProcess/API/C/efl/WKView.h', u'Source/WebKit2/UIProcess/API/efl/EwkView.cpp', u'Source/WebKit2/UIProcess/API/efl/EwkView.h', u'Source/WebKit2/UIProcess/API/efl/ewk_view.cpp', u'Source/WebKit2/UIProcess/efl/WebView.cpp', u'Source/WebKit2/UIProcess/efl/WebView.h']" exit_code: 1
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:41:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:42:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:48:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:49:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:50:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:51:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:66:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 10 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eunmi Lee]

Created attachment 186600 [details]
Patch

Modify checker.py for style error.

[Kenneth Rohde Christiansen]

Comment on attachment 186596 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +
> +        Add WKViewHandleTouchEvent() API to handle touch events using C WK2 API
> +        and not C++ internals. Additionally, WKTouchEvent and several types are
> +        defined to represent touch events as WK style.

Let's make this more clear.

Touch events for our target platforms differ (XInput2, Wayland, Tizen) so
for the EFL port we want to be able to feed events manually.

To keep our current API for desktop EFL/Enlightenment working, we convert
our currently limited EFL events to the new EFL specific C WKTouchEvent type.

The WKViewSendTouchEvent can be used for any use-case (platform/product)
not involving the EFL events.

At the same time we make sure to avoid using WK2 internals in our EFL API
implementation.

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:81
> +#endif /* WKEventEfl_h */

Wouldn't this not be included as <efl/WKEvent.h> 

Maybe we do not need the appended Efl. Anyway it shouldn't matter. WebKit2 does not provide a WKEvent.h itself. So even if we install the headers in the same place there should be no conflict

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:56
> +WK_EXPORT void WKViewHandleTouchEvent(WKViewRef, const WKTouchEvent*);

WKViewSendTouchEvent(WKViewRef, const WKTouchEvent*); is better I think

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:959
> +void EwkView::handleTouchEvent(Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)

send? as it doesnt return a bool on whether it got handed or not... it seems more correct.

Maybe, as this is purely conversion, it should be done directly in the ewk C API in ewk_view?

[Mikhail Pozdnyakov]

Comment on attachment 186600 [details]
Patch

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

> Source/WebKit2/Shared/efl/WebEventFactory.cpp:243
> +    for (unsigned i = 0; i < event.numTouchPoints; ++i) {

maybe it's worth making sure that event.numTouchPoints < kWKMaximumTouchPointsPerTouchEvent?

> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:85
> +{

ASSERT (touchEvent) ?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:963
> +    WKTouchEvent touchEvent;

maybe creation of WKTouchEvent deservse a separate function?

[Chris Dumez]

Comment on attachment 186600 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL][WK2] Add WK2 C API to handle touch event.

events (plural) I guess.

> Source/WebKit2/Shared/NativeWebTouchEvent.h:50
> +    const WKTouchEvent* nativeEvent() const { return &m_nativeEvent; }

It seems strange that the native touch event for EFL is a WK type. Also, this is internal WK2 C++ code. It usually does not rely on C API types.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:996
> +        switch (point->state) {

I guess we could do a cast instead and use COMPILE_ASSERT_MATCHING_ENUM() from ewk_private instead? This would be more efficient.

[Chris Dumez]

Comment on attachment 186600 [details]
Patch

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

> Source/WebKit2/UIProcess/efl/WebView.cpp:79
> +    m_webPageProxy->handleTouchEvent(NativeWebTouchEvent(touchEvent));

Maybe we could add a "NativeWebTouchEvent toNativeWebTouchEvent(const WKTouchEvent&);" to Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h. Internally, we would construct a WebTouchEvent* from the WKTouchEvent and construct a NativeWebTouchEvent from the WebTouchEvent*. This would remove the dependency on WKTouchEvent from NativeWebTouchEvent. I think we should remove this dependency to respect layering between internal WK2 C++ API and external WK2 C API.

See as example:
Source/WebKit2/Shared/API/c/WKSharedAPICast.h:inline WebCore::IntRect toIntRect(const WKRect& wkRect)

[Eunmi Lee]

Comment on attachment 186600 [details]
Patch

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

>> Source/WebKit2/ChangeLog:3
>> +        [EFL][WK2] Add WK2 C API to handle touch event.
> 
> events (plural) I guess.

Yes, I will modify that.

>> Source/WebKit2/Shared/NativeWebTouchEvent.h:50
>> +    const WKTouchEvent* nativeEvent() const { return &m_nativeEvent; }
> 
> It seems strange that the native touch event for EFL is a WK type. Also, this is internal WK2 C++ code. It usually does not rely on C API types.

Yes, C API types are not usually used in the internal WK2 c++ codes.
However in our case, we want to use common event type not Evas event type, (Evas does not have type for "touch" event, anyway)
so we have to define that like WKTouchEvent.

Additionally, the native event has to be stored because I want to use that when I return the result of touch event to the client of WKView.
So, I don't want to remove C API dependency from here.

How about changing the name of event type to EflTouchEvent? I think it can seem strange because the type starts with "WK".

>> Source/WebKit2/Shared/efl/WebEventFactory.cpp:243
>> +    for (unsigned i = 0; i < event.numTouchPoints; ++i) {
> 
> maybe it's worth making sure that event.numTouchPoints < kWKMaximumTouchPointsPerTouchEvent?

Yes, right.

>> Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:85
>> +{
> 
> ASSERT (touchEvent) ?

I will add that.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:963
>> +    WKTouchEvent touchEvent;
> 
> maybe creation of WKTouchEvent deservse a separate function?

yes, it is better.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:996
>> +        switch (point->state) {
> 
> I guess we could do a cast instead and use COMPILE_ASSERT_MATCHING_ENUM() from ewk_private instead? This would be more efficient.

I couldn't do that because the sequence of types is different between EVAS_TOUCH_POINT and TouchPointState in the webkit.
(EVAS_TOUCH_POINT_UP is second member, but kWKTouchPointStateTouchReleased is first member.)

>> Source/WebKit2/UIProcess/efl/WebView.cpp:79
>> +    m_webPageProxy->handleTouchEvent(NativeWebTouchEvent(touchEvent));
> 
> Maybe we could add a "NativeWebTouchEvent toNativeWebTouchEvent(const WKTouchEvent&);" to Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h. Internally, we would construct a WebTouchEvent* from the WKTouchEvent and construct a NativeWebTouchEvent from the WebTouchEvent*. This would remove the dependency on WKTouchEvent from NativeWebTouchEvent. I think we should remove this dependency to respect layering between internal WK2 C++ API and external WK2 C API.
> 
> See as example:
> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:inline WebCore::IntRect toIntRect(const WKRect& wkRect)

I can't remove dependency of WKTouchEvent from NativeTouchEvent, because I save WKTouchEvent in the NativeTouchEvent as a native event to use it when I return the result of touch event to the client of WebView.
So, I want to maintain current codes.

[Eunmi Lee]

Comment on attachment 186596 [details]
Patch

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

>> Source/WebKit2/ChangeLog:10
>> +        defined to represent touch events as WK style.
> 
> Let's make this more clear.
> 
> Touch events for our target platforms differ (XInput2, Wayland, Tizen) so
> for the EFL port we want to be able to feed events manually.
> 
> To keep our current API for desktop EFL/Enlightenment working, we convert
> our currently limited EFL events to the new EFL specific C WKTouchEvent type.
> 
> The WKViewSendTouchEvent can be used for any use-case (platform/product)
> not involving the EFL events.
> 
> At the same time we make sure to avoid using WK2 internals in our EFL API
> implementation.

Thank you, your explanation makes me clear too. :)

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:81
>> +#endif /* WKEventEfl_h */
> 
> Wouldn't this not be included as <efl/WKEvent.h> 
> 
> Maybe we do not need the appended Efl. Anyway it shouldn't matter. WebKit2 does not provide a WKEvent.h itself. So even if we install the headers in the same place there should be no conflict

The WKEvent.h already exists in the WebKit2 - Shared/API/c/WKEvent.h, so I use WKEventEfl.h.

>> Source/WebKit2/UIProcess/API/C/efl/WKView.h:56
>> +WK_EXPORT void WKViewHandleTouchEvent(WKViewRef, const WKTouchEvent*);
> 
> WKViewSendTouchEvent(WKViewRef, const WKTouchEvent*); is better I think

Yes, I agree with you.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:959
>> +void EwkView::handleTouchEvent(Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)
> 
> send? as it doesnt return a bool on whether it got handed or not... it seems more correct.
> 
> Maybe, as this is purely conversion, it should be done directly in the ewk C API in ewk_view?

If so, I will move codes to the ewk_view.
Actually, I'm confused the role of ewk_view and EwkView.

[Eunmi Lee]

Created attachment 186764 [details]
Patch

Fixed for comments.

[Eunmi Lee]

Comment on attachment 186600 [details]
Patch

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

>>> Source/WebKit2/Shared/NativeWebTouchEvent.h:50
>>> +    const WKTouchEvent* nativeEvent() const { return &m_nativeEvent; }
>> 
>> It seems strange that the native touch event for EFL is a WK type. Also, this is internal WK2 C++ code. It usually does not rely on C API types.
> 
> Yes, C API types are not usually used in the internal WK2 c++ codes.
> However in our case, we want to use common event type not Evas event type, (Evas does not have type for "touch" event, anyway)
> so we have to define that like WKTouchEvent.
> 
> Additionally, the native event has to be stored because I want to use that when I return the result of touch event to the client of WKView.
> So, I don't want to remove C API dependency from here.
> 
> How about changing the name of event type to EflTouchEvent? I think it can seem strange because the type starts with "WK".

I've thought about this again, I can remove dependency of C API if I do not use native event variable.
and I can make WKTouchEvent from the NativeWebTouchEvent when I return the result of touch event to the client of WKView.
I have to convert again without storing native event, but C API dependency can be removed, so it would be better.
I will update patch for that. :)

[Kenneth Rohde Christiansen]

Comment on attachment 186764 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:46
> +enum EflTouchPointState {

C API Coding style would use EFL not Efl, but let me see you next patch

[Eunmi Lee]

Created attachment 186833 [details]
Patch

Update patch to remove C API dependency.

[Kenneth Rohde Christiansen]

Comment on attachment 186833 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/EFLEvent.h:37
> +enum EFLTouchEventType {

What was the problem with calling it WK ? it is only available from our port. Anyway if using WK is not fine, then I opt for EWK

> Tools/Scripts/webkitpy/style/checker.py:232
>        # struct members. Also, we allow unnecessary parameter names in
>        # WebKit2 APIs because we're matching CF's header style.
> +      # Additionally, we use word which starts with non-capital letter 'k'
> +      # for types of enums.
>        "Source/WebKit2/UIProcess/API/C/",
>        "Source/WebKit2/Shared/API/c/",
>        "Source/WebKit2/WebProcess/InjectedBundle/API/c/"],
> -     ["-readability/naming",
> +     ["-readability/enum_casing",
> +      "-readability/naming",
>        "-readability/parameter_name",

This should be another patch

[Mikhail Pozdnyakov]

Comment on attachment 186833 [details]
Patch

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

> Source/WebKit2/Shared/NativeWebTouchEvent.h:42
> +    explicit NativeWebTouchEvent(const WebTouchEvent&);

NativeWebTouchEvent inherits WebTouchEvent and does not contain anything useful. Seems we don't need it at all.

[Mikhail Pozdnyakov]

Comment on attachment 186833 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/EFLEvent.h:43
> +typedef EFLTouchEventType EFLTouchEventType;

typedef struct EFLTouchEventType EFLTouchEventType;?

> Source/WebKit2/UIProcess/API/C/efl/EFLEvent.h:52
> +typedef EFLTouchPointState EFLTouchPointState;

Ditto.

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:56
> +WK_EXPORT void WKViewSendTouchEvent(WKViewRef, const EFLTouchEvent*);

this really does not look good: it has to be WK_EXPORT void WKViewSendTouchEvent(WKViewRef, WKTouchEventRef);

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:366
> +static EFLTouchEvent* createEFLTouchEvent(Evas_Object* ewkView, Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)

can we find some better place for it? (I've just cleaned up this file, would not like to do it again :))

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:433
> +    EFLTouchEvent* touchEvent = createEFLTouchEvent(ewkView, type, points, modifiers);

OwnPtr might be useful

[Eunmi Lee]

Comment on attachment 186833 [details]
Patch

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

>> Source/WebKit2/Shared/NativeWebTouchEvent.h:42
>> +    explicit NativeWebTouchEvent(const WebTouchEvent&);
> 
> NativeWebTouchEvent inherits WebTouchEvent and does not contain anything useful. Seems we don't need it at all.

We need this because we have to set the NativeWebTouchEvent not WebTouchEvent as a parameter to call the WebPageProxy::handleTouchEvent(const NativeWebTouchEvent& event).

>> Source/WebKit2/UIProcess/API/C/efl/EFLEvent.h:37
>> +enum EFLTouchEventType {
> 
> What was the problem with calling it WK ? it is only available from our port. Anyway if using WK is not fine, then I opt for EWK

I thought that it can be confusing to use WK because this touch event type is only for EFL.
However, it is not matter because it is only available in the EFL port as you said.

so, I decide to change name to WK again.

>> Source/WebKit2/UIProcess/API/C/efl/EFLEvent.h:43
>> +typedef EFLTouchEventType EFLTouchEventType;
> 
> typedef struct EFLTouchEventType EFLTouchEventType;?

oh, I've missed "enum". I will add it.

>> Source/WebKit2/UIProcess/API/C/efl/WKView.h:56
>> +WK_EXPORT void WKViewSendTouchEvent(WKViewRef, const EFLTouchEvent*);
> 
> this really does not look good: it has to be WK_EXPORT void WKViewSendTouchEvent(WKViewRef, WKTouchEventRef);

OK, I will change codes to use WK Ref type.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:366
>> +static EFLTouchEvent* createEFLTouchEvent(Evas_Object* ewkView, Ewk_Touch_Event_Type type, const Eina_List* points, const Evas_Modifier* modifiers)
> 
> can we find some better place for it? (I've just cleaned up this file, would not like to do it again :))

OK, I will consider that :)

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:433
>> +    EFLTouchEvent* touchEvent = createEFLTouchEvent(ewkView, type, points, modifiers);
> 
> OwnPtr might be useful

Yes, I agree.

>> Tools/Scripts/webkitpy/style/checker.py:232
>>        "-readability/parameter_name",
> 
> This should be another patch

OK, I will upload this as a separated patch.

[Mikhail Pozdnyakov]

(In reply to comment #17)
> (From update of attachment 186833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186833&action=review
> 
> >> Source/WebKit2/Shared/NativeWebTouchEvent.h:42
> >> +    explicit NativeWebTouchEvent(const WebTouchEvent&);
> > 
> > NativeWebTouchEvent inherits WebTouchEvent and does not contain anything useful. Seems we don't need it at all.
> 
> We need this because we have to set the NativeWebTouchEvent not WebTouchEvent as a parameter to call the WebPageProxy::handleTouchEvent(const NativeWebTouchEvent& event).
Guess it can be changed to accept const WebTouchEvent&.

[Eunmi Lee]

Comment on attachment 186833 [details]
Patch

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

>>>> Source/WebKit2/Shared/NativeWebTouchEvent.h:42
>>>> +    explicit NativeWebTouchEvent(const WebTouchEvent&);
>>> 
>>> NativeWebTouchEvent inherits WebTouchEvent and does not contain anything useful. Seems we don't need it at all.
>> 
>> We need this because we have to set the NativeWebTouchEvent not WebTouchEvent as a parameter to call the WebPageProxy::handleTouchEvent(const NativeWebTouchEvent& event).
> 
> Guess it can be changed to accept const WebTouchEvent&.

We can not change the parameter type of handleTouchEvent(const NativeWebTouchEvent& event).
Because the event parameter is set to the doneWithTouchEvent(const NativeWebTouchEvent& event, bool wasEventHandled) and event.nativeEvent() is used in the Qt port.
i.e. NativeWebTouchEvent is needed in order to use nativeEvent().

[Mikhail Pozdnyakov]

(In reply to comment #19)
> (From update of attachment 186833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186833&action=review
> 
> >>>> Source/WebKit2/Shared/NativeWebTouchEvent.h:42
> >>>> +    explicit NativeWebTouchEvent(const WebTouchEvent&);
> >>> 
> >>> NativeWebTouchEvent inherits WebTouchEvent and does not contain anything useful. Seems we don't need it at all.
> >> 
> >> We need this because we have to set the NativeWebTouchEvent not WebTouchEvent as a parameter to call the WebPageProxy::handleTouchEvent(const NativeWebTouchEvent& event).
> > 
> > Guess it can be changed to accept const WebTouchEvent&.
> 
> We can not change the parameter type of handleTouchEvent(const NativeWebTouchEvent& event).
> Because the event parameter is set to the doneWithTouchEvent(const NativeWebTouchEvent& event, bool wasEventHandled) and event.nativeEvent() is used in the Qt port.

Exactly, this looks like Qt port-specific thing that we do not need.

[Eunmi Lee]

Comment on attachment 186833 [details]
Patch

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

>>>>>> Source/WebKit2/Shared/NativeWebTouchEvent.h:42
>>>>>> +    explicit NativeWebTouchEvent(const WebTouchEvent&);
>>>>> 
>>>>> NativeWebTouchEvent inherits WebTouchEvent and does not contain anything useful. Seems we don't need it at all.
>>>> 
>>>> We need this because we have to set the NativeWebTouchEvent not WebTouchEvent as a parameter to call the WebPageProxy::handleTouchEvent(const NativeWebTouchEvent& event).
>>> 
>>> Guess it can be changed to accept const WebTouchEvent&.
>> 
>> We can not change the parameter type of handleTouchEvent(const NativeWebTouchEvent& event).
>> Because the event parameter is set to the doneWithTouchEvent(const NativeWebTouchEvent& event, bool wasEventHandled) and event.nativeEvent() is used in the Qt port.
>> i.e. NativeWebTouchEvent is needed in order to use nativeEvent().
> 
> Exactly, this looks like Qt port-specific thing that we do not need.

Yes, the NativeWebTouchEvent is only needed by Qt,
but the events like NativeWebMouseEvent are used by other ports.

So, I don't think it is good way to change to use WebEvent instead of NativeWebEvent only for Touch.

[Eunmi Lee]

Created attachment 187241 [details]
Patch

Update patch for comments.

[Eunmi Lee]

Created attachment 188794 [details]
Patch

Rebased.

[Chris Dumez]

Comment on attachment 188794 [details]
Patch

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

> Source/WebKit2/Shared/API/c/efl/WKBaseEfl.h:28
> +typedef const struct OpaqueWKTouchEventRef* WKTouchEventRef;

"OpaqueWKTouchEventRef" -> "OpaqueWKTouchEvent"

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:65
> +WKTouchEventRef WKTouchEventCreate(WKTouchEvent event)

Shouldn't we take a WKTouchEvent* in argument to avoid copying?

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:106
> +void WKTouchEventRelease(WKTouchEventRef touchEventRef)

All the WKTypes are ref counted. I think it is confusing that WKTouchEventRef isn't. It seems WKTouchEvent is the only WK type having a custom Release function. Shouldn't we have a WebTouchEvent that is an APIObject (similarly to WebRect and WKRectRef / WKRect) ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:383
> +    WKTouchEventRef touchEventRef = WKTouchEventCreate(*touchEvent.get());

We should really use WKRetainPtr IMHO.

[Eunmi Lee]

Comment on attachment 188794 [details]
Patch

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

>> Source/WebKit2/Shared/API/c/efl/WKBaseEfl.h:28
>> +typedef const struct OpaqueWKTouchEventRef* WKTouchEventRef;
> 
> "OpaqueWKTouchEventRef" -> "OpaqueWKTouchEvent"

my mistake. thanks.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:65
>> +WKTouchEventRef WKTouchEventCreate(WKTouchEvent event)
> 
> Shouldn't we take a WKTouchEvent* in argument to avoid copying?

Yes, I agree. the pointer type is better.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:106
>> +void WKTouchEventRelease(WKTouchEventRef touchEventRef)
> 
> All the WKTypes are ref counted. I think it is confusing that WKTouchEventRef isn't. It seems WKTouchEvent is the only WK type having a custom Release function. Shouldn't we have a WebTouchEvent that is an APIObject (similarly to WebRect and WKRectRef / WKRect) ?

Yes. I've also considered about that WebTouchEvent inherits an APIObject.
But I don't know I can change common class - WebTouchEvent only for EFL port. do you think is that OK?

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:383
>> +    WKTouchEventRef touchEventRef = WKTouchEventCreate(*touchEvent.get());
> 
> We should really use WKRetainPtr IMHO.

If WebTouchEvent can inherit APIObject, I can change to use WKRetainPtr.

[Eunmi Lee]

Comment on attachment 188794 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:106
>>> +void WKTouchEventRelease(WKTouchEventRef touchEventRef)
>> 
>> All the WKTypes are ref counted. I think it is confusing that WKTouchEventRef isn't. It seems WKTouchEvent is the only WK type having a custom Release function. Shouldn't we have a WebTouchEvent that is an APIObject (similarly to WebRect and WKRectRef / WKRect) ?
> 
> Yes. I've also considered about that WebTouchEvent inherits an APIObject.
> But I don't know I can change common class - WebTouchEvent only for EFL port. do you think is that OK?

Anyway, I will prepare patch to inherits an APIObject and get comments for that.

[Eunmi Lee]

Created attachment 189221 [details]
Patch

Modified for comments except inheriting APIObject (I will make patch for this again) and add missing ENABLE(TOUCH_EVENTS).

[Eunmi Lee]

Created attachment 190669 [details]
Patch

Change WebTouchEvent to inherit an APIObject.

[Early Warning System Bot]

Comment on attachment 190669 [details]
Patch

Attachment 190669 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16771946

[Eunmi Lee]

Created attachment 190671 [details]
Patch

Fix build break in the qt-wk2

[Chris Dumez]

Comment on attachment 190671 [details]
Patch

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

> Source/WebKit2/Shared/WebEvent.h:330
> +    static PassRefPtr<WebTouchEvent> create(WebEvent::Type type, Vector<WebPlatformTouchPoint> touchPoints, Modifiers modifiers, double timestamp)

I know you followed the existing code but I think the Vector should really be passed by const reference here.

> Source/WebKit2/Shared/WebEvent.h:338
> +    WebTouchEvent(WebEvent::Type, Vector<WebPlatformTouchPoint>, Modifiers, double timestamp);

Ditto.

> Source/WebKit2/Shared/WebEvent.h:340
> +    WebTouchEvent(const WebTouchEvent&);

Adding a copy constructor does not seem right. RefCounted objects (such as APIObjects) are non-copyable by design.

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:73
> +    Vector<WebPlatformTouchPoint> touchPoints;

You could call touchPoints.reserveInitialCapacity(event->numTouchPoints); right after that since we already know the number of touch points.

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:74
> +    WebPlatformTouchPoint::TouchPointState state;

This could be declared in the for loop scope.

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:78
> +        switch (point.state) {

Feels like this should be in a separate function to improve readability (the switch). e.g. toWebPlatformTouchPointState().

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:99
> +        touchPoints.append(WebPlatformTouchPoint(point.id, state, toIntPoint(point.screenPosition), toIntPoint(point.position), toIntSize(point.radius), point.rotationAngle, point.force));

You could use WTF::Vector::uncheckedAppend() here to save a few capacity checks (provided that you call reserveInitialCapacity() earlier).

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:102
> +    RefPtr<WebTouchEvent> webTouchEvent = WebTouchEvent::create(typeForEvent(event->type), touchPoints, modifiersForWKEvent(event->modifiers), event->timestamp);

Useless local variable: you can return toAPI(WebTouchEvent::create(...).leakRef()) directly.

> Source/WebKit2/UIProcess/API/efl/ewk_touch.cpp:57
> +    double x;

It seems this one could be in the for loop scope.

> Source/WebKit2/UIProcess/API/efl/ewk_touch.cpp:58
> +    double y;

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_touch_private.h:35
> +class AffineTransform;

I thought we stopped using WebCore types in our Ewk code?

[Gyuyoung Kim]

Comment on attachment 190671 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:38
> +    if (type == kWKEventTypeTouchStart)

Isn't possibility to use switch ?

[Dominik Röttsches (drott)]

Comment on attachment 190671 [details]
Patch

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

> Source/WebKit2/Shared/WebEvent.cpp:36
> +    : m_eventType(static_cast<uint32_t>(NoType))

I would suggest to split the patch in two pieces, and find out whether we can get the enabling change landed. So it would be: One patch for the type() -> eventType() renaming, one for the second part of actually exporting WebEvent as an ApiObject. When creating the first patch the ChangeLog would need to describe the rationale/plan to export WebEvent as an ApiObject in order to be able to send touch and other events to the view. Eunmi, what do you think?

[Mikhail Pozdnyakov]

Comment on attachment 190671 [details]
Patch

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

>> Source/WebKit2/Shared/WebEvent.h:340
>> +    WebTouchEvent(const WebTouchEvent&);
> 
> Adding a copy constructor does not seem right. RefCounted objects (such as APIObjects) are non-copyable by design.

Agree with Chris, RefCounted objects are supposed to be created on heap and passed via smart pointers. Practically you won't need copy constructor

[Mikhail Pozdnyakov]

(In reply to comment #34)
> (From update of attachment 190671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190671&action=review
> 
> >> Source/WebKit2/Shared/WebEvent.h:340
> >> +    WebTouchEvent(const WebTouchEvent&);
> > 
> > Adding a copy constructor does not seem right. RefCounted objects (such as APIObjects) are non-copyable by design.
> 
> Agree with Chris, RefCounted objects are supposed to be created on heap and passed via smart pointers. Practically you won't need copy constructor

And copying of ref count is not definitely desired behaviour :)

[Mikhail Pozdnyakov]

Comment on attachment 190671 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:75
> +typedef struct WKTouchEvent WKTouchEvent;

Could you please tell why we need this structure. Cannot we just pass every structure member simply as a parameter for WKTouchEventCreate?

[Eunmi Lee]

Comment on attachment 190671 [details]
Patch

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

Thanks for comments :)

>> Source/WebKit2/Shared/WebEvent.cpp:36
>> +    : m_eventType(static_cast<uint32_t>(NoType))
> 
> I would suggest to split the patch in two pieces, and find out whether we can get the enabling change landed. So it would be: One patch for the type() -> eventType() renaming, one for the second part of actually exporting WebEvent as an ApiObject. When creating the first patch the ChangeLog would need to describe the rationale/plan to export WebEvent as an ApiObject in order to be able to send touch and other events to the view. Eunmi, what do you think?

I agree with you. This patch became bigger and includes many things, so it is better to split, especially renaming of type().

>> Source/WebKit2/Shared/WebEvent.h:330
>> +    static PassRefPtr<WebTouchEvent> create(WebEvent::Type type, Vector<WebPlatformTouchPoint> touchPoints, Modifiers modifiers, double timestamp)
> 
> I know you followed the existing code but I think the Vector should really be passed by const reference here.

yes right. I will fix that.

>>>> Source/WebKit2/Shared/WebEvent.h:340
>>>> +    WebTouchEvent(const WebTouchEvent&);
>>> 
>>> Adding a copy constructor does not seem right. RefCounted objects (such as APIObjects) are non-copyable by design.
>> 
>> Agree with Chris, RefCounted objects are supposed to be created on heap and passed via smart pointers. Practically you won't need copy constructor
> 
> And copying of ref count is not definitely desired behaviour :)

It is difficult issue to me.

The copy constructor of WebTouchEvent is needed in the WebPageProxy::handleTouchEvent() function.
If WebTouchEvent inherits APIObject, the default copy constructor of WebTouchEvent is called and the copy constructor of APIObject is also required.
However, the APIObject is non-copyable because it is RefCounted object as you said.

So, I've made explicit copy constructor of WebTouchEvent which does not require the copy constructor of APIObject.
Actually, I think it is not a good solution.

How can I solve this issue?

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:38
>> +    if (type == kWKEventTypeTouchStart)
> 
> Isn't possibility to use switch ?

Sure.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:73
>> +    Vector<WebPlatformTouchPoint> touchPoints;
> 
> You could call touchPoints.reserveInitialCapacity(event->numTouchPoints); right after that since we already know the number of touch points.

Yes, right.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:74
>> +    WebPlatformTouchPoint::TouchPointState state;
> 
> This could be declared in the for loop scope.

OK.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:78
>> +        switch (point.state) {
> 
> Feels like this should be in a separate function to improve readability (the switch). e.g. toWebPlatformTouchPointState().

I agree with you.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:99
>> +        touchPoints.append(WebPlatformTouchPoint(point.id, state, toIntPoint(point.screenPosition), toIntPoint(point.position), toIntSize(point.radius), point.rotationAngle, point.force));
> 
> You could use WTF::Vector::uncheckedAppend() here to save a few capacity checks (provided that you call reserveInitialCapacity() earlier).

I see, thanks for information. :)

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:102
>> +    RefPtr<WebTouchEvent> webTouchEvent = WebTouchEvent::create(typeForEvent(event->type), touchPoints, modifiersForWKEvent(event->modifiers), event->timestamp);
> 
> Useless local variable: you can return toAPI(WebTouchEvent::create(...).leakRef()) directly.

Yes, right.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.h:75
>> +typedef struct WKTouchEvent WKTouchEvent;
> 
> Could you please tell why we need this structure. Cannot we just pass every structure member simply as a parameter for WKTouchEventCreate?

It is useful to return TouchEvent values to the application :)
You can see that in the Bug 110085's WKTouchEventGetValue.

>> Source/WebKit2/UIProcess/API/efl/ewk_touch.cpp:57
>> +    double x;
> 
> It seems this one could be in the for loop scope.

OK, I will move them.

>> Source/WebKit2/UIProcess/API/efl/ewk_touch_private.h:35
>> +class AffineTransform;
> 
> I thought we stopped using WebCore types in our Ewk code?

OK, I have to think how to convert points without AffineTransform.

[Dominik Röttsches (drott)]

(In reply to comment #37)

> > And copying of ref count is not definitely desired behaviour :)
> 
> It is difficult issue to me.
> 
> The copy constructor of WebTouchEvent is needed in the WebPageProxy::handleTouchEvent() function.
> If WebTouchEvent inherits APIObject, the default copy constructor of WebTouchEvent is called and the copy constructor of APIObject is also required.
> However, the APIObject is non-copyable because it is RefCounted object as you said.
> 
> So, I've made explicit copy constructor of WebTouchEvent which does not require the copy constructor of APIObject.
> Actually, I think it is not a good solution.
> 
> How can I solve this issue?

As discussed on IRC, the underlying issue is that WebPageProxy needs to copy the touch events and buffer them in a queue for deferred sending to WebProcess. 

At the same time, in your patch, NativeWebTouchEvent is reusing WebTouchEvent for storage, which becomes non-copyable when exposing it as APIObject. As a solution, I would suggest to introduce our own storage class, e.g. EflTouchEvent. This one would fulfil a similar purpose as QTouchEvent or the structure that you already had there WKTouchEvent. IMO this would make the patch more consistent with other parts of the implementation.

[Mikhail Pozdnyakov]

(In reply to comment #38)
> (In reply to comment #37)
> 
> > > And copying of ref count is not definitely desired behaviour :)
> > 
> > It is difficult issue to me.
> > 
> > The copy constructor of WebTouchEvent is needed in the WebPageProxy::handleTouchEvent() function.
> > If WebTouchEvent inherits APIObject, the default copy constructor of WebTouchEvent is called and the copy constructor of APIObject is also required.
> > However, the APIObject is non-copyable because it is RefCounted object as you said.
> > 
> > So, I've made explicit copy constructor of WebTouchEvent which does not require the copy constructor of APIObject.
> > Actually, I think it is not a good solution.
> > 
> > How can I solve this issue?
> 
> As discussed on IRC, the underlying issue is that WebPageProxy needs to copy the touch events and buffer them in a queue for deferred sending to WebProcess. 
> 
> At the same time, in your patch, NativeWebTouchEvent is reusing WebTouchEvent for storage, which becomes non-copyable when exposing it as APIObject. As a solution, I would suggest to introduce our own storage class, e.g. EflTouchEvent. This one would fulfil a similar purpose as QTouchEvent or the structure that you already had there WKTouchEvent. IMO this would make the patch more consistent with other parts of the implementation.

Maybe WebTouchEvent can be used some other way within WebPageProxy::handleTouchEvent()? As far as I see copy constructor is used for storing events in a queue, maybe this queue can keep shared pointers instead?

[Eunmi Lee]

Created attachment 192232 [details]
WIP

WIP

[Kenneth Rohde Christiansen]

Comment on attachment 192232 [details]
WIP

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

> Source/WebKit2/Shared/WebPlatformTouchPoint.h:46
> +// FIXME: Having "Platform" in the name makes it sound like this event is platform-specific or low-
> +// level in some way. That doesn't seem to be the case.
> +class WebPlatformTouchPoint : public APIObject {

Seems you should just rename it! And moving it out and doing the rename should be a separate patch for the rest of your patch

[Eunmi Lee]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > 
> > > > And copying of ref count is not definitely desired behaviour :)
> > > 
> > > It is difficult issue to me.
> > > 
> > > The copy constructor of WebTouchEvent is needed in the WebPageProxy::handleTouchEvent() function.
> > > If WebTouchEvent inherits APIObject, the default copy constructor of WebTouchEvent is called and the copy constructor of APIObject is also required.
> > > However, the APIObject is non-copyable because it is RefCounted object as you said.
> > > 
> > > So, I've made explicit copy constructor of WebTouchEvent which does not require the copy constructor of APIObject.
> > > Actually, I think it is not a good solution.
> > > 
> > > How can I solve this issue?
> > 
> > As discussed on IRC, the underlying issue is that WebPageProxy needs to copy the touch events and buffer them in a queue for deferred sending to WebProcess. 
> > 
> > At the same time, in your patch, NativeWebTouchEvent is reusing WebTouchEvent for storage, which becomes non-copyable when exposing it as APIObject. As a solution, I would suggest to introduce our own storage class, e.g. EflTouchEvent. This one would fulfil a similar purpose as QTouchEvent or the structure that you already had there WKTouchEvent. IMO this would make the patch more consistent with other parts of the implementation.

I've tried to solve "copy constructor" problem by change type of storage from WebTouchEvent to EFLTouchEvent, but I've failed because NativeWebTouchEvent already use WebTouchEvent as its parent.
Go back to the beginning,
I think the problem is that we tried to make WebTouchEvent to inherit APIObject which is non-copyable.
So, I'm going to prepare new patch which does not make WebTouchEvent to APIObject and make EFLTouchEvent to inherit APIObject instead.

> 
> Maybe WebTouchEvent can be used some other way within WebPageProxy::handleTouchEvent()? As far as I see copy constructor is used for storing events in a queue, maybe this queue can keep shared pointers instead?

Yes, WebTouchEvent can be used in the handleTouchEvent(), so I will prepare patch as I describe above.
I think it is not easy to make queue to keep shared pointers because handleTouchEvent's parameter is reference of NativeWebTouchEvent which does not a shared pointer type,
and we have to change many things to return shared pointer of NativeWebTouchEvent instead of reference of that.

[Eunmi Lee]

(In reply to comment #41)
> (From update of attachment 192232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192232&action=review
> 
> > Source/WebKit2/Shared/WebPlatformTouchPoint.h:46
> > +// FIXME: Having "Platform" in the name makes it sound like this event is platform-specific or low-
> > +// level in some way. That doesn't seem to be the case.
> > +class WebPlatformTouchPoint : public APIObject {
> 
> Seems you should just rename it! And moving it out and doing the rename should be a separate patch for the rest of your patch

OK, I will prepare separate patch for that.

[Eunmi Lee]

(In reply to comment #43)
> (In reply to comment #41)
> > (From update of attachment 192232 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=192232&action=review
> > 
> > > Source/WebKit2/Shared/WebPlatformTouchPoint.h:46
> > > +// FIXME: Having "Platform" in the name makes it sound like this event is platform-specific or low-
> > > +// level in some way. That doesn't seem to be the case.
> > > +class WebPlatformTouchPoint : public APIObject {
> > 
> > Seems you should just rename it! And moving it out and doing the rename should be a separate patch for the rest of your patch
> 
> OK, I will prepare separate patch for that.

Kenneth,
I encountered a copy constructor problem when I make WebPlatformTouchPoint to inherit APIObject like WebTouchEvent,
and I've defined EFLTouchPoint and make it to inherit APIObject instead of WebPlatformTouchPoint.
also, I don't have to make new patch for changing WebPlatformTouchPoint.

Please comment for new patch :)

[Eunmi Lee]

Created attachment 192913 [details]
Patch

Make new patch by adding EFLTouchEvent and EFLTouchPoint.

[Eunmi Lee]

Created attachment 193064 [details]
Patch

Rebased and Modified wrong UNUSED_PARAM of WKTouchEventCreate.

[Kenneth Rohde Christiansen]

Comment on attachment 193064 [details]
Patch

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

> Source/WebKit2/Shared/NativeWebTouchEvent.h:53
>      const QTouchEvent* nativeEvent() const { return &m_nativeEvent; }
> +#elif PLATFORM(EFL)
> +    const EFLTouchEvent* nativeEvent() const { return m_nativeEvent.get(); }
>  #endif

Why do *we* need this nativeEvent() method at all?

> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:45
> +WK_ADD_API_MAPPING(WKTouchEventRef, EFLTouchEvent)
> +WK_ADD_API_MAPPING(WKTouchPointRef, EFLTouchPoint)

Maybe EWKTouchEvent or EWebTouchEvent would make more sense

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:738
> +    const unsigned count = eina_list_count(points);

length  = ...

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:742
> +        Ewk_Touch_Point* point = static_cast<Ewk_Touch_Point*>(eina_list_nth(points, i));
> +        EINA_SAFETY_ON_NULL_RETURN(point);

I dont see the point of using these EINA_ ...

When can this even happen? You already get the length above. Maybe ASSERT(point) makes more sense

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:744
> +        touchPoints[i] = WKTouchPointCreate(point->id, toAPI(IntPoint(position)), toAPI(transformToScreen().mapPoint(position)), toWKTouchPointState(point->state), WKSizeMake(0, 0), 0, 0);

force == 0 seems wrong...no force at all. why not 1?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1272
> +    OwnArrayPtr<WKTypeRef> touchPoints = adoptArrayPtr(new WKTypeRef[count]);

I prefer length over count

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1279
> +        int id = evas_touch_point_list_nth_id_get(sd->base.evas, i);
> +        int x, y;
> +        evas_touch_point_list_nth_xy_get(sd->base.evas, i, &x, &y);
> +        Evas_Touch_Point_State state = evas_touch_point_list_nth_state_get(sd->base.evas, i);
> +        IntPoint position(x, y);
> +        touchPoints[i] = WKTouchPointCreate(id, toAPI(IntPoint(position)), toAPI(transformToScreen().mapPoint(position)), toWKTouchPointState(state), WKSizeMake(0, 0), 0, 0);

why not do the id = above the last line where you actually use it. Actually reorder things to where they are used... same thing with position vs state

> Source/WebKit2/UIProcess/efl/EFLTouchPoint.h:57
> +    EFLTouchPoint(uint32_t id, WKTouchPointState, const WKPoint&, const WKPoint&, const WKSize&, float rotationAngle, float force);

forceFactor ?

[Eunmi Lee]

Comment on attachment 193064 [details]
Patch

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

Thank you for comments!

>> Source/WebKit2/Shared/NativeWebTouchEvent.h:53
>>  #endif
> 
> Why do *we* need this nativeEvent() method at all?

Because we have to return EFLTouchEvent to the EwkView in the doneWithTouchEvent() - Bug 110085.
So, we maintain the EFLTouchEvent as m_nativeEvent and it will be used to make gesture in the EwkView.

>> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:45
>> +WK_ADD_API_MAPPING(WKTouchPointRef, EFLTouchPoint)
> 
> Maybe EWKTouchEvent or EWebTouchEvent would make more sense

Yes, I will change name to EwkTouchEvent like EwkView as you told in IRC :)

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:738
>> +    const unsigned count = eina_list_count(points);
> 
> length  = ...

OK.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:742
>> +        EINA_SAFETY_ON_NULL_RETURN(point);
> 
> I dont see the point of using these EINA_ ...
> 
> When can this even happen? You already get the length above. Maybe ASSERT(point) makes more sense

Yes, the point should not be null here. I will change to ASSERT.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:744
>> +        touchPoints[i] = WKTouchPointCreate(point->id, toAPI(IntPoint(position)), toAPI(transformToScreen().mapPoint(position)), toWKTouchPointState(point->state), WKSizeMake(0, 0), 0, 0);
> 
> force == 0 seems wrong...no force at all. why not 1?

Yes, right. it should have at least 1.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1272
>> +    OwnArrayPtr<WKTypeRef> touchPoints = adoptArrayPtr(new WKTypeRef[count]);
> 
> I prefer length over count

Yes, I will change it.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1279
>> +        touchPoints[i] = WKTouchPointCreate(id, toAPI(IntPoint(position)), toAPI(transformToScreen().mapPoint(position)), toWKTouchPointState(state), WKSizeMake(0, 0), 0, 0);
> 
> why not do the id = above the last line where you actually use it. Actually reorder things to where they are used... same thing with position vs state

OK, I will reorder things for readability :)

>> Source/WebKit2/UIProcess/efl/EFLTouchPoint.h:57
>> +    EFLTouchPoint(uint32_t id, WKTouchPointState, const WKPoint&, const WKPoint&, const WKSize&, float rotationAngle, float force);
> 
> forceFactor ?

?? I will change the default value of force to 1 :)

[Eunmi Lee]

Created attachment 193543 [details]
Patch

Update for comments.

[Kenneth Rohde Christiansen]

Comment on attachment 193543 [details]
Patch

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

LGTM

> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:37
> +WKTouchPointRef WKTouchPointCreate(int id, WKPoint position, WKPoint screenPosition, WKTouchPointState state, WKSize radius, float rotationAngle, float force)

I would still s/force/forceFactor/

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1294
> +        evas_touch_point_list_nth_xy_get(sd->base.evas, i, &x, &y);
> +        int id = evas_touch_point_list_nth_id_get(sd->base.evas, i);
> +        IntPoint position(x, y);
> +        Evas_Touch_Point_State state = evas_touch_point_list_nth_state_get(sd->base.evas, i);
> +        touchPoints[i] = WKTouchPointCreate(id, toAPI(IntPoint(position)), toAPI(transformToScreen().mapPoint(position)), toWKTouchPointState(state), WKSizeMake(0, 0), 0, 1);

I would still move id down two lines

[Mikhail Pozdnyakov]

Comment on attachment 193543 [details]
Patch

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

> Source/WebKit2/Shared/NativeWebTouchEvent.h:46
> +    NativeWebTouchEvent(EwkTouchEvent*, const WebCore::AffineTransform&);

guess could be const EwkTouchEvent*

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:753
> +    const unsigned length = eina_list_count(points);

const should be omitted.

[Eunmi Lee]

Comment on attachment 193543 [details]
Patch

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

>> Source/WebKit2/Shared/NativeWebTouchEvent.h:46
>> +    NativeWebTouchEvent(EwkTouchEvent*, const WebCore::AffineTransform&);
> 
> guess could be const EwkTouchEvent*

It is not const because EwkTouchEvent* is changed in the constructor in order to increase reference count.

>> Source/WebKit2/UIProcess/API/C/efl/WKEventEfl.cpp:37
>> +WKTouchPointRef WKTouchPointCreate(int id, WKPoint position, WKPoint screenPosition, WKTouchPointState state, WKSize radius, float rotationAngle, float force)
> 
> I would still s/force/forceFactor/

Ah, I see. you want more understandable name :)
I will change the name to forceFactor.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:753
>> +    const unsigned length = eina_list_count(points);
> 
> const should be omitted.

OK, I will remove that.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1294
>> +        touchPoints[i] = WKTouchPointCreate(id, toAPI(IntPoint(position)), toAPI(transformToScreen().mapPoint(position)), toWKTouchPointState(state), WKSizeMake(0, 0), 0, 1);
> 
> I would still move id down two lines

OK, I will move id.

[Eunmi Lee]

Created attachment 193733 [details]
Patch

Modified for comments.

[Kenneth Rohde Christiansen]

Comment on attachment 193733 [details]
Patch

LGTM

[Mikhail Pozdnyakov]

LGTM too, now I guess we need approval from a WK2 owner.

[Eunmi Lee]

Created attachment 199797 [details]
Patch

Rebased.

[Eunmi Lee]

Created attachment 205562 [details]
Patch

Rebased.

[Gyuyoung Kim]

Comment on attachment 205562 [details]
Patch

This patch only touches two WK2 common files(NativeWebTouchEvent.h and APIObject.h). However, those are just update inside #if PLATFORM(EFL) ~ #endif. So, I believe this patch is for EFL specific patch. r+ based on informal review.

[Gyuyoung Kim]

Benjamin, if you think this patch should be reviewed or signed off, please let me know.

[Benjamin Poulain]

(In reply to comment #59)
> Benjamin, if you think this patch should be reviewed or signed off, please let me know.

I did not check the code in detail but I am completely okay with the architecture of this.

[Gyuyoung Kim]

(In reply to comment #60)
> (In reply to comment #59)
> > Benjamin, if you think this patch should be reviewed or signed off, please let me know.
> 
> I did not check the code in detail but I am completely okay with the architecture of this.

Thanks.

[WebKit Commit Bot]

Comment on attachment 205562 [details]
Patch

Clearing flags on attachment: 205562

Committed r152147: <http://trac.webkit.org/changeset/152147>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.