Bug 110085

Summary: [EFL][WK2] Add doneWithTouchEvent callback to the WKViewClient.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit EFLAssignee: Eunmi Lee <enmi.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, commit-queue, gyuyoung.kim, gyuyoung.kim, hugo.lima, kenneth, lucas.de.marchi, luciano.wolf, luiz, mikhail.pozdnyakov, noam, rakuco, ryuan.choi, webkit.review.bot, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 108915    
Bug Blocks: 102643, 111702    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Eunmi Lee 2013-02-18 01:10:41 PST
Add doneWithTouchEvent callback to the WKViewClient.
Comment 1 Eunmi Lee 2013-02-18 01:17:04 PST
Created attachment 188814 [details]
Patch
Comment 2 EFL EWS Bot 2013-02-18 02:59:54 PST
Comment on attachment 188814 [details]
Patch

Attachment 188814 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16604860
Comment 3 Chris Dumez 2013-02-19 00:31:37 PST
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.
Comment 4 Gyuyoung Kim 2013-02-19 01:00:30 PST
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
Comment 5 Eunmi Lee 2013-03-04 02:23:57 PST
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.
Comment 6 Eunmi Lee 2013-03-04 02:37:57 PST
Created attachment 191184 [details]
Patch

Update for comments.
Comment 7 Eunmi Lee 2013-03-13 23:44:09 PDT
Created attachment 193077 [details]
Patch

Rebased for changed Bug 108915.
Comment 8 Eunmi Lee 2013-03-18 23:30:14 PDT
Created attachment 193742 [details]
Patch

Rebased.
Comment 9 Eunmi Lee 2013-07-03 21:56:48 PDT
Created attachment 206049 [details]
Patch

Rebased.
Comment 10 Gyuyoung Kim 2013-07-03 22:12:22 PDT
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 ?
Comment 11 Noam Rosenthal 2013-07-03 22:16:44 PDT
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?
Comment 12 Eunmi Lee 2013-07-04 04:07:53 PDT
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.
Comment 13 Luciano Wolf 2013-07-04 07:47:10 PDT
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.
Comment 14 Mikhail Pozdnyakov 2013-07-04 08:38:21 PDT
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?
Comment 15 Eunmi Lee 2013-07-08 22:39:53 PDT
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().
Comment 16 Eunmi Lee 2013-07-08 23:01:13 PDT
Created attachment 206292 [details]
Patch

Rebase and add ASSERT.
Comment 17 Ryuan Choi 2013-07-12 06:01:56 PDT
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.
Comment 18 Mikhail Pozdnyakov 2013-07-12 06:29:48 PDT
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
Comment 19 Eunmi Lee 2013-07-12 06:30:17 PDT
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.
Comment 20 Eunmi Lee 2013-07-12 06:53:34 PDT
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.
Comment 21 Eunmi Lee 2013-07-12 07:27:55 PDT
Created attachment 206545 [details]
Patch

Update for comments.
Comment 22 Ryuan Choi 2013-07-15 16:02:18 PDT
(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.
Comment 23 Chris Dumez 2013-07-16 00:07:02 PDT
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?
Comment 24 Eunmi Lee 2013-07-17 23:05:41 PDT
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.
Comment 25 Eunmi Lee 2013-07-18 23:21:58 PDT
(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.
Comment 26 Kenneth Rohde Christiansen 2013-07-23 01:58:20 PDT
> 
> 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?
Comment 27 Kenneth Rohde Christiansen 2013-07-23 02:01:06 PDT
Comment on attachment 206545 [details]
Patch

You are not even using didCompleteTouchEvent. doneWithTouchEvent is a great name
Comment 28 Eunmi Lee 2013-07-23 04:07:23 PDT
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
Comment 29 WebKit Commit Bot 2013-07-23 04:20:32 PDT
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
Comment 30 Eunmi Lee 2013-07-23 04:33:35 PDT
Created attachment 207319 [details]
Patch

Rebase.
Comment 31 Ryuan Choi 2013-07-23 04:44:38 PDT
Comment on attachment 207319 [details]
Patch

Try once more
Comment 32 WebKit Commit Bot 2013-07-23 05:34:34 PDT
Comment on attachment 207319 [details]
Patch

Clearing flags on attachment: 207319

Committed r153048: <http://trac.webkit.org/changeset/153048>
Comment 33 WebKit Commit Bot 2013-07-23 05:34:40 PDT
All reviewed patches have been landed.  Closing bug.