Bug 108915

Summary: [EFL][WK2] Add WK2 C API to handle touch events.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit EFLAssignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, andersca, benjamin, cdumez, cmarcelo, commit-queue, dpranke, d-r, eric, gyuyoung.kim, gyuyoung.kim, hausmann, kenneth, levin, lucas.de.marchi, menard, mikhail.pozdnyakov, rakuco, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109128    
Bug Blocks: 107662, 110085, 111543, 111702    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Eunmi Lee 2013-02-05 00:13:55 PST
Add WKViewHandleTouchEvent() API for handling touch event in order to use C WK2 API and not C++ internals.
Comment 1 Eunmi Lee 2013-02-05 04:25:27 PST
Created attachment 186596 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-05 04:27:49 PST
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.
Comment 3 Eunmi Lee 2013-02-05 04:44:37 PST
Created attachment 186600 [details]
Patch

Modify checker.py for style error.
Comment 4 Kenneth Rohde Christiansen 2013-02-05 04:58:27 PST
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?
Comment 5 Mikhail Pozdnyakov 2013-02-05 05:03:57 PST
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?
Comment 6 Chris Dumez 2013-02-05 05:06:42 PST
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.
Comment 7 Chris Dumez 2013-02-05 05:38:51 PST
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)
Comment 8 Eunmi Lee 2013-02-05 20:25:27 PST
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.
Comment 9 Eunmi Lee 2013-02-05 20:38:33 PST
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.
Comment 10 Eunmi Lee 2013-02-05 23:33:29 PST
Created attachment 186764 [details]
Patch

Fixed for comments.
Comment 11 Eunmi Lee 2013-02-06 00:06:55 PST
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. :)
Comment 12 Kenneth Rohde Christiansen 2013-02-06 00:32:11 PST
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
Comment 13 Eunmi Lee 2013-02-06 04:58:00 PST
Created attachment 186833 [details]
Patch

Update patch to remove C API dependency.
Comment 14 Kenneth Rohde Christiansen 2013-02-06 05:17:53 PST
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
Comment 15 Mikhail Pozdnyakov 2013-02-06 06:34:24 PST
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.
Comment 16 Mikhail Pozdnyakov 2013-02-06 06:42:34 PST
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
Comment 17 Eunmi Lee 2013-02-06 19:51:03 PST
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.
Comment 18 Mikhail Pozdnyakov 2013-02-07 00:34:48 PST
(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&.
Comment 19 Eunmi Lee 2013-02-07 03:06:09 PST
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().
Comment 20 Mikhail Pozdnyakov 2013-02-07 04:42:41 PST
(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.
Comment 21 Eunmi Lee 2013-02-07 04:58:33 PST
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.
Comment 22 Eunmi Lee 2013-02-07 22:53:58 PST
Created attachment 187241 [details]
Patch

Update patch for comments.
Comment 23 Eunmi Lee 2013-02-17 21:39:41 PST
Created attachment 188794 [details]
Patch

Rebased.
Comment 24 Chris Dumez 2013-02-19 00:55:13 PST
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.
Comment 25 Eunmi Lee 2013-02-19 18:07:11 PST
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.
Comment 26 Eunmi Lee 2013-02-19 18:28:54 PST
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.
Comment 27 Eunmi Lee 2013-02-19 18:32:55 PST
Created attachment 189221 [details]
Patch

Modified for comments except inheriting APIObject (I will make patch for this again) and add missing ENABLE(TOUCH_EVENTS).
Comment 28 Eunmi Lee 2013-02-28 00:31:28 PST
Created attachment 190669 [details]
Patch

Change WebTouchEvent to inherit an APIObject.
Comment 29 Early Warning System Bot 2013-02-28 00:39:45 PST
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
Comment 30 Eunmi Lee 2013-02-28 00:45:07 PST
Created attachment 190671 [details]
Patch

Fix build break in the qt-wk2
Comment 31 Chris Dumez 2013-03-07 01:28:49 PST
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?
Comment 32 Gyuyoung Kim 2013-03-07 01:58:51 PST
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 ?
Comment 33 Dominik Röttsches (drott) 2013-03-07 05:02:50 PST
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?
Comment 34 Mikhail Pozdnyakov 2013-03-07 06:07:39 PST
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
Comment 35 Mikhail Pozdnyakov 2013-03-07 06:13:44 PST
(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 :)
Comment 36 Mikhail Pozdnyakov 2013-03-07 06:16:27 PST
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?
Comment 37 Eunmi Lee 2013-03-08 01:00:43 PST
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.
Comment 38 Dominik Röttsches (drott) 2013-03-08 04:45:32 PST
(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.
Comment 39 Mikhail Pozdnyakov 2013-03-08 05:03:47 PST
(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?
Comment 40 Eunmi Lee 2013-03-08 08:22:29 PST
Created attachment 192232 [details]
WIP

WIP
Comment 41 Kenneth Rohde Christiansen 2013-03-08 08:26:08 PST
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
Comment 42 Eunmi Lee 2013-03-10 23:57:29 PDT
(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.
Comment 43 Eunmi Lee 2013-03-10 23:58:25 PDT
(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.
Comment 44 Eunmi Lee 2013-03-13 05:55:39 PDT
(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 :)
Comment 45 Eunmi Lee 2013-03-13 06:28:23 PDT
Created attachment 192913 [details]
Patch

Make new patch by adding EFLTouchEvent and EFLTouchPoint.
Comment 46 Eunmi Lee 2013-03-13 21:58:46 PDT
Created attachment 193064 [details]
Patch

Rebased and Modified wrong UNUSED_PARAM of WKTouchEventCreate.
Comment 47 Kenneth Rohde Christiansen 2013-03-18 03:18:16 PDT
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 ?
Comment 48 Eunmi Lee 2013-03-18 05:22:14 PDT
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 :)
Comment 49 Eunmi Lee 2013-03-18 05:54:46 PDT
Created attachment 193543 [details]
Patch

Update for comments.
Comment 50 Kenneth Rohde Christiansen 2013-03-18 06:03:03 PDT
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
Comment 51 Mikhail Pozdnyakov 2013-03-18 06:15:26 PDT
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.
Comment 52 Eunmi Lee 2013-03-18 22:09:24 PDT
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.
Comment 53 Eunmi Lee 2013-03-18 22:38:00 PDT
Created attachment 193733 [details]
Patch

Modified for comments.
Comment 54 Kenneth Rohde Christiansen 2013-03-19 05:04:56 PDT
Comment on attachment 193733 [details]
Patch

LGTM
Comment 55 Mikhail Pozdnyakov 2013-03-20 02:16:21 PDT
LGTM too, now I guess we need approval from a WK2 owner.
Comment 56 Eunmi Lee 2013-04-26 01:18:18 PDT
Created attachment 199797 [details]
Patch

Rebased.
Comment 57 Eunmi Lee 2013-06-26 22:15:18 PDT
Created attachment 205562 [details]
Patch

Rebased.
Comment 58 Gyuyoung Kim 2013-06-26 22:29:25 PDT
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.
Comment 59 Gyuyoung Kim 2013-06-26 22:30:49 PDT
Benjamin, if you think this patch should be reviewed or signed off, please let me know.
Comment 60 Benjamin Poulain 2013-06-27 12:54:08 PDT
(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.
Comment 61 Gyuyoung Kim 2013-06-27 18:34:21 PDT
(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.
Comment 62 WebKit Commit Bot 2013-06-27 18:54:08 PDT
Comment on attachment 205562 [details]
Patch

Clearing flags on attachment: 205562

Committed r152147: <http://trac.webkit.org/changeset/152147>
Comment 63 WebKit Commit Bot 2013-06-27 18:54:16 PDT
All reviewed patches have been landed.  Closing bug.