Bug 110085 – [EFL][WK2] Add doneWithTouchEvent callback to the WKViewClient.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Eunmi Lee]

Add doneWithTouchEvent callback to the WKViewClient.

[Eunmi Lee]

Created attachment 188814 [details]
Patch

[EFL EWS Bot]

Comment on attachment 188814 [details]
Patch

Attachment 188814 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16604860

[Chris Dumez]

Comment on attachment 188814 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKView.h:41
> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEvent touchEvent, bool wasEventHandled, const void* clientInfo);

Shouldn't we pass a WKTouchEventRef instead of a WKTouchEvent? Since this is copied, it feels like not using a pointer will likely be inefficient.

[Gyuyoung Kim]

Comment on attachment 188814 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:42
> +    if (type == WebEvent::TouchStart)

Isn't it better to use switch ~ case instead of if ~ else ?  IMHO, switch ~ case is a little more efficient for compiler generally. Besides it seems to me switch ~ case is better readability. 

There are some related threads in internet as below,
http://stackoverflow.com/questions/97987/advantage-of-switch-over-if-else-statement

[Eunmi Lee]

Comment on attachment 188814 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:42
>> +    if (type == WebEvent::TouchStart)
> 
> Isn't it better to use switch ~ case instead of if ~ else ?  IMHO, switch ~ case is a little more efficient for compiler generally. Besides it seems to me switch ~ case is better readability. 
> 
> There are some related threads in internet as below,
> http://stackoverflow.com/questions/97987/advantage-of-switch-over-if-else-statement

OK, I will use switch ~ case statement.

>> Source/WebKit2/UIProcess/API/C/efl/WKView.h:41
>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEvent touchEvent, bool wasEventHandled, const void* clientInfo);
> 
> Shouldn't we pass a WKTouchEventRef instead of a WKTouchEvent? Since this is copied, it feels like not using a pointer will likely be inefficient.

Yes, right. so I will use WKTouchEventRef.
And, I will add WKTouchEventGetValue() to get WKTouchEvent from WKTouchEventRef because WKTouchEventRef is opaque to users of WKViewClient.

[Eunmi Lee]

Created attachment 191184 [details]
Patch

Update for comments.

[Eunmi Lee]

Created attachment 193077 [details]
Patch

Rebased for changed Bug 108915.

[Eunmi Lee]

Created attachment 193742 [details]
Patch

Rebased.

[Eunmi Lee]

Created attachment 206049 [details]
Patch

Rebased.

[Gyuyoung Kim]

Comment on attachment 206049 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43
> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo);

I think you need to get a review from coodinated graphics folks, probably Noam ?

> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:93
> +        return kWKTouchPointStateTouchReleased;

Don't you need to use COMPILE_ASSERT_MATCHING_ENUM for kWKTouchPointStateTouchXXX ?

[Noam Rosenthal]

Comment on attachment 206049 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43
>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo);
> 
> I think you need to get a review from coodinated graphics folks, probably Noam ?

Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think?

[Eunmi Lee]

Comment on attachment 206049 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43
>>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo);
>> 
>> I think you need to get a review from coodinated graphics folks, probably Noam ?
> 
> Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think?

I think NIX also needs doneWithTouchEventCallback, I'm waiting for Luciano's answer.

>> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:93
>> +        return kWKTouchPointStateTouchReleased;
> 
> Don't you need to use COMPILE_ASSERT_MATCHING_ENUM for kWKTouchPointStateTouchXXX ?

I think it is better to use switch~case statement here instead of COMPILE_ASSERT_MATCHING_ENUM for consistency,
because we usually use swtich~case statement in the WKAPICast.

[Luciano Wolf]

Comment on attachment 206049 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43
>>>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo);
>>> 
>>> I think you need to get a review from coodinated graphics folks, probably Noam ?
>> 
>> Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think?
> 
> I think NIX also needs doneWithTouchEventCallback, I'm waiting for Luciano's answer.

As Nix has a doneWithTouchEventCallback on its NixViewClent (inside NixView.h) it would be great to have it shared between our ports. Although, WKTouchEventRef declared this way will break the compilation.

[Mikhail Pozdnyakov]

Comment on attachment 206049 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:102
> +        return kWKTouchPointStateTouchCancelled;

Is that expected? maybe assertion?

[Eunmi Lee]

Comment on attachment 206049 [details]
Patch

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

>>>>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:43
>>>>> +typedef void (*WKViewDoneWithTouchEventCallback)(WKViewRef view, WKTouchEventRef touchEvent, bool wasEventHandled, const void* clientInfo);
>>>> 
>>>> I think you need to get a review from coodinated graphics folks, probably Noam ?
>>> 
>>> Not sure if this should be in the common WKView or just keep it in the EFL extension. Luciano, what do you think?
>> 
>> I think NIX also needs doneWithTouchEventCallback, I'm waiting for Luciano's answer.
> 
> As Nix has a doneWithTouchEventCallback on its NixViewClent (inside NixView.h) it would be great to have it shared between our ports. Although, WKTouchEventRef declared this way will break the compilation.

We can avoid to break the compilation of NIX if WKTouchEventRef is defined as EFL port.

I've defined WKTouchEventRef for EFL as follows:
In the API/C/efl/WKAPICastEfl.h,
WK_ADD_API_MAPPING(WKTouchEventRef, EwkTouchEvent)
WK_ADD_API_MAPPING(WKTouchPointRef, EwkTouchPoint)
In the Shared/API/c/efl/WKBaseEfl.h,
typedef const struct OpaqueWKTouchEvent* WKTouchEventRef;

>> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:102
>> +        return kWKTouchPointStateTouchCancelled;
> 
> Is that expected? maybe assertion?

We should not reach here, so I will add ASSERT_NOT_REACHED().

[Eunmi Lee]

Created attachment 206292 [details]
Patch

Rebase and add ASSERT.

[Ryuan Choi]

Comment on attachment 206292 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:74
> +inline WKEventType toAPI(WebEvent::Type type)
> +{
> +    switch (type) {
> +#if ENABLE(TOUCH_EVENTS)
> +    case WebEvent::TouchStart:

Whole function should be wrapped by TOUCH_EVENTS, isn't it?

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:132
> +    if (!m_client.doneWithTouchEvent)
> +        return;
> +
> +#if PLATFORM(EFL)
> +    m_client.doneWithTouchEvent(toAPI(view), toAPI(const_cast<EwkTouchEvent*>(event.nativeEvent())), wasEventHandled, m_client.clientInfo);
> +#else
> +    notImplemented();
> +    UNUSED_PARAM(view);
> +    UNUSED_PARAM(event);
> +    UNUSED_PARAM(wasEventHandled);
> +#endif

For other platforms, 122-123 line are unnecessary.

> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:167
> +void ViewClientEfl::doneWithTouchEvent(WKViewRef, WKTouchEventRef event, bool wasEventHandled, const void* clientInfo)
> +{
> +#if ENABLE(TOUCH_EVENTS)
> +    toEwkView(clientInfo)->doneWithTouchEvent(event, wasEventHandled);
> +#else
> +    UNUSED_PARAM(event);
> +    UNUSED_PARAM(wasEventHandled);
> +    UNUSED_PARAM(clientInfo);
> +#endif
> +}
> +

Whole function can be wrapped by TOUCH_EVENTS macro.

[Mikhail Pozdnyakov]

Comment on attachment 206292 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:28
> +#include <stdbool.h>

Could you please explain why it is needed?

> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:59
> +    WKViewDoneWithTouchEventCallback                 doneWithTouchEvent;

I'm not sure about the name.. didCompleteTouchEvent? Think we should ask Kenneth

[Eunmi Lee]

Comment on attachment 206292 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:74
>> +    case WebEvent::TouchStart:
> 
> Whole function should be wrapped by TOUCH_EVENTS, isn't it?

No, it is because other WebEvent::Type can be added later.

>> Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:132
>> +#endif
> 
> For other platforms, 122-123 line are unnecessary.

Yes, that can be moved in to the PLAFORM(EFL).

>> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:167
>> +
> 
> Whole function can be wrapped by TOUCH_EVENTS macro.

I don't think so, because doneWithTouchEvent function is necessary (below 178 line) even though TOUCH_EVENTS is not enabled.

[Eunmi Lee]

Comment on attachment 206292 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:28
>> +#include <stdbool.h>
> 
> Could you please explain why it is needed?

The "bool" type is used in the below 43 line, and we need that if this header file is included in the C application.

>> Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:59
>> +    WKViewDoneWithTouchEventCallback                 doneWithTouchEvent;
> 
> I'm not sure about the name.. didCompleteTouchEvent? Think we should ask Kenneth

I think doneWithTouchEvent is good for recognizing because it is same with name used in the WebKit codes.

>>> Source/WebKit2/UIProcess/API/C/efl/WKAPICastEfl.h:74
>>> +    case WebEvent::TouchStart:
>> 
>> Whole function should be wrapped by TOUCH_EVENTS, isn't it?
> 
> No, it is because other WebEvent::Type can be added later.

As discussion, we don't need other WebEvent::Type now, so I will wrap this function with TOUCH_EVENTS.

>>> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:167
>>> +
>> 
>> Whole function can be wrapped by TOUCH_EVENTS macro.
> 
> I don't think so, because doneWithTouchEvent function is necessary (below 178 line) even though TOUCH_EVENTS is not enabled.

I will wrap this function with TOUCH_EVENTS and wrap 178 line with TOUCH_EVENTS too.

[Eunmi Lee]

Created attachment 206545 [details]
Patch

Update for comments.

[Ryuan Choi]

(In reply to comment #21)
> Created an attachment (id=206545) [details]
> Patch
> 
> Update for comments.

My comments are all adressed.

I think that we have only naming issue, right?
Anyway, Some API names are already different from WebCore's.

[Chris Dumez]

Comment on attachment 206545 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:122
> +#if PLATFORM(EFL)

Do we really need this #ifdef here? Wouldn't it compile for Qt port without it?

[Eunmi Lee]

Comment on attachment 206545 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/WebViewClient.cpp:122
>> +#if PLATFORM(EFL)
> 
> Do we really need this #ifdef here? Wouldn't it compile for Qt port without it?

We need "#if PLATFORM(EFL)" here because 126 line has EFL specific code. (EwkTouchEvent). This file is used only in the EFL now but it can be used in the other port later.

If you concern about the 123~124 line they are not EFL specific code, I think that lines have to be in the #if.
Because I used notImplemented() if it is not PLATORM(EFL) and notImplemented() will be wrong if 123~124 line is out of the #if.

[Eunmi Lee]

(In reply to comment #18)
> (From update of attachment 206292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206292&action=review
> 
> > Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:28
> > +#include <stdbool.h>
> 
> Could you please explain why it is needed?
> 
> > Source/WebKit2/UIProcess/API/C/CoordinatedGraphics/WKView.h:59
> > +    WKViewDoneWithTouchEventCallback                 doneWithTouchEvent;
> 
> I'm not sure about the name.. didCompleteTouchEvent? Think we should ask Kenneth

Mikhail and Kenneth,
How about change the name to "didReceiveResultOfTouchEvent"?
It's because that callback is used to receive the result whether touch event is handled or not.

[Kenneth Rohde Christiansen]

> 
> Mikhail and Kenneth,
> How about change the name to "didReceiveResultOfTouchEvent"?
> It's because that callback is used to receive the result whether touch event is handled or not.

I dont think that is more clear at all.

What about didFinishProcessingTouchEvent or similar?

[Kenneth Rohde Christiansen]

Comment on attachment 206545 [details]
Patch

You are not even using didCompleteTouchEvent. doneWithTouchEvent is a great name

[Eunmi Lee]

Thanks for review :)
Then, let's use doneWithTouchEvent.

(In reply to comment #27)
> (From update of attachment 206545 [details])
> You are not even using didCompleteTouchEvent. doneWithTouchEvent is a great name

[WebKit Commit Bot]

Comment on attachment 206545 [details]
Patch

Rejecting attachment 206545 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 206545, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 file Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp
Hunk #1 succeeded at 159 with fuzz 1 (offset 5 lines).
Hunk #2 succeeded at 185 (offset 6 lines).
patching file Source/WebKit2/UIProcess/efl/ViewClientEfl.h
Hunk #1 FAILED at 56.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit2/UIProcess/efl/ViewClientEfl.h.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Kenneth Rohde Christiansen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/1133438

[Eunmi Lee]

Created attachment 207319 [details]
Patch

Rebase.

[Ryuan Choi]

Comment on attachment 207319 [details]
Patch

Try once more

[WebKit Commit Bot]

Comment on attachment 207319 [details]
Patch

Clearing flags on attachment: 207319

Committed r153048: <http://trac.webkit.org/changeset/153048>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.